-
Notifications
You must be signed in to change notification settings - Fork 155
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix addon controller reconciling too frequently #13252
Conversation
d806c29
to
49003fc
Compare
49003fc
to
6d2655d
Compare
/retest |
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.
/approve
LGTM label has been added. Git tree hash: 7c0daa622e1bcdf674ef842a5a1661239bc8f163
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: embik 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 |
/retest |
/cherrypick release/v2.25 |
@xrstf: new pull request created: #13262 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/test-infra repository. |
/cherrypick release/v2.24 |
@xrstf: new pull request created: #13263 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/test-infra repository. |
What this PR does / why we need it:
In #4773 we tried to make the addon controller reconcile less frequently. The logic was to only react to Cluster changes if the
AddonControllerReconcilingSuccess
condition changed. The only controller setting this condition is the addon controller itself (looking back at the original PR, I don't really understand it... but I was young and dumber).However this condition on the Cluster object isn't really saying much. If you have 3 addons, one of them fails, it is basically random which status the
AddonControllerReconcilingSuccess
on the Cluster object will have.Even worse: Suppose all your addons are happy and healthy. Now you change a field like the Cluster Owner or any other field that is part of the addon TemplateData. Even though you changed the Cluster object, no reconciliation will happen because neither the CNI values nor the condition has changed.
Additionally, Addon objects are reconciled even when their status changes. This isn't much of a problem right now, because there is only 1 Condition and it only gets set exactly once and never heartbeats, but if the status is extended (an I have a branch ready for that 馃榿 ), then status changes on an addon should also be ignored.
To that effect, this PR adjusts the watches:
What type of PR is this?
/kind cleanup
Does this PR introduce a user-facing change? Then add your Release Note here:
Documentation: