Refactor PV scheduling library into a separate package #75434

cofyc · 2019-03-18T05:52:07Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #56098

Special notes for your reviewer:

The refactoring is basically simple, but lots of code need to be extracted into shared places (testing, util) to be used by persistentvolume and volume scheduling packages.

0423f9fd097a97e20402cc133893c9798f9b5a51: Extract testing VolumeReactor into a separate package pkg/controller/volume/persistentvolume/testing
- because volume scheduling need this testing utility too
6c6140c995f16bb9e82f7546e4c2224008212b04
- Extract utilities into a separate package pkg/controller/volume/persistentvolume/util
- Refactor PV scheduling library into separate package pkg/controller/volume/scheduling
6ea9a2513987757b6f1a1ff2a71f1c485c9a1a39: Update scheduler to use new volume scheduling library

Does this PR introduce a user-facing change?:

NONE

maicohjf · 2019-03-18T08:50:27Z

Set configuration context $ kubectl config use-context k8s
List all PVs sorted by name, saving the full kubectl output to /opt/KUCC000/my_volumes . Use
kubectl s own functionally for sorting the output, and do not manipulate it any further.
Question weight 3%

$kubectl get pv --all-namespaces --sort-by="metadata.name" >>
/opt/KUCC000/my_volumes or
$kubectl get pv --all-namespaces --sort-by="spec.capacity.storage" >>
/opt/KUCC000/my_volumes

cofyc · 2019-03-18T09:23:02Z

hack/.golint_failures

@@ -107,6 +107,8 @@ pkg/controller/volume/events
 pkg/controller/volume/expand
 pkg/controller/volume/persistentvolume
 pkg/controller/volume/persistentvolume/options
+pkg/controller/volume/persistentvolume/testing
+pkg/controller/volume/scheduling


Code of testing and scheduling package is extracted from pkg/controller/volume/persistentvolume and has some lint failures. I didn't fix them because I don't want to make this PR too complicated. I will fix them in a follow up PR.

follow up issue: #77084

cofyc · 2019-03-18T09:27:11Z

pkg/controller/volume/persistentvolume/provision_test.go

 				// Inject error to the first
 				// kubeclient.PersistentVolumes.Create() call. All other calls
 				// will succeed.
-				{"create", "persistentvolumes", errors.New("Mock creation error")},
+				{Verb: "create", Resource: "persistentvolumes", Error: errors.New("Mock creation error")},


These key fields are required if we composite a public struct from another package, otherwise composite literal uses unkeyed fields is emitted by go vet.

cofyc · 2019-03-18T09:43:36Z

pkg/controller/volume/persistentvolume/util/util.go

+	// NotSupportedProvisioner is a special provisioner name which can be set
+	// in storage class to indicate dynamic provisioning is not supported by
+	// the storage.
+	NotSupportedProvisioner = "kubernetes.io/no-provisioner"


These constants are duplicated from persistentvolume. A lot of places use private copy in persistentvolume, I can update them to use constants in this shared package in a follow up PR.

cofyc · 2019-03-18T09:49:21Z

pkg/scheduler/scheduler_test.go

