refactor: migrate networkoverhead and topologicalsort client to ctrl runtime #522

zwpaper · 2023-02-23T16:35:47Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of #485

Special notes for your reviewer:

Does this PR introduce a user-facing change?

migrate appGroup and networkTopology client to ctrl runtime

k8s-ci-robot · 2023-02-23T16:35:58Z

Hi @zwpaper. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

Huang-Wei · 2023-03-02T18:05:33Z

@zwpaper a heads-up: I will cut a v0.25.x release next week. So I'll hold off reviewing PRs that migrate to use ctl-runtime in plugins.

After v0.25.x is cut, and master gets bumped to vendor k8s v1.26.x, I will start review these PRs.

Huang-Wei · 2023-03-02T18:06:24Z

And it'd be very appreciable if you can spread the words to other WIP work of migrating to use ctl-runtime in plugins. 🙏

zwpaper · 2023-03-03T05:24:28Z

gotcha, let PodGroup be the guinea pig for ctl-runtime in 0.25.x, and we can migrate the rest in later releases, I will notify this to the rest of the migrations. @Huang-Wei

Huang-Wei · 2023-03-03T05:42:47Z

gotcha, let PodGroup be the guinea pig for ctl-runtime in 0.25.x

Not quite. Even for PodGroup, we did the migration on controllers only, right? (plugins are still using typed clientset)

zwpaper · 2023-03-03T05:50:29Z

Even for PodGroup, we did the migration on controllers only, right? (plugins are still using typed clientset)

yes, we did only do the PodGroup Controller migration only, I have no particular tendency for the migration in or out of the release, it should be all good depending on your opinions.

Huang-Wei · 2023-03-03T06:06:38Z

Yes, I just meant to leave plugin's migration to 0.26.

k8s-triage-robot · 2023-06-28T12:29:01Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

zwpaper · 2023-06-28T18:00:05Z

busy days past, let's continue the migration!

/remove-lifecycle stale

Huang-Wei · 2023-07-25T17:25:39Z

@zwpaper we may not be able to finish this in this release (v0.26). Let's move it to v0.27.

zwpaper · 2023-10-03T15:40:13Z

finally, all tests passed!

@Huang-Wei can you take a look or assign this to the creator of networkaware plugin

Huang-Wei

Thanks @zwpaper ! Some comments below.

Moreover, I'd expect to eliminate all deps to typed client of networktopology entirely, but I still see these entries:

⇒  ag "github.com/diktyo-io/appgroup-api/pkg/generated"
test/integration/utils.go
42:	agversioned "github.com/diktyo-io/appgroup-api/pkg/generated/clientset/versioned"

test/integration/networkoverhead_test.go
44:	agversioned "github.com/diktyo-io/appgroup-api/pkg/generated/clientset/versioned"

test/integration/topologicalsort_test.go
45:	agversioned "github.com/diktyo-io/appgroup-api/pkg/generated/clientset/versioned"

could you check if you can remove them?

pkg/networkaware/networkoverhead/networkoverhead.go

Huang-Wei · 2023-10-04T20:08:23Z

cc @jpedro1992 for review.

jpedro1992 · 2023-10-05T13:24:20Z

Thanks @zwpaper ! Some comments below.

Moreover, I'd expect to eliminate all deps to typed client of networktopology entirely, but I still see these entries:
⇒  ag "github.com/diktyo-io/appgroup-api/pkg/generated"
test/integration/utils.go
42:	agversioned "github.com/diktyo-io/appgroup-api/pkg/generated/clientset/versioned"

test/integration/networkoverhead_test.go
44:	agversioned "github.com/diktyo-io/appgroup-api/pkg/generated/clientset/versioned"

test/integration/topologicalsort_test.go
45:	agversioned "github.com/diktyo-io/appgroup-api/pkg/generated/clientset/versioned"
could you check if you can remove them?

Currently, both appgroup and networktopology controllers are hosted outside this repo and deployed separately from sig-scheduling scheduler and controller.

Would this break this PR and consequently networkAware plugins? Or are you planning to integrate the controllers?

Huang-Wei · 2023-10-05T16:00:25Z

Would this break this PR and consequently networkAware plugins?

No, controllers hosting outside won't be impacted.

Or are you planning to integrate the controllers?

Nope ;)

jpedro1992 · 2023-10-06T11:38:37Z

Would this break this PR and consequently networkAware plugins?

No, controllers hosting outside won't be impacted.

Ok then, I don't see any issues...

Or are you planning to integrate the controllers?

Nope ;)

OK :)

Huang-Wei · 2023-10-24T04:26:13Z

@zwpaper there are some outstanding comments that need to be resolved.

zwpaper · 2023-10-24T05:55:21Z

thanks for the reminder and comments! I will have a look and fix them this weekend

zwpaper · 2023-10-29T14:38:46Z

It should be good to go now! @Huang-Wei PTAL

Huang-Wei

Some nits; LGTM otherwise.

pkg/networkaware/topologicalsort/topologicalsort.go

Huang-Wei · 2023-10-30T20:24:49Z

pkg/networkaware/topologicalsort/topologicalsort_test.go

-				if err != nil {
-					b.Fatalf("Failed to create Workload %q: %v", p.Name, err)
-				}
+				builder.WithObjects(p.DeepCopy())


I'd like to eliminate this for loop via .WithRuntimeObjects():

diff --git a/pkg/networkaware/topologicalsort/topologicalsort_test.go b/pkg/networkaware/topologicalsort/topologicalsort_test.go index d450cdbd..391b0f22 100644 --- a/pkg/networkaware/topologicalsort/topologicalsort_test.go +++ b/pkg/networkaware/topologicalsort/topologicalsort_test.go @@ -25,6 +25,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/rand" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes/scheme" @@ -250,13 +251,11 @@ func TestTopologicalSortLess(t *testing.T) { s := scheme.Scheme utilruntime.Must(agv1alpha1.AddToScheme(s)) - builder := fake.NewClientBuilder(). + client := fake.NewClientBuilder(). WithScheme(s). - WithStatusSubresource(&agv1alpha1.AppGroup{}) - for _, p := range pods { - builder.WithObjects(p.DeepCopy()) - } - client := builder.Build() + WithRuntimeObjects(pods...). + WithStatusSubresource(&agv1alpha1.AppGroup{}). + Build() // Sort TopologyList by Selector sort.Sort(util.ByWorkloadSelector(tt.appGroup.Status.TopologyOrder)) @@ -427,19 +426,16 @@ func BenchmarkTopologicalSortPlugin(b *testing.B) { } for _, tt := range tests { b.Run(tt.name, func(b *testing.B) { - ps := makePodsAppGroup(tt.deploymentNames, tt.agName, tt.podPhase) s := scheme.Scheme utilruntime.Must(agv1alpha1.AddToScheme(s)) - builder := fake.NewClientBuilder(). + client := fake.NewClientBuilder(). WithScheme(s). + WithRuntimeObjects(ps...). WithStatusSubresource(&agv1alpha1.AppGroup{}). - WithObjects(tt.appGroup) - for _, p := range ps { - builder.WithObjects(p.DeepCopy()) - } - client := builder.Build() + WithObjects(tt.appGroup). + Build() ts := &TopologicalSort{ Client: client, @@ -449,9 +445,6 @@ func BenchmarkTopologicalSortPlugin(b *testing.B) { pInfo1 := getPodInfos(b, tt.podNum, tt.agName, tt.selectors, tt.deploymentNames) pInfo2 := getPodInfos(b, tt.podNum, tt.agName, tt.selectors, tt.deploymentNames) - //b.Logf("len(pInfo1): %v", len(pInfo1)) - //b.Logf("len(pInfo2): %v", len(pInfo2)) - b.ResetTimer() for i := 0; i < b.N; i++ { sorting := func(i int) { @@ -498,8 +491,8 @@ func Until(ctx context.Context, pieces int, doWorkPiece workqueue.DoWorkPieceFun workqueue.ParallelizeUntil(ctx, parallelism, pieces, doWorkPiece, chunkSizeFor(pieces)) } -func makePodsAppGroup(podNames []string, agName string, phase v1.PodPhase) []*v1.Pod { - pds := make([]*v1.Pod, 0) +func makePodsAppGroup(podNames []string, agName string, phase v1.PodPhase) []runtime.Object { + var pds []runtime.Object for _, name := range podNames { pod := st.MakePod().Namespace("default").Name(name).Obj() pod.Labels = map[string]string{agv1alpha1.AppGroupLabel: agName}

test/integration/elasticquota_controller_test.go

Signed-off-by: Wei Zhang <kweizh@gmail.com>

Huang-Wei

/lgtm
/approve

Thanks @zwpaper !

k8s-ci-robot · 2023-10-31T18:47:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, zwpaper

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~~ [Huang-Wei]

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 requested review from cwdsuzhou and seanmalloy February 23, 2023 16:36

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 23, 2023

zwpaper mentioned this pull request Feb 24, 2023

migrate controllers and clients to leverage controller-runtime #485

Closed

17 tasks

zwpaper mentioned this pull request Mar 3, 2023

REQUEST: New membership for zwpaper kubernetes/org#4061

Closed

9 tasks

Huang-Wei added this to the v1.26 milestone Mar 3, 2023

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 28, 2023

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 28, 2023

zwpaper force-pushed the refactor-ctrl-runtime branch from b0f47ff to 108bc91 Compare July 24, 2023 17:48

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

zwpaper changed the title ~~WIP refactor: migrate appgroup and net topo client to ctrl runtime~~ WIP refactor: migrate networkoverhead and topologicalsort client to ctrl runtime Jul 24, 2023

Huang-Wei modified the milestones: v1.26, v1.27 Jul 25, 2023

zwpaper force-pushed the refactor-ctrl-runtime branch 2 times, most recently from 90307d6 to 86e8724 Compare July 30, 2023 11:22

zwpaper force-pushed the refactor-ctrl-runtime branch from e456e8f to 56f28f5 Compare September 29, 2023 07:20

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 29, 2023

zwpaper force-pushed the refactor-ctrl-runtime branch 2 times, most recently from 6e62a5c to c2d873d Compare October 3, 2023 15:09

zwpaper changed the title ~~WIP refactor: migrate networkoverhead and topologicalsort client to ctrl runtime~~ refactor: migrate networkoverhead and topologicalsort client to ctrl runtime Oct 3, 2023

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 Oct 3, 2023

Huang-Wei reviewed Oct 4, 2023

View reviewed changes

zwpaper force-pushed the refactor-ctrl-runtime branch 2 times, most recently from 65d24a6 to cd7aea8 Compare October 29, 2023 14:21

Huang-Wei reviewed Oct 30, 2023

View reviewed changes

refactor: migrate appgroup and net topo client to ctrl runtime

9096af8

Signed-off-by: Wei Zhang <kweizh@gmail.com>

zwpaper force-pushed the refactor-ctrl-runtime branch 2 times, most recently from 7d4e69c to a4ecf1a Compare October 31, 2023 17:13

test: migrate networkaware testing to ctrl runtime

7098da7

zwpaper force-pushed the refactor-ctrl-runtime branch from a4ecf1a to 7098da7 Compare October 31, 2023 17:26

Huang-Wei reviewed Oct 31, 2023

View reviewed changes

k8s-ci-robot assigned Huang-Wei Oct 31, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2023

k8s-ci-robot merged commit ca5d17b into kubernetes-sigs:master Oct 31, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: migrate networkoverhead and topologicalsort client to ctrl runtime #522

refactor: migrate networkoverhead and topologicalsort client to ctrl runtime #522

zwpaper commented Feb 23, 2023 •

edited by Huang-Wei

Loading

k8s-ci-robot commented Feb 23, 2023

Huang-Wei commented Mar 2, 2023

Huang-Wei commented Mar 2, 2023

zwpaper commented Mar 3, 2023

Huang-Wei commented Mar 3, 2023

zwpaper commented Mar 3, 2023

Huang-Wei commented Mar 3, 2023

k8s-triage-robot commented Jun 28, 2023

zwpaper commented Jun 28, 2023

Huang-Wei commented Jul 25, 2023

zwpaper commented Oct 3, 2023

Huang-Wei left a comment

Huang-Wei commented Oct 4, 2023

jpedro1992 commented Oct 5, 2023

Huang-Wei commented Oct 5, 2023

jpedro1992 commented Oct 6, 2023

Huang-Wei commented Oct 24, 2023

zwpaper commented Oct 24, 2023

zwpaper commented Oct 29, 2023

Huang-Wei left a comment

Huang-Wei Oct 30, 2023

Huang-Wei left a comment

k8s-ci-robot commented Oct 31, 2023

refactor: migrate networkoverhead and topologicalsort client to ctrl runtime #522

refactor: migrate networkoverhead and topologicalsort client to ctrl runtime #522

Conversation

zwpaper commented Feb 23, 2023 • edited by Huang-Wei 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?

k8s-ci-robot commented Feb 23, 2023

Huang-Wei commented Mar 2, 2023

Huang-Wei commented Mar 2, 2023

zwpaper commented Mar 3, 2023

Huang-Wei commented Mar 3, 2023

zwpaper commented Mar 3, 2023

Huang-Wei commented Mar 3, 2023

k8s-triage-robot commented Jun 28, 2023

zwpaper commented Jun 28, 2023

Huang-Wei commented Jul 25, 2023

zwpaper commented Oct 3, 2023

Huang-Wei left a comment

Choose a reason for hiding this comment

Huang-Wei commented Oct 4, 2023

jpedro1992 commented Oct 5, 2023

Huang-Wei commented Oct 5, 2023

jpedro1992 commented Oct 6, 2023

Huang-Wei commented Oct 24, 2023

zwpaper commented Oct 24, 2023

zwpaper commented Oct 29, 2023

Huang-Wei left a comment

Choose a reason for hiding this comment

Huang-Wei Oct 30, 2023

Choose a reason for hiding this comment

Huang-Wei left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 31, 2023

zwpaper commented Feb 23, 2023 •

edited by Huang-Wei

Loading