migrate elasticquota/capacityscheduling to controller-runtime #665

Huang-Wei · 2023-11-15T21:39:38Z

What type of PR is this?

/kind feature
/kind cleanup

What this PR does / why we need it:

migrate elasticquota/capacityscheduling to controller-runtime

This is the last piece to complete #485.

Which issue(s) this PR fixes:

Part of #485

Special notes for your reviewer:

Now, pkg/generated is the only package that include generated type client bits:

⇒  ag "scheduler-plugins/pkg/generated/clientset/versioned"
pkg/generated/informers/externalversions/scheduling/v1alpha1/elasticquota.go
30:	versioned "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned"

pkg/generated/informers/externalversions/scheduling/v1alpha1/podgroup.go
30:	versioned "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned"

pkg/generated/informers/externalversions/internalinterfaces/factory_interfaces.go
27:	versioned "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned"

pkg/generated/informers/externalversions/factory.go
30:	versioned "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned"

pkg/generated/clientset/versioned/clientset.go
28:	schedulingv1alpha1 "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned/typed/scheduling/v1alpha1"

pkg/generated/clientset/versioned/typed/scheduling/v1alpha1/elasticquota.go
30:	scheme "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned/scheme"

pkg/generated/clientset/versioned/typed/scheduling/v1alpha1/podgroup.go
30:	scheme "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned/scheme"

pkg/generated/clientset/versioned/typed/scheduling/v1alpha1/scheduling_client.go
26:	"sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned/scheme"

pkg/generated/clientset/versioned/typed/scheduling/v1alpha1/fake/fake_scheduling_client.go
24:	v1alpha1 "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned/typed/scheduling/v1alpha1"

pkg/generated/clientset/versioned/fake/clientset_generated.go
27:	clientset "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned"
28:	schedulingv1alpha1 "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned/typed/scheduling/v1alpha1"
29:	fakeschedulingv1alpha1 "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned/typed/scheduling/v1alpha1/fake"

But, I'm not going to remove them. Instead, I will send out a notice and deprecation notice in the release notes (v0.27). So plugins that use this repo's typed clientset have buffer to make changes.

Does this PR introduce a user-facing change?

migrate elasticquota/capacityscheduling to controller-runtime

netlify · 2023-11-15T21:39:44Z

✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.

Name	Link
🔨 Latest commit	`28e0736`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-scheduler-plugins/deploys/6556448995c685000828d021

Huang-Wei · 2023-11-15T21:40:06Z

@zwpaper please take a look.

Huang-Wei · 2023-11-15T21:42:15Z

pkg/capacityscheduling/capacity_scheduling.go

-			FilterFunc: func(obj interface{}) bool {
-				switch t := obj.(type) {
-				case *v1alpha1.ElasticQuota:
+	dynamicCache, err := ctrlruntimecache.New(handle.KubeConfig(), ctrlruntimecache.Options{Scheme: scheme})


Also cc @denkensk, this might need your second look, as I'm changing typed informer to controller-runtime's equivalent (I think ;)) utility. Please shed your light.

look good to me.

Huang-Wei · 2023-11-15T22:42:33Z

The unit test failure is interesting:

        main_test.go:782: initializing profiles: creating profile for scheduler name default-scheduler: initializing plugin "CapacityScheduling": no matches for kind "ElasticQuota" in version "scheduling.x-k8s.io/v1alpha1"

This error occurred when hitting this (new) line:

elasticQuotaInformer, err := dynamicCache.GetInformer(context.Background(), &v1alpha1.ElasticQuota{})

But why integreation test passed? It's b/c integration test fully spins up the CRD (ElasticQuota), while main_test.go doesn't. Unfortunately the (dynamic) GVK discovery in controller-runtime libs issue a http call to discover all API resources, but when we used typed client, it's not needed.

Huang-Wei · 2023-11-16T06:29:19Z

The 2nd commit fixed the issue.

denkensk · 2023-11-16T10:46:06Z

pkg/capacityscheduling/capacity_scheduling.go

+	} else {
+		c.client = client
 	}


