Skip to content
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

feat(scheduling): implement azure, cinder, ebs and gce as filter plugin #86662

Merged

Conversation

@draveness
Copy link
Member

draveness commented Dec 27, 2019

/sig scheduling
/kind feature
/priority important-soon

Fixes #86613

  1. Implement azure, cinder, ebs and gce as filter plugin
  2. Initialize cloud-specific volume limits plugin with NonCSILimits
  3. Remove related predicates

We could remove the non_csi.go and non_csi_test.go after the CSI migration graduates to GA in #82375

NONE
@draveness

This comment has been minimized.

Copy link
Member Author

draveness commented Dec 27, 2019

/assign ahg-g

@draveness draveness force-pushed the draveness:feature/cloud-specific-predicates branch from f778184 to 9679d9e Dec 27, 2019
@draveness

This comment has been minimized.

Copy link
Member Author

draveness commented Dec 27, 2019

/retest

1 similar comment
@draveness

This comment has been minimized.

Copy link
Member Author

draveness commented Dec 27, 2019

/retest

Copy link
Member

ahg-g left a comment

Thanks

@draveness draveness force-pushed the draveness:feature/cloud-specific-predicates branch from 9679d9e to 88bcee5 Dec 28, 2019
@draveness draveness force-pushed the draveness:feature/cloud-specific-predicates branch from 88bcee5 to 4a19b9c Dec 28, 2019
@draveness draveness force-pushed the draveness:feature/cloud-specific-predicates branch from 4a19b9c to 09edc9d Dec 28, 2019
Copy link
Member

ahg-g left a comment

looking at it again, I am not sure why the individual files of each provider was combined into one, it would have been more readable if they were separated (and add a file for the common code), but since you already combined them, I am not going to ask you to revert the change.

var (
csiNode *storage.CSINode
err error
)
Comment on lines 220 to 223

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 28, 2019

Member

not sure this is easier to read than having the var for each variable, this is basically doubling the number of lines used for no reason.

This comment has been minimized.

Copy link
@draveness

draveness Dec 28, 2019

Author Member

done, this is a direct copy of the original code, and I prefer having the var for each of them too,

@draveness

This comment has been minimized.

Copy link
Member Author

draveness commented Dec 28, 2019

looking at it again, I am not sure why the individual files of each provider was combined into one, it would have been more readable if they were separated (and add a file for the common code), but since you already combined them, I am not going to ask you to revert the change.

Thanks, I combined them because all of the codes in the non_csi file will be removed after the CSI migration graduates into GA.

@draveness draveness force-pushed the draveness:feature/cloud-specific-predicates branch from 09edc9d to 320ac4e Dec 28, 2019
@draveness

This comment has been minimized.

Copy link
Member Author

draveness commented Dec 28, 2019

I have addressed the comments, PTAL, Thanks!

@draveness draveness requested a review from ahg-g Dec 28, 2019
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Dec 28, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 28, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 28, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, draveness

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 merged commit d605766 into kubernetes:master Dec 28, 2019
15 checks passed
15 checks passed
cla/linuxfoundation draveness authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 28, 2019
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.