-
Notifications
You must be signed in to change notification settings - Fork 74
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
✨ Add mwrSet rolloutStrategy #255
✨ Add mwrSet rolloutStrategy #255
Conversation
/hold |
83e6cfc
to
0479e6e
Compare
/cc @haoqing0110 |
@@ -86,6 +90,23 @@ type LocalPlacementReference struct { | |||
// +kubebuilder:validation:Required | |||
// +kubebuilder:validation:MinLength=1 | |||
Name string `json:"name"` | |||
|
|||
// +optional |
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.
Should here add // +kubebuilder:default={type: All}
?
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.
its already there in the Rollout API definition here
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.
Adding the default value on the top of RolloutStrategy is to ensure even no RolloutStrategy field defined in yaml,
there's will be RolloutStrategy and "type: All" generated.
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.
okay, done.
|
||
// availableDecisionGroups shows number of decisionGroups having manifestWorks available regards total number of decisionGroups | ||
// ex; 2/4 (2 out of 4) | ||
AvailableDecisionGroups string `json:"availableDecisionGroups"` |
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.
Can have a discussion here, whether need a common way to show the rollout progress. Today, addon shows this message in condition Progressing.
status:
installProgression:
- name: aws-placement
namespace: default
configReferences
...
conditions:
- lastTransitionTime: "2022-11-02T05:48:12Z"
message: 200/400 upgrading...
reason: Upgrading
status: "True"
type: Progressing
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.
Good to have common condition but I guess it hard to happen. Like in ManifestWorkReplicaSet there is no progressing condition neither upgrading reason. As well as the Manifest has only apply/processing/available state.
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.
I think we might need to introduce progressing/upgrading in ManifestWorkReplicaSet for rollout strategy. We should be able to check whether a manifestwork is applied or not already (spec is changed but not applied yet). And in the long term, we should be able to define progress/degraded condition for manifestwork.
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.
The ManifestWorkReplicaSetSummary showing the applied/progrssing/available/degraded state for all clusters manifestworks. The PlacementSummary is more to shows the placement rollout state. Not sure, Do we need to add manifestWorkSummary for each placementSummary ?
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.
done, added the ManifestWorkReplicaSetSummary to the placementSummary so every placement will has its own summary info AND added a condition for RollOut.
/unhold |
1f00f8c
to
e5c00af
Compare
AvailableClusters string `json:"availableClusters"` | ||
|
||
// availableDecisionGroups shows number of decisionGroups having manifestWorks available regards total number of decisionGroups | ||
// ex; 2/4 (2 out of 4) |
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.
is x/x a common format? how to validate it and do you expect some other controller to recognize it?
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.
it is more like a descriptive message and yes, we can validate it.
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.
can we have a more clear description of AvailableDecisionGroups? if not all manifestwork in this decision is available, is this an AvailableDecisionGroups?
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.
No, it count the groups that have all clusters manifestwork in available state. I updated the comment
|
||
// availableDecisionGroups shows number of decisionGroups having manifestWorks available regards total number of decisionGroups | ||
// ex; 2/4 (2 out of 4) | ||
AvailableDecisionGroups string `json:"availableDecisionGroups"` |
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.
I think we might need to introduce progressing/upgrading in ManifestWorkReplicaSet for rollout strategy. We should be able to check whether a manifestwork is applied or not already (spec is changed but not applied yet). And in the long term, we should be able to define progress/degraded condition for manifestwork.
e5c00af
to
508104b
Compare
75801e8
to
a2e8c33
Compare
Signed-off-by: melserngawy <melserng@redhat.com>
a2e8c33
to
5cb314b
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, serngawy 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 |
b099eba
into
open-cluster-management-io:main
No description provided.