-
Notifications
You must be signed in to change notification settings - Fork 37
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
[99][sig-policy] PolicySet rollout #103
[99][sig-policy] PolicySet rollout #103
Conversation
/cc @gparvin |
ref: https://issues.redhat.com/browse/ACM-6523 Signed-off-by: Dale Haiducek <[email protected]>
7a92de3
to
900b91a
Compare
- **Details** | ||
|
||
- Add an `OrderPolicies` boolean and the `RolloutStrategy` struct to the spec of the `PolicySet`. | ||
- `OrderPolicies` signals to the `governance-policy-propagator` controller whether to inject a dependency on the |
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.
Could an injected dependency between policies conflict with an actual dependency?
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 existing dependency would be added to the list of dependencies on the replicated policy (rather than a conflict per se), so both would block the policy until they're fulfilled. The dependencies are one point of complexity here, but for this it'd become a union of the dependencies.
specified in this enhancement, with the policy set overriding any `rolloutStrategy` specified in the individual | ||
policy. | ||
- When the policy set rollout is signaled by the `policy.open-cluster-management.io/last-rollout` annotation, the | ||
propagator identifies whether a policy was bound to a placement via a policy set. From there: |
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.
And the policy should only have the one placement defined. That is the placement on the policyset in this case. So if the policy is in another policyset too --- that 2nd policyset must use the same placement. What if that policyset doesn't specify the rollout strategy?
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.
If the second policy set doesn't specify a rollout, I think the rollout would take precedence if it specifies the same cluster. But I'm not sure why you'd have the same policy in two separate sets with the same placement?
- Add `rolloutStrategy` and `rolloutStatus` to the `PolicySet`. For a policy set, the rollout is successful for a | ||
cluster only after all policies in the set become compliant. | ||
- The `rolloutStatus` would be an aggregated status across all clusters and policies. The aggregated root policy | ||
status would be: `Progressing` (this would include `ToApply`), `Succeeded`, or `Failed`. |
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.
If max failure is 3, and failed policies are 3, then Does it mark as Succeeded?
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.
Hmm. Interesting question. I could see it either way but I'm thinking it'd still be marked as Failed since the rollout failed on some clusters. For something like ProgressivePerGroup, setting MaxFailures would give a tolerance to allow the rollout to move on to the next group in the case of flaky cluster(s), so the aggregated status would stay in Progressing
until then end, when it would wind up Failed
.
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.
oh, MaxFailures is used to decide to go further or not. I got it Thanks!!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, yiraeChristineKim 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 |
/hold |
a196ed0
into
open-cluster-management-io:main
Followup to:
ref: https://issues.redhat.com/browse/ACM-6523