-
Notifications
You must be signed in to change notification settings - Fork 20
interface: support multiple resourceSnapshot versions across clusters #33
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c231ce4 to
7a85d9f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
7a85d9f to
e087787
Compare
circy9
left a comment
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.
Assuming members are all running snapshot v1 now, if snapshot v2 is discovered and rolled out to member1 (still waiting for availability) and not to member2 yet, how does the per-CRP and per-member status look like?
It's the exact content as the example I show in the PR description. CRP whole status shows |
891db47 to
339275e
Compare
…ns across clusters Signed-off-by: Wantong Jiang <wantjian@microsoft.com>
Signed-off-by: Wantong Jiang <wantjian@microsoft.com>
aa31336 to
215172c
Compare
215172c to
1890ad9
Compare
circy9
left a comment
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.
Based on offline discussions, we agree to do the following:
-
For all strategies, per-cluster observedresourceindex and RolloutStarted/Overridden/WorkSynchronized/Applied/Available should reflect the current observedresourceindex status of the member cluster (consistent with the corresponding binding status).
-
For external rollout strategy, per-CRP observedresourceindex and RolloutStarted/Overridden/WorkSynchronized/Applied/Available should be an aggregate of the per-cluster status:
- observedresourceindex is non-empty only if all the clusters have the same observedresourceindex.
- When observedresourceindex is empty, all the conditions will be unknonw.
- When observedresourceindex is set,
- If any one of the cluster is unknown, the aggregated condition will be unknown.
- (Else) if any one of the cluster is false, the aggregated condition is false.
- If all of the clusters are true, the aggregated condition is true.
- For rollingupdate strategy, per-CRP observedresourceindex is the lastest resourceIndex and the conditions are for the latest.
Signed-off-by: Wantong Jiang <wantjian@microsoft.com>
Description of your changes
This PR is the interface update to support showing multiple resourceSnapshot versions across targeted clusters on CRP.
Current behavior:
In current CRP status, there's
crp.status.ObservedResourceIndexthat is always set to the index of the latest resourceSnapshot currently exists on the hub cluster as current rollingUpdate rollout strategy only supports rolling to the latest resourceSnapshot version. Thecrp.status.Conditions(CRP status as a whole) as well ascrp.status.placementStatuses(each targeted member clsuter status) are both based on this index.This has several cons:
In above example, rollout does happen to member1, it's just waiting for the resource to become available. But in the CRP condition, it still shows "RolloutNotStarted". And in member2, we actually loses more information that it currently is still running on the earlier version.
Proposed API
The proposed solution is to modify the definition of
crp.status.ObservedResourceIndexfrom the index of latest resourceSnapshot observed on hub cluster to the index of the latest resourceSnapshot observed across all targeted memberclusters. For example, when the currently latest index is 4 on hub cluster but 2, 2, 3 on member clusters respectively, currentcrp.statusObservedResourceIndexis 4, but will be 3 with the proposed changed. This does not break existing rollingUpdate strategy as all member clusters will be updated to 4 eventually and crp will reach the final terminate state. And with updateRun or other external rollout strategy, users can find it more accurate to reflect the current CRP status: CRP now shows the version that user rolls out, not the default-latest version.There are two options to handle per-cluster status:
crp.status.ObservedResourceIndex. This aligns with current API.ObservedResourceIndexinside per-cluster placement status. And each cluster's status is based on this version. For example, when current version on each cluster is 2, 3, 3 respectively, the first cluster shows RolloutNotStarted or RolloutUnknown as it's not being updated to version 3 yet, while with this new option, it's RolloutStarted and observedResourceIndex is 2. This somehow breaks current API but it displays more accurate status.With the proposed solution, we can easily support different rollout strategy, show cluster status more accurately, and can support rollout back in the future.
Revised Apr 28:
When rollout strategy type is set to
#### Revised Apr 29:External, rollout is managed by an external controller and crp is not aware of the actual rollout plan/target. In this case, settingObservedResourceIndexandRolloutStarted/Applied/Availableconditions in the crp whole status does not make sense and can cause confusion. In this case, we only demonstrate the per-cluster observedResourceIndex and conditions incrp.statusResoucePlacementStatusesarray. CRP condition will only showScheduledandRolloutStarted Unknown.Per offline discussion, we have decided that:
For both
RollingUpdateandExternalrollout strategies,ObservedResourceIndexand conditions includingRolloutStarted/Overridden/WorkSynchronized/Applied/Availableshould reflect the current observed resource index of the member cluster and be consistent with the correspondingclusterResourceBindingstatus.CRP status serves as an aggregate of the per-cluster resourcePlacement status.
RollingUpdatestrategy:ObservedResourceIndexis the default-latest resourceSnapshot index and the conditions are based on it.Externalstrategy:ObservedResourceIndexis not empty only if all the member clusters observe the same resource index.When
ObservedResourceIndexis empty, all conditions exceptScheduledshould be inUnknownstate.When
ObservedResourceIndexis not empty, conditions show the aggregation of the clusters' status:Unknown, aggregated state isUnknown.False, aggregated state isFalse.True, aggregated state isTrue.Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer