Skip to content

/kubernetes

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor PV scheduling library into a separate package #75434

Merged
merged 3 commits into from May 4, 2019
+1,313 −1,048

Conversation

Reviewers

@Huang-Wei Huang-Wei

@msau42 msau42

@childsb childsb

@jsafrane jsafrane

Assignees

@msau42 msau42

Labels
approved cncf-cla: yes kind/cleanup lgtm priority/important-soon release-note-none sig/apps sig/scheduling size/XXL
Projects
None yet
Milestone
No milestone
7 participants
@cofyc @maicohjf @k8s-ci-robot @msau42 @cblecker @mikedanese @Huang-Wei
@cofyc
Copy link
Contributor

commented Mar 18, 2019 •
edited

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.

  • 0423f9f: Extract testing VolumeReactor into a separate package pkg/controller/volume/persistentvolume/testing
    • because volume scheduling need this testing utility too
  • 6c6140c
    • Extract utilities into a separate package pkg/controller/volume/persistentvolume/util
    • Refactor PV scheduling library into separate package pkg/controller/volume/scheduling
  • 6ea9a25: Update scheduler to use new volume scheduling library

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress release-note-none kind/cleanup size/XXL cncf-cla: yes needs-sig needs-priority labels Mar 18, 2019

@k8s-ci-robot k8s-ci-robot requested review from childsb and jsafrane Mar 18, 2019

@k8s-ci-robot k8s-ci-robot added sig/apps sig/scheduling and removed needs-sig labels Mar 18, 2019

@cofyc cofyc force-pushed the cofyc:fix56098 branch 2 times, most recently from 738daf1 to de3bb0d Mar 18, 2019

@maicohjf

This comment has been minimized.

Sign in to view
Copy link

commented Mar 18, 2019 •
edited

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 cofyc force-pushed the cofyc:fix56098 branch from 0b425c6 to 22d1333 Mar 18, 2019

@cofyc

cofyc reviewed Mar 18, 2019
View changes

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

This comment has been minimized.

Sign in to view
Copy link
@cofyc

cofyc Mar 18, 2019 •
edited

Author Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@cofyc

cofyc Apr 25, 2019

Author Contributor

follow up issue: #77084
@cofyc

cofyc reviewed Mar 18, 2019
View changes

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")},

This comment has been minimized.

Sign in to view
Copy link
@cofyc

cofyc Mar 18, 2019

Author Contributor

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 cofyc force-pushed the cofyc:fix56098 branch from 22d1333 to 4608ad8 Mar 18, 2019

@cofyc

cofyc reviewed Mar 18, 2019
View changes

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"

This comment has been minimized.

Sign in to view
Copy link
@cofyc

cofyc Mar 18, 2019

Author Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@cofyc

cofyc Apr 25, 2019

Author Contributor

#77085
@cofyc

cofyc reviewed Mar 18, 2019
View changes

pkg/scheduler/scheduler_test.go Outdated
@@ -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"

This comment has been minimized.

Sign in to view
Copy link
@cofyc

cofyc Mar 18, 2019 •
edited

Author Contributor

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 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 k8s-ci-robot removed the do-not-merge/work-in-progress label Mar 18, 2019

@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Mar 18, 2019

/assign @msau42

@k8s-ci-robot k8s-ci-robot assigned msau42 Mar 18, 2019

@k8s-ci-robot k8s-ci-robot added the needs-rebase label Mar 24, 2019

@cofyc cofyc force-pushed the cofyc:fix56098 branch from 4608ad8 to dac3b79 Mar 24, 2019

@k8s-ci-robot k8s-ci-robot removed the needs-rebase label Mar 24, 2019

@k8s-ci-robot k8s-ci-robot added the needs-rebase label Apr 21, 2019

@k8s-ci-robot

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 21, 2019

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.

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.
@msau42

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 25, 2019

/lgtm
/priority important-soon

@k8s-ci-robot k8s-ci-robot added lgtm priority/important-soon and removed needs-priority labels Apr 25, 2019

This was referenced Apr 25, 2019

Open
Open
@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 26, 2019

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

@k8s-ci-robot k8s-ci-robot added the needs-rebase label May 1, 2019

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

@k8s-ci-robot k8s-ci-robot removed lgtm needs-rebase labels May 2, 2019

@cofyc
Update scheduler to use new volume scheduling library 
To fix scheme issue, use k8s.io/client-go/kubernetes/scheme instead of
legacyscheme.
Loading status checks…
214ea1a

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

@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 2, 2019

resolved some conflicts with #75848, no code change
@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 2, 2019

/retest
1 similar comment
@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 2, 2019

/retest
@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 2, 2019

@msau42 rebased without code change, please LGTM again
@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 3, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented May 3, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 3, 2019

@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 4, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented May 4, 2019

/approve
@k8s-ci-robot

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 4, 2019

[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:

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 k8s-ci-robot added the approved label May 4, 2019

@k8s-ci-robot k8s-ci-robot merged commit ef550e6 into kubernetes:master May 4, 2019
19 of 20 checks passed

19 of 20 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation cofyc authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@cofyc cofyc deleted the cofyc:fix56098 branch May 4, 2019

@cofyc

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 4, 2019

@mikedanese Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.