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.

Sign up for GitHub

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

Jump to bottom

Deprecate cloudprovider specific volume limit predicates #74544

Merged
merged 1 commit into from
Mar 8, 2019
Merged

Deprecate cloudprovider specific volume limit predicates #74544

merged 1 commit into from
Mar 8, 2019

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Feb 25, 2019

Fixes #72920

Deprecate AWS, Azure, GCE and Cinder specific volume limit predicates.

/sig storage
/sig scheduling

cc @bsalamat @ravisantoshgudimetla @msau42

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 25, 2019
msau42
msau42 reviewed Feb 25, 2019
View reviewed changes
pkg/scheduler/algorithm/predicates/predicates.go
@@ -79,12 +79,20 @@ const (
// CheckServiceAffinityPred defines the name of predicate checkServiceAffinity.
CheckServiceAffinityPred = "CheckServiceAffinity"
// MaxEBSVolumeCountPred defines the name of predicate MaxEBSVolumeCount.
// DEPRECATED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the in-tree translation layer to CSI going to live? Directly in the CSI predicate? cc @davidz627

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking translation layer is going to live in attach_limit.go util file which is shared between volume plugins and scheduler. But as we discussed I am working on a proposal for using CSINodeInfo for storing attach limits and this detail will be more flexed out in that KEP - kubernetes/enhancements#730

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think adding a comment mentioning enhancements KEP PR within code as a comment might be helpful?

wgliang
wgliang reviewed Feb 26, 2019
View reviewed changes
Copy link
Contributor

@wgliang wgliang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will it be truly deprecated? I mean that there is still relevant code in effect.

@misterikkit
Copy link

Is the replacement already available?

@gnufied
Copy link
Member Author

gnufied commented Feb 26, 2019

@misterikkit Since we are in process of migrating in-tree volume plugins to CSI, there is already a CSI predicate available which handles all volume plugins and hence when using CSI volumes - the cloudprovider specific predicates are not required. We will be able to truly remove these predicates once CSI migration goes GA but we will have a translation layer as well, which would take care of handling in-tree volume plugins even if you are not using CSI.

@gnufied
Copy link
Member Author

gnufied commented Feb 27, 2019

/kind feature
/priority important-soon

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 27, 2019
@gnufied
Copy link
Member Author

gnufied commented Mar 5, 2019

/test pull-kubernetes-integration

ravisantoshgudimetla
ravisantoshgudimetla approved these changes Mar 6, 2019
View reviewed changes
Copy link
Contributor

@ravisantoshgudimetla ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@bsalamat - Do you have any other suggestions? If not, can you add milestone PR?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2019
@ravisantoshgudimetla
Copy link
Contributor

/retest

yastij
yastij approved these changes Mar 6, 2019
View reviewed changes
Copy link
Member

@yastij yastij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

pkg/scheduler/algorithm/predicates/predicates.go
MaxEBSVolumeCountPred = "MaxEBSVolumeCount"
// MaxGCEPDVolumeCountPred defines the name of predicate MaxGCEPDVolumeCount.
// DEPRECATED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any timeline set to move these out of the scheduler ?

@childsb childsb added this to the v1.14 milestone Mar 6, 2019
bsalamat
bsalamat reviewed Mar 6, 2019
View reviewed changes
Copy link
Member

@bsalamat bsalamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @gnufied! I am glad that to see progress here. I have a minor comment.

pkg/scheduler/algorithm/predicates/predicates.go
@@ -79,12 +79,20 @@ const (
// CheckServiceAffinityPred defines the name of predicate checkServiceAffinity.
CheckServiceAffinityPred = "CheckServiceAffinity"
// MaxEBSVolumeCountPred defines the name of predicate MaxEBSVolumeCount.
// DEPRECATED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please point out what the replacement is in the comment. Without a clear replacement, deprecating an existing feature causes confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is mentioned right there!

// DEPRECATED
// All cloudprovider specific predicates are deprecated in favour of MaxCSIVolumeCountPred.

MaxCSIVolumeCountPred will support all these in-tree volume types via migration path we discussed. I am working on updating the Volume limit KEP as per what we discussed - kubernetes/enhancements#730 .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MaxCSIVolumeCountPred will support these in-tree volume types even if you are not using CSI.

bsalamat
bsalamat approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@bsalamat bsalamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, gnufied

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 Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit d5e4973 into kubernetes:master Mar 8, 2019
@draveness draveness mentioned this pull request Sep 5, 2019
Closed
@hex108 hex108 mentioned this pull request Oct 21, 2019
Closed
@carlory carlory mentioned this pull request Apr 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wgliang wgliang wgliang left review comments

@msau42 msau42 msau42 left review comments

@yastij yastij yastij approved these changes

@ravisantoshgudimetla ravisantoshgudimetla ravisantoshgudimetla approved these changes

@bsalamat bsalamat bsalamat approved these changes

@misterikkit misterikkit Awaiting requested review from misterikkit

Assignees

@yastij yastij

@ravisantoshgudimetla ravisantoshgudimetla

@bsalamat bsalamat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.14
Development

Successfully merging this pull request may close these issues.
9 participants
@gnufied @misterikkit @ravisantoshgudimetla @k8s-ci-robot @yastij @wgliang @bsalamat @msau42 @childsb