-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api): Support custom InstaScale container image #120
feat(api): Support custom InstaScale container image #120
Conversation
/assign @anishasthana @jbusche |
This could cause issues in cases where the requested controller version isn't compatible with the CRD versions that were installed with the CodeFlare operator. |
Right, that occurred to me as I implemented it. We may want to put a strategy in place to mitigate that risk. Maybe @jbusche and @anishasthana can provide more details about #115 before moving forward. |
I'm curious -- if we only update the image with bundle updates, shouldn't OLM take care of this for us? (The bundle will include both the latest CRDs and the latest controller, right?) |
In the upgrade case, my understanding is the CRDs will be updated by OLM, but the controller will still run a custom image, if it's been specified in the InstaScale resource. Theoretically, even the deployment resources, owned by the InstaScale resource, and generated by the operator, may become incompatible with the custom / past version controller image. If such case would arise in the lineage of the InstaScale controller, the CodeFlare operator could adapt its logic, provided with the controller version. Similarly, incompatibilities between the specified custom image version and the installed CRD versions could be detected, and possibly reported into the InstaScale resource conditions. |
If you're testing a new version of InstaScale that relies on some new changes to the |
Ah yeah, I was thinking of images in a different context.. Perhaps for now we update the field documentation to make it very clear that using images that are not the default could result in broken deployments?
|
+1, documenting the risk is the most straightforward way to mitigate it in the short term.
Sure, it'll be more difficult to address the forward compatibility case, as described by @KPostOffice, with this approach, but I think it'll be less frequent than the backward compatibility case, that can be convenient to revert back to a previous version, while OLM makes it very hard to do. |
@astefanutti This is great - I'm trying to think how I would test it today. As for specifying a custom image, I understand there are times when a different image might break things, for example; specifying too old of an image, or jumping ahead to something too new. I wouldn't suggest we encourage people to mess with the default images unless directed by CodeFlare support. But I've been on support for a previous operator for two + years, and there were many times when customers (and dev/support) quickly needed an updated image:
|
++ that makes sense, I think the change is good. @astefanutti Has clearly noted the potential for undefined behavior as well. this /lgtm but I haven't tested it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@astefanutti We probably need a testcase similar to these
Hi @astefanutti, OK with @tedhtchang 's help, I tried it out. Here's what I did:
and under spec: add something like this:
Looks great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tested fine on my cluster
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jbusche The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
@jbusche great, thanks for the feedback and the detailed instructions. I'll have them adapted and included as part of project-codeflare/codeflare-sdk#137, so it's documented how to test local changes manually. |
@tedhtchang I've just created #124. |
Fixes #115.