@@ -38,7 +38,8 @@ import (
 	clientcache "k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
-	"k8s.io/kubernetes/pkg/controller/volume/persistentvolume"
+	_ "k8s.io/kubernetes/pkg/apis/core/install"


Why I import this package explicitly is because we need to register core API types (e.g. v1.Pod used in this file) into legacyscheme.Scheme. In old code, it is registered implicitly in k8s.io/kubernetes/pkg/controller/volume/persistentvolume.

Another way is to use k8s.io/client-go/kubernetes/scheme which registers all non-internal API types by default.

cofyc · 2019-03-18T10:00:01Z

/assign @msau42

k8s-ci-robot · 2019-04-21T23:51:09Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

cofyc · 2019-04-25T16:48:48Z

hi, @deads2k @mikedanese @ixdy
Changes under pkg/controller/volume had been approved by @msau42 already, but this PR modified pkg/controller/BUILD because a new package is added, I need your approval, could you do me a favor?

msau42 · 2019-04-25T16:54:23Z

/lgtm
/priority important-soon

cofyc · 2019-04-26T16:01:20Z

cc @derekwaynecarr ask for approval on pkg/controller/BUILD which is modified by a new package under pkg/controller/volume.

To fix scheme issue, use k8s.io/client-go/kubernetes/scheme instead of legacyscheme.

cofyc · 2019-05-02T06:59:38Z

resolved some conflicts with #75848, no code change

cofyc · 2019-05-02T08:15:00Z

/retest

cofyc · 2019-05-02T09:31:50Z

/retest

cofyc · 2019-05-02T14:45:05Z

@msau42 rebased without code change, please LGTM again

cofyc · 2019-05-03T01:09:50Z

cc @liggitt ask for approval on pkg/controller/BUILD which is modified by a new package under pkg/controller/volume. Thanks!

msau42 · 2019-05-03T01:27:11Z

/lgtm

cofyc · 2019-05-04T01:02:09Z

cc @thockin ask for an approval on pkg/controller because BUILD file is modified by a new package under pkg/controller/volume. Thanks!

mikedanese · 2019-05-04T03:10:52Z

/approve

k8s-ci-robot · 2019-05-04T03:11:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, cofyc, Huang-Wei, mikedanese, msau42

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:

~~cmd/kube-scheduler/OWNERS~~ [Huang-Wei,mikedanese]
~~hack/OWNERS~~ [cblecker]
~~pkg/controller/OWNERS~~ [mikedanese]
~~pkg/scheduler/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

cofyc · 2019-05-04T15:57:30Z

@mikedanese Thanks!

k8s-ci-robot requested review from childsb and jsafrane March 18, 2019 05:53

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 18, 2019

cofyc force-pushed the fix56098 branch 2 times, most recently from 738daf1 to de3bb0d Compare March 18, 2019 06:00

cofyc force-pushed the fix56098 branch from 0b425c6 to 22d1333 Compare March 18, 2019 09:17

cofyc commented Mar 18, 2019

View reviewed changes

cofyc force-pushed the fix56098 branch from 22d1333 to 4608ad8 Compare March 18, 2019 09:31

cofyc commented Mar 18, 2019

View reviewed changes

cofyc changed the title ~~WIP: Refactor PV scheduling library into separate package~~ Refactor PV scheduling library into separate package Mar 18, 2019

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 Mar 18, 2019

k8s-ci-robot assigned msau42 Mar 18, 2019

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

cofyc force-pushed the fix56098 branch from 4608ad8 to dac3b79 Compare March 24, 2019 06:56

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

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

This was referenced Apr 25, 2019

Follow up of #75434: fix go lint failures #77084

Closed

Follow up of #75434: remove duplicate constants #77085

Closed

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

cofyc added 2 commits May 2, 2019 14:45

Extract testing VolumeReactor into a separate package

0b6c028

Refactor PV scheduling library into separate package

842fed6

cofyc force-pushed the fix56098 branch from 6ea9a25 to ac12a9b Compare May 2, 2019 06:48

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 2, 2019

Update scheduler to use new volume scheduling library

214ea1a

To fix scheme issue, use k8s.io/client-go/kubernetes/scheme instead of legacyscheme.

cofyc force-pushed the fix56098 branch from ac12a9b to 214ea1a Compare May 2, 2019 06:58

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2019

k8s-ci-robot merged commit ef550e6 into kubernetes:master May 4, 2019

cofyc deleted the fix56098 branch May 4, 2019 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor PV scheduling library into a separate package #75434

Refactor PV scheduling library into a separate package #75434

cofyc commented Mar 18, 2019 •

edited

maicohjf commented Mar 18, 2019 •

edited

cofyc Mar 18, 2019 •

edited

cofyc Apr 25, 2019

cofyc Mar 18, 2019

cofyc Mar 18, 2019

cofyc Apr 25, 2019

cofyc Mar 18, 2019 •

edited

cofyc commented Mar 18, 2019

k8s-ci-robot commented Apr 21, 2019

cofyc commented Apr 25, 2019

msau42 commented Apr 25, 2019

cofyc commented Apr 26, 2019

cofyc commented May 2, 2019

cofyc commented May 2, 2019

cofyc commented May 2, 2019

cofyc commented May 2, 2019

cofyc commented May 3, 2019

msau42 commented May 3, 2019

cofyc commented May 4, 2019

mikedanese commented May 4, 2019

k8s-ci-robot commented May 4, 2019

cofyc commented May 4, 2019

Refactor PV scheduling library into a separate package #75434

Refactor PV scheduling library into a separate package #75434

Conversation

cofyc commented Mar 18, 2019 • edited

maicohjf commented Mar 18, 2019 • edited

cofyc Mar 18, 2019 • edited

Choose a reason for hiding this comment

cofyc Apr 25, 2019

Choose a reason for hiding this comment

cofyc Mar 18, 2019

Choose a reason for hiding this comment

cofyc Mar 18, 2019

Choose a reason for hiding this comment

cofyc Apr 25, 2019

Choose a reason for hiding this comment

cofyc Mar 18, 2019 • edited

Choose a reason for hiding this comment

cofyc commented Mar 18, 2019

k8s-ci-robot commented Apr 21, 2019

cofyc commented Apr 25, 2019

msau42 commented Apr 25, 2019

cofyc commented Apr 26, 2019

cofyc commented May 2, 2019

cofyc commented May 2, 2019

cofyc commented May 2, 2019

cofyc commented May 2, 2019

cofyc commented May 3, 2019

msau42 commented May 3, 2019

cofyc commented May 4, 2019

mikedanese commented May 4, 2019

k8s-ci-robot commented May 4, 2019

cofyc commented May 4, 2019

cofyc commented Mar 18, 2019 •

edited

maicohjf commented Mar 18, 2019 •

edited

cofyc Mar 18, 2019 •

edited

cofyc Mar 18, 2019 •

edited