[99][sig-policy] Policy placement strategy

[dhaiducek]

ref: https://issues.redhat.com/browse/ACM-6523

[dhaiducek]

/cc @yiraeChristineKim @JustinKuli @gparvin

[mprahl]

Could you please elaborate on this more?

I'm thinking about the case where the root policy is updated and the ConfigurationPolicy gets evaluated but the status doesn't change, how would this work? Also, if the root policy is updated but the generation of the ConfigurationPolicy doesn't change and its evaluation interval is like 2h or never when compliant, this would not trigger an evaluation in time.

Other controllers also don't post when they last evaluated.

[dhaiducek]

We do provide the LastEvaluated in the ConfigurationPolicy, so I was thinking we can add logic to have the status-sync controller update the evaluation time when the root-policy-timestamp is greater than the last evaluated time, but I'd have to dig into when the LastEvaluated gets sent if it's not on every reconcile...

        // An ISO-8601 timestamp of the last time the policy was evaluated
        LastEvaluated string `json:"lastEvaluated,omitempty"`

https://github.com/open-cluster-management-io/config-policy-controller/blob/7e648171a8bd1ac051fe130f5539fbc36347907b/api/v1/configurationpolicy_types.go#L197

Good point about other controllers, though. I'm not sure--I'll have to consider it more and look at our code. Maybe wipe the status on an update to let it repopulate or something?

[dhaiducek]

Looks like you have the same thought! I'll poke at it a bit to see whether this is feasible.

on a rollout, we could consider resetting the policy status from the Policy Propagator or setting it to a new status indicating a rollout is in progress. The status sync could recognize this and reset the compliance status on the policy templates. There is already logic in the controllers to always send a compliance event when there is no status and it would cause configuration policies to be evaluated immediately regardless of evaluation interval. Then when the policies get a new status, we know the deployment has occurred.

You'd have to think about the specific workflow but I think that may be a good option to communicate between the hub and managed clusters during a rollout.

#99 (comment)

[mprahl]

@dhaiducek after brainstorming with @gparvin since he has a good sense of the requirements from a core user, I'd like to propose the following:

• New policies are progressively propagated to clusters regardless of whether they are inform or enforce. The status of the root policy should indicate that cluster in later groups are pending rollout or some kind of other state.
• Existing policies that are updated should only have their remediationAction: enforce progressively applied. Take for instance a scenario where we have clusters A, B, and C and A is a canary cluster. An update to my policy that sets remediationAction: enforce would at first have remedation: enforce on cluster A and remediationAction: inform on clusters B and C. The root policy status needs to indicate that clusters B and C are pending enforcement. For example, their compliance state could be NonCompliant - Pending Rollout.
• PolicyAutomation (separate story) would have a new mode of everyRollout so can be triggered when a rollout fails as opposed to each cluster becoming noncompliant.

@dhaiducek and @JustinKuli , what do you think?

[dhaiducek]

I like the idea to switch to inform to prevent unintended updates. We could add the RolloutStatus to each cluster's status, but I'm not sure we should change the compliance state to NonCompliant during a rollout, especially if users have alerts set up or are active users of the dashboards--that could get unintentionally noisy? Maybe we just use the Pending state? But overall I think this is the right direction to having a controlled rollout.

[mprahl]

It's a fair point but at the same time, as a user, I would like to see the state of the policy on the cluster if there are inform policies applied even if they are pending rollout.

We could use the Pending state but include the noncompliant message so at least the information is there.

What do you think @dhaiducek and @gparvin?

[gparvin]

I prefer the compliance being NonCompliant when that reflects reality and "maybe" have a separate Remediation Pending status (new field). Though there may be too many things we could do in this extra remediation status field:

• pending rollout
• remediation applied by policy controller
• automated remediation applied
• remediated by automation or user intervention
• no remediation needed

[dhaiducek]

