From 23521155ce41d996c26b77ef4d73d471b81e5c3e Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Wed, 20 Nov 2024 18:10:42 +0000 Subject: [PATCH 1/4] added initial files --- keps/238-controller-revision/README.md | 394 +++++++++++++++++++++++++ keps/238-controller-revision/kep.yaml | 19 ++ 2 files changed, 413 insertions(+) create mode 100644 keps/238-controller-revision/README.md create mode 100644 keps/238-controller-revision/kep.yaml diff --git a/keps/238-controller-revision/README.md b/keps/238-controller-revision/README.md new file mode 100644 index 00000000..cdc6ac0f --- /dev/null +++ b/keps/238-controller-revision/README.md @@ -0,0 +1,394 @@ +# KEP-NNNN: Your short, descriptive title + + + + + + +- [Summary](#summary) +- [Motivation](#motivation) + - [Goals](#goals) + - [Non-Goals](#non-goals) +- [Proposal](#proposal) + - [User Stories (Optional)](#user-stories-optional) + - [Story 1](#story-1) + - [Story 2](#story-2) + - [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional) + - [Risks and Mitigations](#risks-and-mitigations) +- [Design Details](#design-details) + - [LWS controller](#lws-controller) + - [Pod Controller](#pod-controller) + - [Controller Revision Implementation](#controller-revision-implementation) + - [Test Plan](#test-plan) + - [Prerequisite testing updates](#prerequisite-testing-updates) + - [Unit tests](#unit-tests) + - [Integration tests](#integration-tests) + - [e2e tests](#e2e-tests) + - [Graduation Criteria](#graduation-criteria) +- [Implementation History](#implementation-history) +- [Drawbacks](#drawbacks) +- [Alternatives](#alternatives) + + +## Summary + + + +This KEP aims to add controller revision to store previous states of the LWS object. +It fixes bug #238, and opens the road for future rollback support. + +## Motivation + + +If a replica is restarted during rolling update, and the replica hasn't been updated +yet, the worker pod spec that is used is the updated one, while the leader pod spec +is the original one. We can fix this by storing the worker pod spec using controller revision. + +### Goals + + + +Stores the state of the LWS object in order to use the correct version when recreating a replica. + +### Non-Goals + + + +This KEP does not add rollback support, though the design takes into account that it will be +added in the future. + +## Proposal +We propose adding controller revision to the LWS controller. This allows us to store previous +iterations of the LWS object, in order to make it possible to recreate pods with the right pod +spec when restarted during rolling update. This requires adding three new fields to the lws status: +`lws.status.CurrentRevision` `lws.Status.UpdateRevision` and `lws.Status.CollisionCount`. + + + +### User Stories (Optional) + + + +#### Story 1 + +#### Story 2 + +### Notes/Constraints/Caveats (Optional) + + + +### Risks and Mitigations + + + +## Design Details +The following LWS API fields need to be added to support controller revision. + +```golang +// LeaderWorkerSetStatus defines the observed state of LeaderWorkerSet +type LeaderWorkerSetStatus struct { + // currentRevision, if not empty, indicates the version of lws + // used to generate the worker pods in sequence [0,currentReplicas) + CurrentRevision string `json:"currentRevision,omitempty"` + + // updateRevision, if not empty, indicates the version of lws + // used to generate the worker pods in sequence + // [replicas-updatedReplicas,replicas) + UpdateRevision string `json:"updateRevision,omitempty"` + + // collisionCount is the count of hash collisions for the controller + // revision uses this field as a collision avoidance mechanism + // when it needs to create the name for the newest ControllerRevision. + // +optional + CollisionCount *int32 `json:"collisionCount,omitempty"` +} +``` + +### LWS controller + +The status of the revisions will be updated before the rollingUpdate is started + +```golang +func Reconcile() { + currentRevision, updateRevision, collisionCount, err := GetLeaderWorkerSetRevisions(ctx, r.Client, lws) + if err != nil { + log.Error(err, "Getting StatefulSet revisions") + return ctrl.Result{}, err + } + lws.Status.CurrentRevision = currentRevision.Name + lws.Status.UpdateRevision = updateRevision.Name + lws.Status.CollisionCount = new(int32) + lws.Status.CollisionCount = &collisionCount + + partition, replicas, err := r.rollingUpdateParameters(ctx, lws) +} +``` + +Once the update has been determined to be done, `currentRevision` will be set to be the value of `updateRevision` + +```golang +func updateConditions() { + if updatedAndReadyCount == int(*lws.Spec.Replicas) { + conditions = append(conditions, makeCondition(leaderworkerset.LeaderWorkerSetAvailable)) + lws.Status.CurrentRevision = lws.Status.UpdateRevision + } +} +``` + +### Pod Controller + +In order to determine what worker pod spec to use to create the worker pods, we can compare +the value of the template hash generated by the LWS object, with the template hash that the leader pod +hash. Because the leader pod spec is determined by the statefulset controller, there is a guarantee that +it will always have the right pod spec. So, if the template hashes don't match, it means that the leader pod was created +using the old pod spec, meaning the old worker pod spec needs to be used. + +```golang + +func Reconcile() { + currentRevision, _, _, err := GetLeaderWorkerSetRevisions(ctx, r.Client, lws) + constructWorkerStatefulSetApplyConfiguration(currentRevision) +} + +func constructWorkerStatefulSetApplyConfiguration(currentRevision) { + updatedTemplateHash := LeaderWorkerTemplateHash(&lws) + podTemplateSpec := *WorkerTemplate.DeepCopy() + if updatedTemplateHash != leaderPod.Labels[templateHash] { + originalLws, err := ApplyRevision(&lws, currentRevision) + if err != nil { + return nil, err + } + podTemplateSpec = *originalLws.WorkerTemplate.DeepCopy() + } +} +``` + +### Controller Revision Implementation +A new package will be created for the functions that interact with controllerRevision, similar to how it is done in [upstream](https://github.com/kubernetes/kubernetes/blob/cb31e42b85d0a1e2be6639ecd4f7b9414887552a/pkg/controller/history/controller_history.go). + +Functions for creating patches, applying revisions, and truncating history will be added to `controller_utils` since both the pod and lws controllers need to access these functions. + +For now, the history will only contain the current and updatedRevisions. Once rollback is implemented, this can be updated. + +Only the `LeaderWorkerTemplate` will be included in the controller revision patch. This can be modified later while still being backwards compatible. + +```golang +func createPatch() { + template := spec["leaderWorkerTemplate"].(map[string]interface{}) + specCopy["leaderWorkerTemplate"] = template + template["$patch"] = "replace" + objCopy["spec"] = specCopy + patch, err := json.Marshal(objCopy) +} +``` + + + + + +### Test Plan + + + +[X] I/we understand the owners of the involved components may require updates to +existing tests to make this code solid enough prior to committing the changes necessary +to implement this enhancement. + +##### Prerequisite testing updates + + + +##### Unit tests +- Test controller revision implemenation functions + + + + +##### Integration tests + +Test that currentRevision, updatedRevision, and collisionCount work as intended. + +Test that worker pod is recreated with the correct pod template if restarted during rolling update. + + + + + + +##### e2e tests + + + + +### Graduation Criteria + + + +## Implementation History + + + +## Drawbacks + + + +## Alternatives +An implementation similar to the one done for StatefulSet was considered. In this implementation, we would create +a label to store what controller revision was used to generate the pods, and replacing the label `leaderworkerset.sigs.k8s.io/template-revision-hash`. +However, this requires updating the value of those labels (or adding a new label alltogether). Doing this, triggers a rolling update when upgrading +from an LWS version that does not have controller revision. + +Moreover, there are features that need to trigger a rolling update (e.g NetworkConfig). In order to replicate this behavior when using +a label to store controller revision, NetworkConfig must be part of the controller revision patch. However, this would mean that NetworkConfig would +also be rolled back. + + \ No newline at end of file diff --git a/keps/238-controller-revision/kep.yaml b/keps/238-controller-revision/kep.yaml new file mode 100644 index 00000000..7ec330b2 --- /dev/null +++ b/keps/238-controller-revision/kep.yaml @@ -0,0 +1,19 @@ +title: Controller Revision +kep-number: 238 +authors: + - edwinhr716 +status: implementable +creation-date: 2024-11-20 +reviewers: + - kerthcet + - liurupeng + - ahg-g +approvers: + - kerthcet + - liurupeng + - ahg-g + +# The most recent milestone for which work toward delivery of this KEP has been +# done. This can be the current (upcoming) milestone, if it is being actively +# worked on. +latest-milestone: "v0.5.0" From 660e9608129e356760d1a46546c0d96a69936d14 Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Wed, 20 Nov 2024 18:18:07 +0000 Subject: [PATCH 2/4] cleanup --- keps/238-controller-revision/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/keps/238-controller-revision/README.md b/keps/238-controller-revision/README.md index cdc6ac0f..568290fb 100644 --- a/keps/238-controller-revision/README.md +++ b/keps/238-controller-revision/README.md @@ -1,4 +1,4 @@ -# KEP-NNNN: Your short, descriptive title +# KEP-238: Controller-Revision -Stores the state of the LWS object in order to use the correct version when recreating a replica. + - Stores the state of the LWS object in order to use the correct version when recreating a replica. + - Change the mechanism with which we determine whether an LWS object has changed. + ### Non-Goals @@ -102,8 +110,7 @@ added in the future. ## Proposal We propose adding controller revision to the LWS controller. This allows us to store previous iterations of the LWS object, in order to make it possible to recreate pods with the right pod -spec when restarted during rolling update. This requires adding three new fields to the lws status: -`lws.status.CurrentRevision` `lws.Status.UpdateRevision` and `lws.Status.CollisionCount`. +spec when restarted during rolling update, and be able to compare different LWS objects to determine if the spec has changed. ## Design Details -The following LWS API fields need to be added to support controller revision. +There are a few constraints that need to be taken into account in the design: +- The label `templateHash` is not a reliable way to determine whether or not an LWS object has been updated, as seen in #281 +- The same LWS object can generate two different controllerRevision names + - For instance, everytime the LWS is queried, it will have different `lws.ResourceVersion`, causing two revisions to be named differently even if they have the same LWS object +- Adding a new default pod label will trigger rolling update when updating from a different LWS controller version, so no new pod labels can be added. + +Taking those into account, we'll combine controller revisions and template hashes. Each controller revision will have a templateHash as a label, essentially creating a map of revisions with template hashes as keys. + + +### LWS controller +In order to fix #281, we will use the hash key in the leaderSts to make a controller revision lookup, and get the LWS object that was used to create it. To determine if an update has happened, we'll compare the fields that are used to generate the template hash. + ```golang -// LeaderWorkerSetStatus defines the observed state of LeaderWorkerSet -type LeaderWorkerSetStatus struct { - // currentRevision, if not empty, indicates the version of lws - // used to generate the worker pods in sequence [0,currentReplicas) - CurrentRevision string `json:"currentRevision,omitempty"` - - // updateRevision, if not empty, indicates the version of lws - // used to generate the worker pods in sequence - // [replicas-updatedReplicas,replicas) - UpdateRevision string `json:"updateRevision,omitempty"` - - // collisionCount is the count of hash collisions for the controller - // revision uses this field as a collision avoidance mechanism - // when it needs to create the name for the newest ControllerRevision. - // +optional - CollisionCount *int32 `json:"collisionCount,omitempty"` +func templateUpdated(sts, lws) bool { + controllerRevision := GetLeaderWorkerSetRevisionFromTemplateHash(sts.Labels[templateHash]) + baselineLws:= controllerutils.ApplyRevision(lws, controllerRevision) + return !utils.EqualLeaderWorkerTemplates(baselineLws, lws) } ``` -### LWS controller +There are three cases where we need to create a new controllerRevision: +- There are no existing controller revisions in the history +- A new LWS object is created +- An update has been made to the LWS object -The status of the revisions will be updated before rolling update starts. +#### Case 1: No exsting controller revisions in history +This is the case when upgrading from a version that did not have controller revisions. Since a controller revision is needed to determine whether or not an update has happened, it will be created in `rollingUpdateParameters`, so that way an update isn't triggered until a controller revision has been created. ```golang -func Reconcile() { - currentRevision, updateRevision, collisionCount, err := GetLeaderWorkerSetRevisions(ctx, r.Client, lws) - if err != nil { - log.Error(err, "Getting StatefulSet revisions") - return ctrl.Result{}, err +func rollingUpdateParameters() { + if !existingControllerRevisions { + createLeaderWorkerSetRevision(lws, leaderSts.labels[templateHash]) + return + } + + if templateUpdated(leaderSts, lws) { + // triggers the rolling update } - lws.Status.CurrentRevision = currentRevision.Name - lws.Status.UpdateRevision = updateRevision.Name - lws.Status.CollisionCount = new(int32) - lws.Status.CollisionCount = &collisionCount +} +``` + +#### Case 2 & 3: Regular create and update +Because an update can be triggered by changing the value of a label, what templateHash value is used when building the leader sts also has to be safeguarded. - partition, replicas, err := r.rollingUpdateParameters(ctx, lws) +```golang +func SSAwithStatefulSet(lws, partition, lws) { + templateHash := utils.LeaderWorkerTemplateHash(lws) + if leaderStsExists && !templateUpdated(leaderSts, lws){ + templatehash := leaderSts.Labels[templateHash] + } + createLeaderWorkerSetRevision(lws, templateHash) + constructLeaderStatefulSetApplyConfiguration(lws, partition, replicas, templateHash) } ``` -Once the update has been determined to be done, `currentRevision` will be set to be the value of `updateRevision` +Once the update has been determined to be done, we'll reset the history to only have the revision of the current LWS object in the history. ```golang func updateConditions() { if updatedAndReadyCount == int(*lws.Spec.Replicas) { conditions = append(conditions, makeCondition(leaderworkerset.LeaderWorkerSetAvailable)) - lws.Status.CurrentRevision = lws.Status.UpdateRevision + truncateHistory() } } ``` ### Pod Controller +Now that there is a map between the template hash of the leader and controller revision, a lookup can be done to select the worker pod spec that will be used. -In order to determine what worker pod spec to use to create the worker pods, we can compare -the value of the template hash generated by the LWS object, with the template hash that the leader pod -hash has. Because the leader pod spec is determined by the statefulset controller, there is a guarantee that -it will always have the right pod spec. So, if the template hashes don't match, it means that the leader pod was created -using the old pod spec, meaning the old worker pod spec needs to be used. ```golang - func Reconcile() { - currentRevision, _, _, err := GetLeaderWorkerSetRevisions(ctx, r.Client, lws) - constructWorkerStatefulSetApplyConfiguration(currentRevision) + controllerRevision := GetLeaderWorkerSetRevisionFromTemplateHash(pod.Labels[templateHash]) + constructWorkerStatefulSetApplyConfiguration(controllerRevision) } func constructWorkerStatefulSetApplyConfiguration(currentRevision) { - updatedTemplateHash := LeaderWorkerTemplateHash(&lws) - podTemplateSpec := *WorkerTemplate.DeepCopy() - if updatedTemplateHash != leaderPod.Labels[templateHash] { - originalLws, err := ApplyRevision(&lws, currentRevision) - if err != nil { - return nil, err - } - podTemplateSpec = *originalLws.WorkerTemplate.DeepCopy() - } + currentLws := controllerutils.ApplyRevision(lws, controllerRevision) + podTemplateSpec := **currentLws.WorkerTemplate.DeepCopy() } ``` @@ -237,19 +246,9 @@ A new package will be created for the functions that interact with controllerRev Functions for creating patches, applying revisions, and truncating history will be added to `controller_utils` since both the pod and lws controllers need to access these functions. -For now, the history will only contain the current and updatedRevisions. Once rollback is implemented, this can be updated. - -Only the `LeaderWorkerTemplate` will be included in the controller revision patch. This can be modified later while still being backwards compatible. +We'll store the whole LWS spec to make it easier to compare LWS objects when determining if it has been updated. -```golang -func createPatch() { - template := spec["leaderWorkerTemplate"].(map[string]interface{}) - specCopy["leaderWorkerTemplate"] = template - template["$patch"] = "replace" - objCopy["spec"] = specCopy - patch, err := json.Marshal(objCopy) -} -``` +In order to ensure that there only exists one revision per templateHash, two controller revisions will be determined to be equal if they have the same template hash label. diff --git a/keps/238-controller-revision/kep.yaml b/keps/238-controller-revision/kep.yaml index 7ec330b2..28475c33 100644 --- a/keps/238-controller-revision/kep.yaml +++ b/keps/238-controller-revision/kep.yaml @@ -6,11 +6,9 @@ status: implementable creation-date: 2024-11-20 reviewers: - kerthcet - - liurupeng - ahg-g approvers: - kerthcet - - liurupeng - ahg-g # The most recent milestone for which work toward delivery of this KEP has been