fix(role-instance): fix update role instance status conflict#220
fix(role-instance): fix update role instance status conflict#220cheyang merged 6 commits intosgl-project:mainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate RoleInstance status update conflicts by switching the status update to a RetryOnConflict pattern that re-GETs the latest object before updating the status subresource.
Changes:
- Add
retry.RetryOnConflictaround RoleInstance status updates. - Re-fetch the RoleInstance (
Getby namespace/name) before callingStatus().Update.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Syspretor
force-pushed
the
fix/update-conflict
branch
from
ae05c6a to
374b6e7
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to address conflicts when updating RoleInstance / RoleInstanceSet status by changing how status writes are performed during reconciliation.
Changes:
- Add
RetryOnConflict+ re-Getbefore updatingRoleInstancestatus to reduce update conflicts. - Switch
RoleInstanceSetstatus update fromStatus().Patch(MergeFrom(...))toStatus().Update(...)within an existingRetryOnConflictloop.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/reconciler/roleinstanceset/statelessmode/status.go | Changes status write to Status().Update inside the existing conflict-retry loop. |
| pkg/reconciler/roleinstance/instance_status.go | Wraps RoleInstance status updates in RetryOnConflict and refreshes object before updating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce/update conflicts when writing RoleInstance / RoleInstanceSet status, in the presence of concurrent status writers (e.g., readiness/inplace controllers).
Changes:
- Switched
RoleInstanceSetstateless-mode status writes fromStatus().Patch(MergeFrom(...))toStatus().Update(...)inside a conflict-retry loop. - Updated
RoleInstancestatus writes to useRetryOnConflictwith a fresh GET, and attempted to preserve externally-owned conditions during status updates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/reconciler/roleinstanceset/statelessmode/status.go |
Changes stateless RoleInstanceSet status persistence from Patch to Update (still retried on conflict). |
pkg/reconciler/roleinstance/instance_status.go |
Adds retry-on-conflict status updates and condition “merge” logic intended to prevent overwriting concurrent condition updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR aims to address status update conflicts for RoleInstance / RoleInstanceSet reconciliation by changing how status updates are performed, including retry-on-conflict behavior and preserving concurrently-written status conditions.
Changes:
- Switch
RoleInstanceSet(stateless mode) status update toStatus().Update()within aRetryOnConflictloop. - Add
RetryOnConflict+ live-object refetch forRoleInstancestatus updates, attempting to preserve externally-owned conditions. - Introduce a helper to define which
RoleInstancecondition types are controller-owned vs externally-owned.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/reconciler/roleinstanceset/statelessmode/status.go | Changes RoleInstanceSet status write path to use Status().Update() after a fresh GET in conflict retries. |
| pkg/reconciler/roleinstance/instance_status.go | Adds conflict-retry status updates for RoleInstance and attempts to preserve externally-written conditions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR aims to address RoleInstance status update conflicts caused by concurrent writers by changing how status updates are performed (retrying on conflicts and attempting to preserve externally-owned conditions).
Changes:
- Update RoleInstanceSet (stateless mode) status update logic to use
Status().Update()after a fresh GET withinRetryOnConflict. - Add conflict-retry logic to RoleInstance status updates and attempt to preserve externally-owned status conditions across updates.
- Introduce an explicit “controller-owned condition types” set to decide which conditions should/shouldn’t be preserved.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/reconciler/roleinstanceset/statelessmode/status.go | Switches RoleInstanceSet status write path to Status().Update() inside a retry loop. |
| pkg/reconciler/roleinstance/instance_status.go | Adds retry-on-conflict status updates and logic intended to preserve externally-owned conditions during status writes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce/update-conflict errors when updating RoleInstance / RoleInstanceSet status by switching to conflict-retry update patterns and attempting to preserve concurrently-written status conditions.
Changes:
RoleInstanceSet(stateless mode): switch status write toStatus().Update()insideRetryOnConflict.RoleInstance: addRetryOnConflictstatus update flow that refetches the live object and attempts to carry forward externally-owned conditions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/reconciler/roleinstanceset/statelessmode/status.go | Updates RoleInstanceSet status using refetch + RetryOnConflict + Status().Update(). |
| pkg/reconciler/roleinstance/instance_status.go | Adds conflict-retry status update and condition “ownership” logic to avoid clobbering concurrent condition writers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR aims to address RoleInstance / RoleInstanceSet status update conflicts by adjusting how status updates are performed under concurrent writers (Kubernetes resourceVersion conflicts and concurrent status condition updates).
Changes:
- Switch
RoleInstanceSet(stateless mode) status updates to useStatus().Update()(still underRetryOnConflict). - Add
RetryOnConflicttoRoleInstancestatus updates and attempt to preserve “externally-owned” status conditions during updates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/reconciler/roleinstanceset/statelessmode/status.go | Changes status update strategy for RoleInstanceSet to use Status().Update() within a conflict-retry loop. |
| pkg/reconciler/roleinstance/instance_status.go | Introduces conflict-retry for RoleInstance status updates and condition merging logic intended to avoid clobbering concurrent condition writers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil | ||
| } | ||
| return err | ||
| } |
| // The rule for inclusion is: | ||
| // - Controller-computed types (Ready, AllPodsReady, FailedScale, FailedUpdate) are | ||
| // always generated fresh each reconcile and must win over the live object. | ||
| // - RoleInstanceInPlaceUpdateReady and RoleInstanceCustomReady are written by | ||
| // external controllers, but setInstanceConditions always copies them from | ||
| // instance.Status into newStatus (via getInstanceInplaceUpdateReadyCondition and | ||
| // the custom-condition passthrough). They are therefore already present in | ||
| // newStatus.Conditions, so they must be listed here to prevent a duplicate entry | ||
| // being appended from the live clone. | ||
| func controllerOwnedConditionTypes() sets.Set[workloadsv1alpha2.RoleInstanceConditionType] { | ||
| return sets.New[workloadsv1alpha2.RoleInstanceConditionType]( | ||
| // Computed by this controller on every reconcile. | ||
| workloadsv1alpha2.RoleInstanceReady, | ||
| workloadsv1alpha2.RoleInstanceAllPodsReady, | ||
| workloadsv1alpha2.RoleInstanceFailedScale, | ||
| workloadsv1alpha2.RoleInstanceFailedUpdate, | ||
| // Written externally but always copied into newStatus by setInstanceConditions, | ||
| // so they are already present and must not be appended again from the live clone. | ||
| workloadsv1alpha2.RoleInstanceInPlaceUpdateReady, | ||
| workloadsv1alpha2.RoleInstanceCustomReady, |
| // - RoleInstanceInPlaceUpdateReady and RoleInstanceCustomReady are written by | ||
| // external controllers, but setInstanceConditions always copies them from | ||
| // instance.Status into newStatus (via getInstanceInplaceUpdateReadyCondition and | ||
| // the custom-condition passthrough). They are therefore already present in | ||
| // newStatus.Conditions, so they must be listed here to prevent a duplicate entry | ||
| // being appended from the live clone. | ||
| func controllerOwnedConditionTypes() sets.Set[workloadsv1alpha2.RoleInstanceConditionType] { | ||
| return sets.New[workloadsv1alpha2.RoleInstanceConditionType]( | ||
| // Computed by this controller on every reconcile. | ||
| workloadsv1alpha2.RoleInstanceReady, | ||
| workloadsv1alpha2.RoleInstanceAllPodsReady, | ||
| workloadsv1alpha2.RoleInstanceFailedScale, | ||
| workloadsv1alpha2.RoleInstanceFailedUpdate, | ||
| // Written externally but always copied into newStatus by setInstanceConditions, | ||
| // so they are already present and must not be appended again from the live clone. | ||
| workloadsv1alpha2.RoleInstanceInPlaceUpdateReady, | ||
| workloadsv1alpha2.RoleInstanceCustomReady, | ||
| ) |
| clone.Status = *newStatus | ||
| clone.Status.Conditions = append(clone.Status.Conditions, liveCustomConditions...) |
Ⅰ. Motivation
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.