@gparvin I'm unclear on when you say "when that reflects reality". Are you saying policies should continue to report based on the content it has at the time, or are you saying that if the policy is out-of-date, it should be considered NonCompliant? For the latter, I suppose I'm thinking about a case where it might have been a minor update to the policy where the ultimate compliance wouldn't have changed due to the update? But maybe there's a "Policy" best practice I'm missing there if that's the meaning.

And, if we set it to "NonCompliant" because a rollout is happening, how would setting it to "NonCompliant" affect Ansible automation? As @gparvin points out, we might need some way to distinguish "NonCompliant--this is out of date" vs "NonCompliant--this is broken"? In that case, should Ansible automation only act when the RolloutStatus is Succeed? (I'm crossing my fingers that gets updated to Succeeded 😆)

Interestingly, to the "too many things we could do" point, having an enum for status is apparently against the Kubernetes status conventions (though it seems RolloutStatus is already set as such), where "Status" should simply be "True", "False", or "Unknown": https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties

[mprahl]

@dhaiducek, I think the point is that if we follow the idea that a progressive rollout to a policy update makes the enforce part progressive and nothing else, then the pending rollout clusters should still accurately report back their inform status.

Having an additional field in the root policy status, propagated policy status, or both indicating that a rollout is pending would be sufficient in my opinion.

For the PolicyAutomation front, I don't want us to get caught up on this since it's a very underutilized feature. For now, we could have an additional optional spec field on the PolicyAutomation such as ignorePendingRollout.

[dhaiducek]

Okay, that all sounds good to me, too. I've updated the enhancement with a point about setting the remediationAction to inform. I still need to check whether removing the status might work as a better alternative to the lastEvaluated status.

[mprahl]

Similar to my last comment, how is this field set?

[dhaiducek]

Addressing thread in #99 (comment)

[mprahl]

I prefer this level.

[gparvin]

This means it needs to be in PolicySet as well.

[dhaiducek]

@mprahl I just realized that "this" was unclear. Are you saying you prefer it being in the Policy?

[gparvin]

I added my comments. Thanks for the work on this!

[gparvin]

Is this a specific feature request? To me it sounds like this is worded wrong -- maybe. Is the idea the policies must be compliant within the timeout to continue if Compliant "after" the timeout? I was expecting the word "before" instead of "after"

[dhaiducek]

In the decision strategy, there's a timeout to wait for "Success" (which in the case of Policy is "Compliant"). The rollout will continue when clusters in the current rollout have succeeded OR have timed out.

[gparvin]

I'm trying to understand "TimeOut":
Is it possible the "Current generation" and lastEvaluated gets updated but is somehow less than the root policy timestamp? I'm not sure I've figured that out yet.

[dhaiducek]

The lastEvaluated might be reworked to use the transition time instead (See #99 (comment)). The root-policy-timestamp would be when the replicated policy spec was created/updated with the current generation, so any time before that would have been a previous version of the policy.

[gparvin]

So this is really a "replication time" -- not a timestamp related to something that happened on the root policy?

[dhaiducek]

Correct--and that might be a better name for it 🙂

[JustinKuli]

+1 on something like open-cluster-management.io/replication-timestamp

[dhaiducek]

How about last-replicated?

[gparvin]

This item and maybe next item -- api removal would require a new version and deprecation of old version (I didn't look at the CRD to see how we handle this today since I know we do different things for root vs replicated policy) -- edit -- I see this is mentioned later so maybe this comment isn't needed...

[dhaiducek]

Right--I might make a separate issue to look at the CRD cleanup outside of this enhancement. But I'm also not sure it's used anywhere...

[JustinKuli]

Re: API updates, I think we could stop populating the field, without removing it from the CRD (and therefore without requiring a version bump). Especially since it's just in the status, it doesn't feel like it's as much of a breaking change.

[dhaiducek]

@JustinKuli We'll have to investigate it (and I'll probably create a new issue to not muddle this enhancement), but I don't think we populate it currently 😅

[gparvin]

I don't see PolicySet mentioned, this needs to work the same way for PolicySet as it does with Policy.

[gparvin]

What about PolicyAutomation? I don't think there's really any impact other than awareness automation can be triggered against a policy that is in the mixed state.

[gparvin]

Odd behavior if you change object templates and drill down to the old templates on a cluster without the new generation. Going back to the root policy you would not be able to edit/see that old object template. Expected behavior I think...

[dhaiducek]

Right, I'm not sure placement impacts PolicyAutomation, but definitely something to verify.

Odd indeed, but the old templates ought to be reflected in the replicated policy, and it'd hopefully be relatively straightforward to find the rollout status in the root policy to figure out that it hadn't been updated yet. That said, maybe storing a per-cluster rollout status in the root policy would be good for visibility.

[gparvin]

Can this be used with selective policy enforcement?
Also, will there be CRD changes to additionalPrinterColumns to indicate a rollout scenario is underway or complete for the binding which can be for multiple policies and/or policysets?

[dhaiducek]

Good questions. I need to figure out how selective policy enforcement works, but I think they could work together, with the propagator filtering out clusters from the clusters returned from the Placement library.

The additionalPrinterColumns would be a good addition to the CRD!

[mprahl]

@dhaiducek I worry about the rollout strategy being defined on the PlacementBinding instead of the Policy because you can have multiple PlacementBindings per Policy. If multiple set rolloutStrategy, which one wins? Could a subset of clusters rollout Progressive and others rollout ProgressivePerGroup?

Also, the PlacementBinding can be used for multiple policies and it'd be annoying to have to use a separate PlacementBinding just because your policies have different rollout strategies.

Lastly, I think the UX around PlacementBinding is challenging. I find it much more intuitive to configure how my policy is deployed by setting it on the policy.

[mprahl]

In regards to selective policy enforcement, I personally think it should be ignored or an error returned when a rollout strategy is also defined.

[imiller0]

The multiple placement bindings question also seems to apply to the order/timing of when the enforce is done. If two bindings for the same policy have overlapping sets of clusters included both will (as I understand it) calculate decisiongroups independently. Any given cluster may be in group 1 of binding A and group 100 of binding B. Will it get enforced twice? If the max concurrency is 10, do you end up with 11 clusters being enforced due to the different groupings? etc.

[gparvin]

This means it needs to be in PolicySet as well.

[JustinKuli]

Just thinking out loud, but I don't know if the generation number is enough here. Maybe I'm being paranoid, but if I delete the root policy and recreate it quickly enough that the propagator hasn't removed the old ones yet, I may have changed the policy while keeping the generation the same (or maybe the generation here is actually a larger than the one on the root policy). With the long-term compliance history stuff, we're already considering making a hash of the policy to identify it, I think? Maybe that could also be used here...

[dhaiducek]

I thought about the hash and especially if the compliance history would make use of it, that'd definitely be the way to go. It becomes complex, however, if hub templates are involved since the root policy and replicated policy would look different, which is why I went with the generation. If the root policy generation is less than the annotation on the replicated one, that could also be a signal to delete the replicated policy and recreate it.

[JustinKuli]

+1 on something like open-cluster-management.io/replication-timestamp

[JustinKuli]

Re: API updates, I think we could stop populating the field, without removing it from the CRD (and therefore without requiring a version bump). Especially since it's just in the status, it doesn't feel like it's as much of a breaking change.

[JustinKuli]

I think it does currently consider multiple clusters, just in a fake way (which is probably what we want). I'm not saying that what it does is necessarily great, it probably needs to be looked at in detail for testing this feature, but it is already there to an extent: https://github.com/stolostron/governance-policy-propagator/blob/main/test/e2e/case1_propagation_test.go#L78

[gparvin]

Does an inform policy that rolls out with some clusters being NonCompliant ever transition out of Progressing to Succeeded? I think a clarification is needed on how an inform policy rollout can be successful and still identify NonCompliant clusters.

[dhaiducek]

It's an interesting question. I was thinking inform would follow the enforce behaviors, transitioning to Succeeded or Failed based on the compliance.

But for inform, I suppose we could transition the rollout to Succeeded once a status is returned regardless of compliance? It'd differ in behavior from enforce, but maybe that's okay?

[gparvin]

Yes, for inform I think as customers learn where problems "may" be they will improve inform policies to detect those issues. Then the rollout will help identify those problem clusters and I don't think the rollout should fail or be limited because inform policies are working as expected. Example: Initially we want our addon to just be available. In v2 we learn we want to look out for terminating config policies....

[mprahl]

A user shouldn't define a progressive rollout for an inform policy since it will cause the policy to be enforced unless the ConfigurationPolicy has remediationAction: informOnly which in that event, the rollout would have no effect.

I think the rollout status should be set to Succeeded if any status is provided as Dale mentioned. Syntax errors in the ConfigurationPolicy would lead to a failed rollout status.

[mprahl]

The nice part of this is that even users that don't use progressive policy rollouts will get a single field to know if all policy templates have reported an up to date status for the cluster.

[gparvin]

Can I not roll out a policyset that contains inform and enforce policies and the inform policies would not be enforced? I should be able to put any random policies in a policyset with some enforce and some inform and it should be honored appropriately during rollout. Enforcing some policies could have unintended consequences -- rebooting nodes, upgrading, etc

[serngawy]

I think it needs more clarification on how the RolloutStatus will move from ToApply->progressing->Succeeded/TimeOut.
Here is some thoughts;

• ToApply;
  1- when the child policy is not get created yet for the managedCluster
  2- when the child policy is created for the managedCluster AND the policy remediation is Inform and the policy in NonCompliant state.

• Progressing;
  1- When the child policy is created for the managedCluster AND the policy remediation is Enforce and the policy in NonCompliant state.

• Succeeded;
  1- When the child policy is created for the managedCluster and it is in Compliant state for either enforce/inform remediation

• TimeOut;
  1- When the child policy is created for the managedCluster AND the policy remediation is Enforce and the policy in NonCompliant state for longer time than it is defined in the rolloutStrategy->TimeOut

[mprahl]

I think ToApply means the the policy hasn't been enforced yet on the cluster.

Progressing would mean the policy has been enforced on the cluster but the rollout hasn't completed yet.

[imiller0]

Is the set of "valid" status fields different on the root policy vs the replicated policies? It sounds like ToApply would be valid on a replicated policy but not the root. Similarly it sounds like progressing would be valid on the root but not the replicated set. If so it might help to define the set of valid values for root vs replicated.

[dhaiducek]

Good point, @mprahl--I wrapped ToApply in for simplicity but it would be clearer to use ToApply.

@imiller0 The RolloutStatus is an iota we're taking from the Placement library: https://github.com/open-cluster-management-io/api/blob/9675ab737d2086dd7094fc962c9617cc71ec7484/cluster/v1alpha1/helpers.go#L21-L38 but having these well defined for each the root and replicated policies would be good.

[mprahl]

Yes to this.

[mprahl]

The Policy CRD in my opinion based on the comment here:
https://github.com/open-cluster-management-io/enhancements/pull/99/files#r1309173623

[mprahl]

Suggested change

	5. How is "soak time" (the minimum time for a policy to reach a successful state) and timeout (the maximum amount of
	time to wait for a successful state), as defined in the placement enhancement, handled for Policies?
	5. How is "soak time" (the minimum time for a policy to reach a successful state) and timeout (the maximum amount of
	time to wait for a state), as defined in the placement enhancement, handled for Policies?

[imiller0]

Is this solely a priority ordering or does "mandatory" mean "fail if timed out"?

[dhaiducek]

This would solely be priority ordering, but that's an interesting point to have a different timeout setting for the mandatory groups and the subsequent groups--that could be an update for the Placement library in the future.

MandatoryDecisionGroups is described in the DecisionStrategy enhancement here: https://github.com/open-cluster-management-io/enhancements/tree/main/enhancements/sig-architecture/64-placementStrategy#how-workload-applier-apis-will-benefits-from-using-placementstrategy

[imiller0]

This may be out of scope here, but for rollback to work correctly in the general case I believe that the policy would need to be "mustonlyhave" compliance type.

[dhaiducek]

@imiller0 That's true for objects on the managed cluster--this is referring specifically to the previous version of the policy itself on the hub rather than rolling back changes on the managed cluster.

[imiller0]

Is the set of "valid" status fields different on the root policy vs the replicated policies? It sounds like ToApply would be valid on a replicated policy but not the root. Similarly it sounds like progressing would be valid on the root but not the replicated set. If so it might help to define the set of valid values for root vs replicated.

[imiller0]

Once set to enforce do they remain in enforce mode until a subsequent change to the root policy or do they revert to inform once compliant?

[dhaiducek]

They remain in enforce until the root policy is updated again.

[imiller0]

The multiple placement bindings question also seems to apply to the order/timing of when the enforce is done. If two bindings for the same policy have overlapping sets of clusters included both will (as I understand it) calculate decisiongroups independently. Any given cluster may be in group 1 of binding A and group 100 of binding B. Will it get enforced twice? If the max concurrency is 10, do you end up with 11 clusters being enforced due to the different groupings? etc.

[imiller0]

Does this risk clusters being skipped? Or gaps in the decision groups if clusters are unbound/rebound?

[imiller0]

How does the rolloutStrategy work with policy dependency? Is it possible that if the two policies are bound using separate policybindings and the policies are modified simultaneously the group ordering could be such that rollout of one policy is entirely blocked until completion of all groups of the other policy?

[imiller0]

If a policy is modified/updated by the user while a rollout is ongoing, does the process start over with all policies being set back to inform and rollout beginning with group 1 again?

[dhaiducek]

That's correct

[serngawy]

I think it needs more clarification on how the RolloutStatus will move from ToApply->progressing->Succeeded/TimeOut.
Here is some thoughts;

• ToApply;
  1- when the child policy is not get created yet for the managedCluster
  2- when the child policy is created for the managedCluster AND the policy remediation is Inform and the policy in NonCompliant state.

• Progressing;
  1- When the child policy is created for the managedCluster AND the policy remediation is Enforce and the policy in NonCompliant state.

• Succeeded;
  1- When the child policy is created for the managedCluster and it is in Compliant state for either enforce/inform remediation

• TimeOut;
  1- When the child policy is created for the managedCluster AND the policy remediation is Enforce and the policy in NonCompliant state for longer time than it is defined in the rolloutStrategy->TimeOut

[serngawy]

I don't think there is a Failed status cause the policy-controller will continue to enforce the policy to the managedCluster as long as it is in NonCompliant state. So status could be ToApply, Progressing, Succeeded, TimeOut.

[dhaiducek]

@serngawy True, but if the enforcement is blocked for some reason (like a missing API on the managed cluster or an invalid manifest), it may be good to reflect that in the root policy since there's no way for the rollout to progress?

[serngawy]

I guess better to include the progressivePerGroup fields as well.

remediationAction: inform # <-- Remediation action updated to prepare for rollout
rolloutStrategy:
  type: ProgressivePerGroup
  progressivePerGroup:
    mandatoryDecisionGroups:
    - groupName: dev-emea   (Same group name as defined in the placement.)
    timeout: None```

[mprahl]

This looks great. We'll work on adding the new requirements discussed today in a separate PR.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, mprahl

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:

• enhancements/sig-policy/OWNERS [dhaiducek,mprahl]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment