operator: do not reconcile all IOPs if `REVISION` env is set to blank #51250

kanurag94 · 2024-05-27T08:44:42Z

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 a default install of istio to a revisioned install without changing REVISION to be default 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.

"" (BLANK) means either non revisioned or IOPs with revision default will be reconciled.

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.

Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

Signed-off-by: Anurag Aggarwal <[email protected]>

istio-policy-bot · 2024-05-27T08:44:47Z

😊 Welcome @kanurag94! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

istio-testing · 2024-05-27T08:44:53Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

zirain · 2024-05-28T01:44:49Z

/ok-to-test

tanujd11 · 2024-05-28T05:15:40Z

/retest

tanujd11 · 2024-05-28T05:16:27Z

/retest all

istio-testing · 2024-05-28T05:16:30Z

@tanujd11: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test bookinfo-build
/test gencheck
/test integ-ambient
/test integ-basic-arm64
/test integ-cni
/test integ-distroless
/test integ-ds
/test integ-helm
/test integ-ipv6
/test integ-operator-controller
/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster
/test integ-security
/test integ-security-istiodremote
/test integ-security-multicluster
/test integ-telemetry
/test integ-telemetry-discovery
/test integ-telemetry-istiodremote
/test integ-telemetry-mc
/test lint
/test macbuildcheck
/test release-notes
/test release-test
/test unit-tests
/test unit-tests-arm64

The following commands are available to trigger optional jobs:

/test benchmark
/test experimental-tracing
/test integ-ambient-calico
/test integ-assertion

Use /test all to run the following jobs that were automatically triggered:

gencheck_istio
integ-ambient_istio
integ-basic-arm64_istio
integ-cni_istio
integ-distroless_istio
integ-ds_istio
integ-helm_istio
integ-ipv6_istio
integ-operator-controller_istio
integ-pilot-istiodremote-mc_istio
integ-pilot-istiodremote_istio
integ-pilot-multicluster_istio
integ-pilot_istio
integ-security-istiodremote_istio
integ-security-multicluster_istio
integ-security_istio
integ-telemetry-discovery_istio
integ-telemetry-istiodremote_istio
integ-telemetry-mc_istio
integ-telemetry_istio
lint_istio
macbuildcheck_istio
release-notes_istio
release-test_istio
unit-tests-arm64_istio
unit-tests_istio

In response to this:

/retest all

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.

tanujd11 · 2024-05-28T05:16:52Z

/test all

howardjohn

This seems like it could be a breaking change?

kanurag94 · 2024-05-28T15:06:32Z

@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]>

hanxiaop · 2024-05-31T14:11:37Z

operator/pkg/controller/istiocontrolplane/istiocontrolplane_controller.go

- 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 {


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.

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]>

kanurag94 · 2024-06-07T11:26:40Z

@howardjohn @hanxiaop gentle ping

kanurag94 · 2024-06-24T06:05:43Z

Gentle ping @hanxiaop

istio-policy-bot · 2024-07-16T05:04:48Z

🚧 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.

do not reconcile all IOPs if REVISION env is set to blank

c6f1224

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 requested a review from a team as a code owner May 27, 2024 08:44

istio-policy-bot added area/environments area/user experience labels May 27, 2024

istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test labels May 27, 2024

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels May 28, 2024

howardjohn reviewed May 28, 2024

View reviewed changes

add release note

e299942

Signed-off-by: Anurag Aggarwal <[email protected]>

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 30, 2024

update golden template

d86e61f

Signed-off-by: Anurag Aggarwal <[email protected]>

hanxiaop reviewed May 31, 2024

View reviewed changes

add revision to be reconciled when REVISION is blank

1b2dd5f

Signed-off-by: Anurag Aggarwal <[email protected]>

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jul 1, 2024

istio-policy-bot closed this Jul 16, 2024

istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: do not reconcile all IOPs if `REVISION` env is set to blank #51250

operator: do not reconcile all IOPs if `REVISION` env is set to blank #51250

kanurag94 commented May 27, 2024 •

edited

Loading

istio-policy-bot commented May 27, 2024

istio-testing commented May 27, 2024

zirain commented May 28, 2024

tanujd11 commented May 28, 2024

tanujd11 commented May 28, 2024

istio-testing commented May 28, 2024

tanujd11 commented May 28, 2024

howardjohn left a comment

kanurag94 commented May 28, 2024

hanxiaop May 31, 2024

kanurag94 May 31, 2024

kanurag94 commented Jun 7, 2024

kanurag94 commented Jun 24, 2024

istio-policy-bot commented Jul 16, 2024

operator: do not reconcile all IOPs if REVISION env is set to blank #51250

operator: do not reconcile all IOPs if REVISION env is set to blank #51250

Conversation

kanurag94 commented May 27, 2024 • edited Loading

istio-policy-bot commented May 27, 2024

istio-testing commented May 27, 2024

zirain commented May 28, 2024

tanujd11 commented May 28, 2024

tanujd11 commented May 28, 2024

istio-testing commented May 28, 2024

tanujd11 commented May 28, 2024

howardjohn left a comment

Choose a reason for hiding this comment

kanurag94 commented May 28, 2024

hanxiaop May 31, 2024

Choose a reason for hiding this comment

kanurag94 May 31, 2024

Choose a reason for hiding this comment

kanurag94 commented Jun 7, 2024

kanurag94 commented Jun 24, 2024

istio-policy-bot commented Jul 16, 2024

operator: do not reconcile all IOPs if `REVISION` env is set to blank #51250

operator: do not reconcile all IOPs if `REVISION` env is set to blank #51250

kanurag94 commented May 27, 2024 •

edited

Loading