Skip to content

Skip reconciliation when using sailoperator.io/ignore annotation#1256

Merged
istio-testing merged 4 commits intoistio-ecosystem:mainfrom
bmangoen:feat/ignore-update-when-annotation
Oct 15, 2025
Merged

Skip reconciliation when using sailoperator.io/ignore annotation#1256
istio-testing merged 4 commits intoistio-ecosystem:mainfrom
bmangoen:feat/ignore-update-when-annotation

Conversation

@bmangoen
Copy link
Copy Markdown
Contributor

@bmangoen bmangoen commented Oct 6, 2025

What type of PR is this?

  • Enhancement / New Feature
  • Bug Fix
  • Refactor
  • Optimization
  • Test
  • Documentation Update

What this PR does / why we need it:

With this fix, setting sailoperator.io/ignore: "true" on a resource will skip its reconciliation.
This is a temporary hack until we implement the Manifest Customization feature

Which issue(s) this PR fixes:

Fixes #1242

Additional information:

Signed-off-by: bmangoen <bmangoen@redhat.com>
@bmangoen bmangoen requested a review from a team as a code owner October 6, 2025 09:04
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 6, 2025

Codecov Report

❌ Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.48%. Comparing base (38027c9) to head (682c3dc).
⚠️ Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
pkg/predicate/predicate.go 57.14% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1256      +/-   ##
==========================================
- Coverage   77.78%   77.48%   -0.31%     
==========================================
  Files          44       44              
  Lines        2823     2834      +11     
==========================================
  Hits         2196     2196              
- Misses        521      529       +8     
- Partials      106      109       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: bmangoen <bmangoen@redhat.com>
@dgn
Copy link
Copy Markdown
Collaborator

dgn commented Oct 8, 2025

Could you add a step to one of the integration tests, making a change to an object with the annotation and making sure that the controller does not reconcile it?

Copy link
Copy Markdown
Collaborator

@dgn dgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm so far, but an integration test would be good

@t3mi
Copy link
Copy Markdown
Contributor

t3mi commented Oct 8, 2025

Thanks for taking the request into action! Just wanted to clarify that initial request was related only to the operator managed resources, e.g. only Istio / IstioRevision / IstioRevisionTag / IstioCNI / ZTunnel should be the target for the annotation.

This commit adds the step to test the skip reconciliation when the sailoperator.io/ignore annotation is set to true on a resource.

Signed-off-by: bmangoen <bmangoen@redhat.com>
@dgn
Copy link
Copy Markdown
Collaborator

dgn commented Oct 9, 2025

Thanks for taking the request into action! Just wanted to clarify that initial request was related only to the operator managed resources, e.g. only Istio / IstioRevision / IstioRevisionTag / IstioCNI / ZTunnel should be the target for the annotation.

Interesting. What's your use case for that? The issue you linked as an example was caused by Azure making changes to a WebhookConfiguration, which resulted in the Sail Operator reverting those changes. Annotating the WebhookConfiguration would help in this case. Why would you want to pause reconciliation of an operator-owned resource?

@t3mi
Copy link
Copy Markdown
Contributor

t3mi commented Oct 9, 2025

Correct. Until there's a solution to ignore specific fields on child resources, I’d prefer to pause the reconciliation of selected operator-owned resources.
For example, with the webhook configuration, there's currently no way to set annotations on the validation webhook during installation. As a result, I observe that the sail-operator re-installs the istiod Helm chart every 1–2 seconds in the cluster. This interval is too short (unless it could be relaxed) to manually set an annotation on the child resource before the operator triggers another reconciliation.
I'm seeing a similar issue with IstioCNI, although I haven’t yet tracked down the root cause — I just noticed that the Helm chart revision keeps increasing continuously.

skriss
skriss previously requested changes Oct 9, 2025
This commit adds more unit tests cases for IgnoreUpdateWhenAnnotation predicate.

Signed-off-by: bmangoen <bmangoen@redhat.com>
@bmangoen bmangoen requested a review from skriss October 10, 2025 09:20
@dgn
Copy link
Copy Markdown
Collaborator

dgn commented Oct 10, 2025

Correct. Until there's a solution to ignore specific fields on child resources, I’d prefer to pause the reconciliation of selected operator-owned resources. For example, with the webhook configuration, there's currently no way to set annotations on the validation webhook during installation. As a result, I observe that the sail-operator re-installs the istiod Helm chart every 1–2 seconds in the cluster. This interval is too short (unless it could be relaxed) to manually set an annotation on the child resource before the operator triggers another reconciliation. I'm seeing a similar issue with IstioCNI, although I haven’t yet tracked down the root cause — I just noticed that the Helm chart revision keeps increasing continuously.

Understood - I don't think that's something we'll implement though, as it just hides bugs in the operator. If the operator is continuously reconciling a resource, there's something wrong (usually another controller is acting on the same resources) and I'd prefer us to work on that root cause (ignoring the other controller's change, for example) than just pausing the reconciliation of a control plane component.

I do see where you're coming from though. You can try adding --log-enqueue-events to your operator cmdline args in the Deployment, that will make sure the operator logs which kube events it's processing. That should show why it is continuously reconciling your IstioCNI resource. If you then file a bug with the logs, we should be able to figure it out.

With respect to the helm revision increasing, that's actually a known issue (#429) but it doesn't necessarily mean that the operator is applying any changes to the cluster. It's just that helm increases the revision count, even if you just rollback to a specific revision (to make sure all resources are in the expected state)

}
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(webhook), webhook)).To(Succeed())

GinkgoWriter.Println("webhook:", webhook)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove debug print?

@bmangoen bmangoen dismissed skriss’s stale review October 15, 2025 08:17

More test cases were added

@istio-testing istio-testing merged commit 2543fb8 into istio-ecosystem:main Oct 15, 2025
15 of 17 checks passed
@istio-testing
Copy link
Copy Markdown
Collaborator

In response to a cherrypick label: new pull request created: #1276

dgn pushed a commit to dgn/sail-operator that referenced this pull request Mar 17, 2026
…io-ecosystem#1256)

* Skip reconciliation when using sailoperator.io/ignore annotation

Signed-off-by: bmangoen <bmangoen@redhat.com>

* Watch all resources with IgnoreUpdateWhenAnnotation predicate

Signed-off-by: bmangoen <bmangoen@redhat.com>

* fix: add integration tests

This commit adds the step to test the skip reconciliation when the sailoperator.io/ignore annotation is set to true on a resource.

Signed-off-by: bmangoen <bmangoen@redhat.com>

* Add more steps for unit tests

This commit adds more unit tests cases for IgnoreUpdateWhenAnnotation predicate.

Signed-off-by: bmangoen <bmangoen@redhat.com>

---------

Signed-off-by: bmangoen <bmangoen@redhat.com>
Signed-off-by: Daniel Grimm <dgrimm@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Pause reconciliation

5 participants