Add Controller Revision (Implementation of KEP #238) #277

Edwinhr716 · 2024-12-06T19:11:01Z

What type of PR is this?

/kind feature

What this PR does / why we need it

This PR adds controller revision. This allows the controller to store previous iterations of the LWS object, which then makes it possible to select the correct pod template spec to use if a replica is restarted during rolling update.

Which issue(s) this PR fixes

Fixes #238
Fixes #239
Fixes #240
Fixes #281
Fixes #291

Special notes for your reviewer

Does this PR introduce a user-facing change?

ahg-g

First round, I didn't look at tests.

api/leaderworkerset/v1/leaderworkerset_types.go

pkg/history/controller_history.go

pkg/utils/controller/controller_utils.go

pkg/controllers/leaderworkerset_controller.go

pkg/utils/controller/controller_utils.go

pkg/controllers/leaderworkerset_controller.go

ahg-g

more comments on simplifying the history pkg

pkg/history/controller_history.go

pkg/utils/controller/controller_utils.go

pkg/history/controller_history.go

test/testutils/util.go

test/testutils/wrappers.go

Edwinhr716 · 2024-12-23T18:59:31Z

Addressed majority of comments, still need to add more tests and rebase to be ready to merge

ahg-g · 2024-12-23T19:13:34Z

Great, i will look later today, please ensure we have sufficient integration test coverage

…ented TruncateHistory and fixed getPatch

pkg/controllers/leaderworkerset_controller.go

pkg/utils/revision/revision_utils.go

pkg/controllers/leaderworkerset_controller.go

ahg-g · 2024-12-23T22:34:56Z

pkg/controllers/leaderworkerset_controller.go

@@ -387,6 +410,9 @@ func (r *LeaderWorkerSetReconciler) updateConditions(ctx context.Context, lws *l
 		conditions = append(conditions, makeCondition(leaderworkerset.LeaderWorkerSetUpgradeInProgress))
 	} else if updatedAndReadyCount == int(*lws.Spec.Replicas) {
 		conditions = append(conditions, makeCondition(leaderworkerset.LeaderWorkerSetAvailable))
+		if err := revisionutils.TruncateHistory(ctx, r.Client, lws, templateHash); err != nil {


as a follow up, we can store the current and updated revisions for debugging purposes. The current one can be set here, while the updated one can be set in the if block above.

pkg/controllers/pod_controller.go

test/testutils/wrappers.go

pkg/utils/revision/revision_utils.go

Edwinhr716 · 2024-12-27T21:40:07Z

Finished addressing comments. I agree, makes sense to change templateHash to revisionKey. Also changed that in the latest commit.

pkg/utils/revision/revision_utils.go

pkg/controllers/leaderworkerset_controller.go

pkg/controllers/pod_controller.go

pkg/controllers/leaderworkerset_controller.go

pkg/utils/revision/revision_utils.go

Edwinhr716 · 2024-12-28T00:38:58Z

Addressed comments. Flakiness of tests is much higher now that everything uses GetRevisionKey(), unsure why. I'll retest later to see if that fixes it.

FYI I do want to add integration tests before merging which is why I had the WIP tag in the title

pkg/controllers/leaderworkerset_controller.go

pkg/controllers/pod_controller_test.go

pkg/utils/revision/revision_utils.go

test/e2e/e2e_test.go

ahg-g · 2024-12-28T01:02:18Z

test/e2e/e2e_test.go

+				return fmt.Errorf("StatefulSet did not have the expected container name")
+			}
+			return nil
+		}, timeout, interval).Should(gomega.Succeed())


This test will be flaky, how do you guarantee that the update isn't faster than your check here?

There is no guarantee, that is just a risk that is taken when checking the state mid update. Similar issue in other e2e tests. All I can do is have as many replicas as possible so that the update takes long to reach the "-0" leader sts. Hasn't failed after many runs so far though.

We can't have tests that don't offer guarantees, those will flake, which means when they fail, we don't know if they fail because of a bug or the race condition. The test you are trying to do here should be done via an integration test where we control update progress.

Right, but other update tests also don't offer guarantees. For example this one https://github.com/kubernetes-sigs/lws/blob/main/test/e2e/e2e_test.go#L181, what guarantees that the update isn't done before this check is performed? I guess I'm just confused as to why we can do it there but not here, both are checking the state mid update.

pkg/controllers/leaderworkerset_controller.go

k8s-ci-robot · 2024-12-28T02:58:55Z

@Edwinhr716: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-lws-test-e2e-main-1-28	`0ff8f71`	link	true	`/test pull-lws-test-e2e-main-1-28`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

ahg-g · 2024-12-28T04:13:11Z

test/e2e/e2e_test.go

+
+		gomega.Eventually(func() (bool, error) {
+			return initialLeaderPod.UID == midUpdateLeaderPod.UID, nil
+		}, timeout, interval).Should(gomega.Equal(false))


why is this in an eventually clause? nothing is changing inside it

ahg-g · 2024-12-28T04:28:53Z

/lgtm
/approve

Will use #294 to track integration tests

k8s-ci-robot · 2024-12-28T04:29:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, Edwinhr716

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:

~~OWNERS~~ [ahg-g]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 6, 2024

k8s-ci-robot requested a review from ahg-g December 6, 2024 19:11

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 6, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2024

ahg-g reviewed Dec 22, 2024

View reviewed changes

ahg-g reviewed Dec 23, 2024

View reviewed changes

ahg-g mentioned this pull request Dec 23, 2024

Release v0.5.0 requirements #221

Open

ahg-g reviewed Dec 23, 2024

View reviewed changes

test/testutils/util.go Outdated Show resolved Hide resolved

ahg-g reviewed Dec 23, 2024

View reviewed changes

test/testutils/wrappers.go Outdated Show resolved Hide resolved

Edwinhr716 added 12 commits December 23, 2024 19:46

initial working prototype of controller revision

db8a7c5

update value of currentRevision after update is done

df4872b

switched names from sts to lws

de9dd70

test changes

267af37

added unit, integration, and e2e tests

7389ea1

cleanup and added integration test for collisionCount

5a49f88

fix lint

f28995a

changed from string to const on labels

1f5adfa

changed revision logic based on updated design

1b978cf

removed status changes, cleaned up tests that referenced them. Implem…

522f1b0

…ented TruncateHistory and fixed getPatch

addressed comments, refactored

9436aeb

rebased

8d3da19

Edwinhr716 force-pushed the controllerRevision branch from e674b72 to 8d3da19 Compare December 23, 2024 19:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2024

Edwinhr716 added 2 commits December 23, 2024 20:21

fixed failing tests and lint error

5fa3b34

fixed lint, again

6a29f29

ahg-g reviewed Dec 23, 2024

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 27, 2024

Edwinhr716 commented Dec 27, 2024

View reviewed changes

pkg/utils/revision/revision_utils.go Outdated Show resolved Hide resolved

further changes from templateHash to RevisionKey

949bc68

ahg-g changed the title ~~[WIP] Add Controller Revision (Implementation of KEP #238)~~ Add Controller Revision (Implementation of KEP #238) Dec 27, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 27, 2024

ahg-g reviewed Dec 27, 2024

View reviewed changes

pkg/utils/revision/revision_utils.go Outdated Show resolved Hide resolved

further changing from templateHash to revisionKey

6b292d7

ahg-g reviewed Dec 28, 2024

View reviewed changes

pkg/controllers/leaderworkerset_controller.go Show resolved Hide resolved

minor fixes

a065e93

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 28, 2024

Edwinhr716 added 2 commits December 28, 2024 02:15

fixed all tests failing, still need to debug other tests

8b62baa

added log messages to listRevision for debugging

897e009

Edwinhr716 added 3 commits December 28, 2024 03:12

fixed bug with getHighestRevision

7822c18

added log messages for create revision function

cafadfc

removed fetch after creation, not needed

a856b3d

ahg-g reviewed Dec 28, 2024

View reviewed changes

removed e2e tests, will be added as integration tests intead

1faace7

k8s-ci-robot assigned ahg-g Dec 28, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 28, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 28, 2024

k8s-ci-robot merged commit c03d4c9 into kubernetes-sigs:main Dec 28, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Controller Revision (Implementation of KEP #238) #277

Add Controller Revision (Implementation of KEP #238) #277

Edwinhr716 commented Dec 6, 2024 •

edited by ahg-g

Loading

ahg-g left a comment

ahg-g left a comment

Edwinhr716 commented Dec 23, 2024

ahg-g commented Dec 23, 2024

ahg-g Dec 23, 2024

Edwinhr716 commented Dec 27, 2024

Edwinhr716 commented Dec 28, 2024 •

edited

Loading

ahg-g Dec 28, 2024

Edwinhr716 Dec 28, 2024 •

edited

Loading

ahg-g Dec 28, 2024

Edwinhr716 Dec 28, 2024

k8s-ci-robot commented Dec 28, 2024 •

edited

Loading

ahg-g Dec 28, 2024

ahg-g commented Dec 28, 2024

k8s-ci-robot commented Dec 28, 2024

Add Controller Revision (Implementation of KEP #238) #277

Add Controller Revision (Implementation of KEP #238) #277

Conversation

Edwinhr716 commented Dec 6, 2024 • edited by ahg-g Loading

What type of PR is this?

What this PR does / why we need it

Which issue(s) this PR fixes

Special notes for your reviewer

Does this PR introduce a user-facing change?

ahg-g left a comment

Choose a reason for hiding this comment

ahg-g left a comment

Choose a reason for hiding this comment

Edwinhr716 commented Dec 23, 2024

ahg-g commented Dec 23, 2024

ahg-g Dec 23, 2024

Choose a reason for hiding this comment

Edwinhr716 commented Dec 27, 2024

Edwinhr716 commented Dec 28, 2024 • edited Loading

ahg-g Dec 28, 2024

Choose a reason for hiding this comment

Edwinhr716 Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

ahg-g Dec 28, 2024

Choose a reason for hiding this comment

Edwinhr716 Dec 28, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 28, 2024 • edited Loading

ahg-g Dec 28, 2024

Choose a reason for hiding this comment

ahg-g commented Dec 28, 2024

k8s-ci-robot commented Dec 28, 2024

Edwinhr716 commented Dec 6, 2024 •

edited by ahg-g

Loading

Edwinhr716 commented Dec 28, 2024 •

edited

Loading

Edwinhr716 Dec 28, 2024 •

edited

Loading

k8s-ci-robot commented Dec 28, 2024 •

edited

Loading