-
Notifications
You must be signed in to change notification settings - Fork 46
feat(controller): ConfigMap image mapping override #169
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
Conversation
b644848 to
677ecac
Compare
rhuss
left a comment
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.
Thanks for the PR, great stuff :). Here's my first round of review.
My main points are:
- We must not reference RHODS in this upstream operator. Why and where do you need the RHODS version ? (i can't really see why this is needed). This is really important.
- Probably not related to this PR, nevertheless: The enabling of network policies should not be a 'feature flag' (which is about enabling/disabling feature that has its own configuration), but a 'configuration option' (you can combine both, feature flag and configuration option but you should not 'misuse' a feature flag as configuration of a permanent feature)
Lets deal with this first as the rest of the PR might change based on this, So the objective of this PR is to allow the operator to set a new image for a specific major/minor version. The code needs to know the RHODS version in order to know if a config map for If ^ is correct then perhaps at least part of this PR should be mid/downstream ? |
|
I wonder why we need to add the version number at all to the key. Couldn't the operator just unconditionally pick up the image under the key |
Because we've asserted "The major and minor versions of the LLS distribution and the LLS operator must match, but the z-stream version can differ.", if the minor version is upgrade (either manually or automatically) then the operator needs to know to stop using the image and instead switch over to the default for the new version. If what we want is to unconditionally use a specific image, we already have |
677ecac to
64745bf
Compare
|
Updated to remove the inclusion of the version number in the mapping key |
64745bf to
a8b6669
Compare
|
This pull request has merge conflicts that must be resolved before it can be merged. @derekhiggins please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
a8b6669 to
f4d1d86
Compare
f4d1d86 to
d254ac8
Compare
|
@mergify rebase |
✅ Branch has been successfully rebased |
d254ac8 to
ba1d75e
Compare
ba1d75e to
95c5e88
Compare
|
rebased to prompt CI into running (small change to the way the context is passed into ParseImageMappingOverrides to keep the linter happy) |
95c5e88 to
b0236ad
Compare
|
@mergify rebase |
✅ Branch has been successfully rebased |
b0236ad to
5d839b8
Compare
|
@derekhiggins The image overrides tests continue to fail |
Thanks @VaishnaviHire I hadn't noticed the failure, I'll take a look |
5d839b8 to
d432add
Compare
Done, |
|
@mergify rebase |
✅ Branch has been successfully rebased |
d432add to
875598e
Compare
|
@mergify rebase |
✅ Branch has been successfully rebased |
875598e to
6ae368e
Compare
|
@mergify rebase |
o Add operator config detection in configMapUpdatePredicate o Update feature flags when operator config changes o Trigger reconciliation of all LlamaStackDistributions on operator config change Signed-off-by: Derek Higgins <[email protected]>
Implements a mechanism for the Llama Stack Operator to read and apply LLS Distribution image updates from a ConfigMap, enabling independent patching for security fixes or bug fixes without requiring a new LLS Operator. - Add ImageMappingOverrides field to LlamaStackDistributionReconciler - Implement parseImageMappingOverrides() to read image-overrides from ConfigMap - Support symbolic name mapping (e.g., `starter`) to specific images - Included unit tests The operator now reads image overrides from the 'image-overrides' key in the operator ConfigMap, supporting YAML format with version-to-image mappings. Overrides take precedence over default distribution images and are refreshed on each reconciler initialization. Closes: RHAIENG-1079 Signed-off-by: Derek Higgins <[email protected]>
✅ Branch has been successfully rebased |
6ae368e to
c77f6bf
Compare
Dismissing Russ review, as it was based on an older version
After it was made requirements were discuses and the PR adjusted appropriately.
He isn't available at the moment to Ack the PR and we'd like to merge it this week.
feat: ConfigMap image mapping overrides for LLS Distro
Also
Fix for deprecated Endpoints Generated by cursor but output tested here here , search for the string "Found 1 pods in namespace"