``

Suggested change

} else {

c.client = client

}

}

c.client = client

denkensk · 2023-11-16T10:46:38Z

pkg/util/client.go

@@ -0,0 +1,28 @@
+package util


let me remove this file. It's not used now.

denkensk · 2023-11-16T10:47:22Z

pkg/capacityscheduling/capacity_scheduling.go

-			FilterFunc: func(obj interface{}) bool {
-				switch t := obj.(type) {
-				case *v1alpha1.ElasticQuota:
+	dynamicCache, err := ctrlruntimecache.New(handle.KubeConfig(), ctrlruntimecache.Options{Scheme: scheme})


look good to me.

yibozhuang

/lgtm

k8s-ci-robot · 2023-11-16T23:59:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Nov 15, 2023

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 15, 2023

k8s-ci-robot requested review from swatisehgal and zwpaper November 15, 2023 21:39

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

Huang-Wei removed the request for review from swatisehgal November 15, 2023 21:39

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

Huang-Wei force-pushed the elasticquota-migrate-controller-runtime branch from 0eb30b4 to b6cb294 Compare November 15, 2023 21:40

Huang-Wei commented Nov 15, 2023

View reviewed changes

Huang-Wei requested a review from denkensk November 15, 2023 21:42

Huang-Wei mentioned this pull request Nov 15, 2023

migrate controllers and clients to leverage controller-runtime #485

Closed

17 tasks

Huang-Wei force-pushed the elasticquota-migrate-controller-runtime branch 2 times, most recently from 4ee7493 to c06e135 Compare November 16, 2023 00:54

denkensk reviewed Nov 16, 2023

View reviewed changes

Huang-Wei added 2 commits November 16, 2023 08:34

migrate elalsticquota/capacityscheduling to controller-runtime

ce51061

refactor main_test.go: install CRDs using envtest

28e0736

Huang-Wei force-pushed the elasticquota-migrate-controller-runtime branch from c06e135 to 28e0736 Compare November 16, 2023 16:34

yibozhuang approved these changes Nov 16, 2023

View reviewed changes

k8s-ci-robot assigned yibozhuang Nov 16, 2023

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

k8s-ci-robot merged commit fc5b4b2 into kubernetes-sigs:master Nov 17, 2023

Huang-Wei deleted the elasticquota-migrate-controller-runtime branch November 17, 2023 00:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate elasticquota/capacityscheduling to controller-runtime #665

migrate elasticquota/capacityscheduling to controller-runtime #665

Huang-Wei commented Nov 15, 2023

netlify bot commented Nov 15, 2023 •

edited

Loading

Huang-Wei commented Nov 15, 2023

Huang-Wei Nov 15, 2023

denkensk Nov 16, 2023

Huang-Wei commented Nov 15, 2023

Huang-Wei commented Nov 16, 2023

denkensk Nov 16, 2023

denkensk Nov 16, 2023

Huang-Wei Nov 16, 2023

denkensk Nov 16, 2023

yibozhuang left a comment

k8s-ci-robot commented Nov 16, 2023

migrate elasticquota/capacityscheduling to controller-runtime #665

migrate elasticquota/capacityscheduling to controller-runtime #665

Conversation

Huang-Wei commented Nov 15, 2023

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?

netlify bot commented Nov 15, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.

Huang-Wei commented Nov 15, 2023

Huang-Wei Nov 15, 2023

Choose a reason for hiding this comment

denkensk Nov 16, 2023

Choose a reason for hiding this comment

Huang-Wei commented Nov 15, 2023

Huang-Wei commented Nov 16, 2023

denkensk Nov 16, 2023

Choose a reason for hiding this comment

denkensk Nov 16, 2023

Choose a reason for hiding this comment

Huang-Wei Nov 16, 2023

Choose a reason for hiding this comment

denkensk Nov 16, 2023

Choose a reason for hiding this comment

yibozhuang left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 16, 2023

netlify bot commented Nov 15, 2023 •

edited

Loading