This repository has been archived by the owner on May 6, 2022. It is now read-only.
Detect no-op status updates #2531
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
cncf-cla: yes
luksa
force-pushed
the
check_noop_status_update
branch
2 times, most recently
from
e97ae69
to
c18b052
Compare
|
/approve |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
needs-rebase
|
this looks good, just needs to be rebased |
luksa
force-pushed
the
check_noop_status_update
branch
from
c18b052
to
0a045be
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jboyd01 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 |
k8s-ci-robot
removed
the
needs-rebase
|
Rebased & resolved conflicts. |
|
Please merge #2529 first, since it's included in this PR here, but really should end up as being a separate commit. |
These functions try to resolve the service class/plan. If they resolve it successfully, they just modify the instance object, but don't actually update it on the server. If they can't resolve it, the do update the object on the server and they also return an error. It would be cleaner if they only returned an error and let the caller perform the actual update. This way, the functions' behavior is consistent between the two possible outcomes. This commit does exactly that.
…mespacedReferences
luksa
force-pushed
the
check_noop_status_update
branch
from
0a045be
to
61e0c2b
Compare
|
/lgtm |
viviyww
pushed a commit
to viviyww/service-catalog
that referenced
this pull request
May 10, 2019
* Clean up resolveServiceClassRef & similar functions These functions try to resolve the service class/plan. If they resolve it successfully, they just modify the instance object, but don't actually update it on the server. If they can't resolve it, the do update the object on the server and they also return an error. It would be cleaner if they only returned an error and let the caller perform the actual update. This way, the functions' behavior is consistent between the two possible outcomes. This commit does exactly that. * Check for no-op status update when calling updateServiceInstanceCondition * Fix Fake ServiceInstance client so it mutates object's ResourceVersion * Check for no-op status update in resolveClusterReferences & resolveNamespacedReferences * Don't assume updateServiceInstanceStatus() always updates the object
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a
What this PR does / why we need it:
When the controller updates the status of an object (e.g. ServiceInstance), it stops reconciling the object and waits for the watch event to come back from the API server, which is when it then resumes the reconciliation. In some cases, the status update does not produce a watch event, because the controller doesn't actually modify the object in the status update (i.e. the state of the object in the request matches the existing object state in the API server). The controller should detect this and continue reconciling the object instead of stopping.
This PR makes the controller detect such no-op status updates. It also fixes a few other problems, where the controller assumed the object was updated, when it really wasn't.
Merge Checklist:
breaking the chart release and existing clients who provide a
flag that will get an error when they try to update