fix: store revisions to ensure grove upgrades do not cause new rollouts#710
fix: store revisions to ensure grove upgrades do not cause new rollouts#710steved wants to merge 2 commits into
Conversation
| uses the normal update strategy. An active or otherwise unobserved legacy | ||
| revision is left unchanged and reports a reconciliation error until the | ||
| migration precondition is resolved. Downgrading afterward to an operator that | ||
| does not understand ControllerRevision-backed selection is unsupported. |
There was a problem hiding this comment.
Preventing the option to rollback is a big deal IMHO. Can you think of ways to support it, even if it comes with some price, including even pods being restarted? At least so we can discuss the tradeoffs. Being able to rollback is important, especially given the complexity of changes like this one and other major ones that are in the pipeline.
There was a problem hiding this comment.
Let me take a look. It should work theoretically as long as the actual template hash doesn't change, but I didn't test it.
| | `podGangStatuses` _[PodGangStatus](#podgangstatus) array_ | PodGangStatuses captures the status for all the PodGang's that are part of the PodCliqueSet. | | | | ||
| | `currentGenerationHash` _string_ | CurrentGenerationHash is a hash value generated out of a collection of fields in a PodCliqueSet.<br />Since only a subset of fields is taken into account when generating the hash, not every change in the PodCliqueSetSpec will<br />be accounted for when generating this hash value. A field in PodCliqueSetSpec is included if a change to it triggers<br />a rolling recreate of PodCliques and/or PodCliqueScalingGroups.<br />Only if this value is not nil and the newly computed hash value is different from the persisted CurrentGenerationHash value<br />then an update needs to be triggered. | | | | ||
| | `currentGenerationHash` _string_ | CurrentGenerationHash is the opaque identity selected for the current rollout-relevant revision. | | | | ||
| | `currentRevision` _string_ | CurrentRevision names the ControllerRevision containing the selected rollout revision. | | | |
There was a problem hiding this comment.
Is there any kind of cleanup for old revisions or are they all kept for the whole PCS lifetime?
There was a problem hiding this comment.
Right now all but the active one are cleaned up: https://github.com/steved/grove/blob/7cb40f286add7cd4006e736a1f03dec202e0c6c5/operator/internal/controller/podcliqueset/revision.go#L300-L321
| r.ensureFinalizer, | ||
| r.processGenerationHashChange, | ||
| r.processRevision, | ||
| r.syncPodCliqueSetResources, |
There was a problem hiding this comment.
After selecting a new ControllerRevision, this reconcile continues directly into child sync, and child sync reloads the selected revision through the controller-runtime client/cache. In a real manager this can race informer cache propagation: the Create has reached the API server, but the cache may not yet contain the new ControllerRevision. Fake-client tests will not catch this. Consider requeueing after revision selection, using APIReader for this read, or passing the selected revision through the sync path.
| type Data struct { | ||
| Version int `json:"version"` | ||
| Desired json.RawMessage `json:"desired"` | ||
| Cliques map[string]json.RawMessage `json:"cliques"` |
There was a problem hiding this comment.
The revision payload stores the rollout template data twice: Desired contains all pod templates, and Cliques stores each clique template again. For large PodCliqueSets this can make each ControllerRevision much larger than the PCS template itself, and rollout can temporarily retain more than one revision. Can we avoid the duplication, or add a guard/test/documentation around the maximum supported object size?
There was a problem hiding this comment.
Yeah - this was to preserve clique ordering and still retain the map. Let me see if there's a better way to do this.
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Implements a ControllerRevision-backed solution for tracking the desired state of pod cliques to ensure that Grove upgrades (and underlying dependency changes) do not cause pods to be updated unnecessarily.
Which issue(s) this PR fixes:
Fixes #709
Special notes for your reviewer:
Does this PR introduce a API change?
Additional documentation e.g., enhancement proposals, usage docs, etc.: