Adding SubdomainPolicy to support a service per replica #197

Edwinhr716 · 2024-08-16T22:31:25Z

What type of PR is this?

/kind feature

What this PR does / why we need it

Details can be found in KEP #188

Which issue(s) this PR fixes

Fixes #173

Special notes for your reviewer

Does this PR introduce a user-facing change?

Adds NetworkConfig and SubdomainPolicy to the API

pkg/controllers/leaderworkerset_controller.go

test/testutils/validators.go

api/leaderworkerset/v1/leaderworkerset_types.go

liurupeng · 2024-08-20T06:36:37Z

Would be great if you could

Add one test to verify that after headless service mode update, for tpu case, we could see container env vars being updated in integration test
Add a e2e test to verify the creation of each headless service mode and an update
Manually test the multi-host TPU solution for not configuration the headless service mode and switch to the "LeaderSharedWorkerDedicated" case
We may need to update the KEP for implementation part about how to populate TPU env vars for LeadersSharedWorkersDedicated mode and handle update

Edwinhr716 · 2024-08-21T17:39:13Z

I think to test that scenario, an e2e is better since the env variables change due to the Pod.Spec.Subdomain value changing, which in a integration-test for the pod-webhook we'll have to manually change that value
I can do an e2e test that covers both 1 & 2

pkg/controllers/leaderworkerset_controller.go

api/leaderworkerset/v1/leaderworkerset_types.go

liurupeng · 2024-09-03T20:34:48Z

pkg/controllers/leaderworkerset_controller.go

@@ -122,27 +122,60 @@ func (r *LeaderWorkerSetReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	return ctrl.Result{}, nil
 }

-func (r *LeaderWorkerSetReconciler) createHeadlessServiceIfNotExists(ctx context.Context, lws *leaderworkerset.LeaderWorkerSet) error {
+func (r *LeaderWorkerSetReconciler) createMultipleHeadlessServices(ctx context.Context, lws *leaderworkerset.LeaderWorkerSet, replicas int32) error {


ReconcileHeadlessServices

test/testutils/validators.go

test/e2e/e2e_test.go

pkg/controllers/leaderworkerset_controller.go

liurupeng · 2024-09-05T04:24:29Z

overall lgtm, @ahg-g could help check as well? thanks!

…hing struct

Co-authored-by: RupengLiu <rupliu@google.com>

ahg-g

Didn't review the tests yet.

I think we can simplify the implementation, see comments.

pkg/controllers/leaderworkerset_controller.go

pkg/webhooks/leaderworkerset_webhook.go

api/leaderworkerset/v1/leaderworkerset_types.go

pkg/webhooks/leaderworkerset_webhook.go

pkg/controllers/leaderworkerset_controller.go

pkg/controllers/pod_controller.go

test/testutils/validators.go

test/e2e/e2e_test.go

ahg-g · 2024-09-12T18:57:28Z

/lgtm
/approve
/label tide/merge-method-squash

k8s-ci-robot · 2024-09-12T18:57:34Z

[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 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 Aug 16, 2024

k8s-ci-robot requested review from ahg-g and liurupeng August 16, 2024 22:31

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

kannon92 reviewed Aug 17, 2024

View reviewed changes

pkg/controllers/leaderworkerset_controller.go Outdated Show resolved Hide resolved

liurupeng reviewed Aug 20, 2024

View reviewed changes

pkg/controllers/leaderworkerset_controller.go Outdated Show resolved Hide resolved

liurupeng reviewed Aug 20, 2024

View reviewed changes

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

liurupeng reviewed Aug 20, 2024

View reviewed changes

api/leaderworkerset/v1/leaderworkerset_types.go Outdated Show resolved Hide resolved

Edwinhr716 changed the title ~~Adding SubdomainPolicy to support a service for the leaders, and a service per replica for the workers~~ [WIP] Adding SubdomainPolicy to support a service for the leaders, and a service per replica for the workers Aug 21, 2024

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 21, 2024

Edwinhr716 changed the title ~~[WIP] Adding SubdomainPolicy to support a service for the leaders, and a service per replica for the workers~~ Adding SubdomainPolicy to support a service per replica Aug 23, 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 Aug 23, 2024

Edwinhr716 changed the title ~~Adding SubdomainPolicy to support a service per replica~~ [WIP] Adding SubdomainPolicy to support a service per replica Aug 28, 2024

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

Edwinhr716 changed the title ~~[WIP] Adding SubdomainPolicy to support a service per replica~~ Adding SubdomainPolicy to support a service per replica Aug 30, 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 Aug 30, 2024

liurupeng reviewed Sep 3, 2024

View reviewed changes

liurupeng reviewed Sep 4, 2024

View reviewed changes

pkg/controllers/leaderworkerset_controller.go Outdated Show resolved Hide resolved

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

Edwinhr716 added 5 commits September 5, 2024 17:05

initial prototype

11f87ae

added integration tests, changed to have NetworkConfig be the overarc…

a689a3d

…hing struct

fixed labels and comments'

29b0b30

adding support for replicas were networkconfig has not been defaulted

75a88e5

changed implementation to UniquePerReplica

3473276

Edwinhr716 and others added 9 commits September 5, 2024 17:49

fixed tests

d3205a3

fixed bug with annotation

0d00a3c

changing from one policy to the other now triggers a rollingUpdate

1f8acd5

fixed lint error

c6c0392

added check for nil

6150bbf

Update api/leaderworkerset/v1/leaderworkerset_types.go

c414cf5

Co-authored-by: RupengLiu <rupliu@google.com>

Update pkg/controllers/leaderworkerset_controller.go

1e52e8c

Co-authored-by: RupengLiu <rupliu@google.com>

extended e2e test to validate from UniquePerReplica to Shared

ff86c59

rebased

103a98a

Edwinhr716 force-pushed the headless-service-impl branch from e4d4b04 to 103a98a Compare September 5, 2024 18:26

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

ahg-g reviewed Sep 6, 2024

View reviewed changes

Edwinhr716 added 2 commits September 10, 2024 02:56

changed to creating UniquePerReplica in pod controller

0dcc715

fixed lint error

c342d77

ahg-g reviewed Sep 12, 2024

View reviewed changes

pkg/controllers/pod_controller.go Outdated Show resolved Hide resolved

test/testutils/validators.go Show resolved Hide resolved

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

test/e2e/e2e_test.go Outdated Show resolved Hide resolved

cleaned up tests, changed createHeadlessService to be a single function

4cefc55

Edwinhr716 force-pushed the headless-service-impl branch from 7a3c05e to 4cefc55 Compare September 12, 2024 18:47

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 12, 2024

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 12, 2024

k8s-ci-robot assigned ahg-g Sep 12, 2024

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

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

k8s-ci-robot merged commit 39f4dd3 into kubernetes-sigs:main Sep 12, 2024
9 checks passed

Edwinhr716 mentioned this pull request Oct 7, 2024

Update KEP #173 to reflect actual implementation #228

Closed

Edwinhr716 mentioned this pull request Oct 31, 2024

When rollout new controller version, it should not trigger a rolling update #240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding SubdomainPolicy to support a service per replica #197

Adding SubdomainPolicy to support a service per replica #197

Edwinhr716 commented Aug 16, 2024

liurupeng commented Aug 20, 2024 •

edited

Loading

Edwinhr716 commented Aug 21, 2024

liurupeng Sep 3, 2024

liurupeng commented Sep 5, 2024

ahg-g left a comment

ahg-g commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

Adding SubdomainPolicy to support a service per replica #197

Adding SubdomainPolicy to support a service per replica #197

Conversation

Edwinhr716 commented Aug 16, 2024

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?

liurupeng commented Aug 20, 2024 • edited Loading

Edwinhr716 commented Aug 21, 2024

liurupeng Sep 3, 2024

Choose a reason for hiding this comment

liurupeng commented Sep 5, 2024

ahg-g left a comment

Choose a reason for hiding this comment

ahg-g commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

liurupeng commented Aug 20, 2024 •

edited

Loading