-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
operator: do not reconcile all IOPs if REVISION
env is set to blank
#51250
Conversation
Signed-off-by: Anurag Aggarwal <[email protected]>
😊 Welcome @kanurag94! This is either your first contribution to the Istio istio repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
Hi @kanurag94. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
/retest |
/retest all |
@tanujd11: The
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/test all |
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.
This seems like it could be a breaking change?
@howardjohn I think this is very unlikely to cause a breaking behavior. It may only be a misconfiguration if someone added REVISION env but set it to blank. We can still call it out in release notes. |
Signed-off-by: Anurag Aggarwal <[email protected]>
Signed-off-by: Anurag Aggarwal <[email protected]>
operatorRevision, _ := os.LookupEnv("REVISION") | ||
if operatorRevision != "" && operatorRevision != iop.Spec.Revision { | ||
scope.Infof("Ignoring IstioOperator CR %s with revision %s, since operator revision is %s.", iopName, iop.Spec.Revision, operatorRevision) | ||
if operatorRevision, revisionEnvExists := os.LookupEnv("REVISION"); revisionEnvExists && operatorRevision != iop.Spec.Revision { |
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.
At the very least, I think REVISION="" should keep the capability to reconcile iop.Spec.Revision="default", otherwise it will be inconsistent with the existing installation revision semantics.
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 @hanxiaop for reviewing. That makes sense. I have added the change. PTAL
Unrelated to this PR, I think there is one more place I recently came across this inconsistency https://github.com/istio/istio/blob/master/operator/pkg/helmreconciler/reconciler.go#L605
Signed-off-by: Anurag Aggarwal <[email protected]>
@howardjohn @hanxiaop gentle ping |
Gentle ping @hanxiaop |
🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2024-05-31. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions. Created by the issue and PR lifecycle manager. |
Please provide a description of this PR:
Currently operator which has
REVISION
env set to""
or unset tries to reconcile IOPs of all revisions. Thus there is no easy way to allow migration from adefault
install of istio to a revisioned install without changing REVISION to bedefault
at multiple IOPs to avoid this behavior. However that introduces multiple changes needed in the environment and possibly restarting all gateways deployments since now the IOPs are expected to have the revision as well.With this PR the intent to only reconcile a non revisioned IOP (blank revision) and not others is honored when REVISION env is present but blank. If env is not set, the behavior remains same.
To help us figure out who should review this PR, please put an X in all the areas that this PR affects.
Please check any characteristics that apply to this pull request.