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

Add Cinder Max Volume Limit #72980

Merged
merged 2 commits into from Jan 16, 2019

Conversation

@gnufied
Copy link
Member

gnufied commented Jan 16, 2019

We currently have 3 specific predicates that filter and count attachable limits for AWS, Azure and GCE-PD. This PR brings Cinder in-line with these other volume types.

This PR also implements initial support for reporting volume limits on per-node basis for Openstack nodes. The implementation here however provides a placeholder value which can be overridden.

Add support for max attach limit for Cinder
Add Cinder Max Volume Limit
Also add place holder support for reporting limits from node.
@@ -370,6 +377,8 @@ func getMaxVolumeFunc(filterName string) func(node *v1.Node) int {
return DefaultMaxGCEPDVolumes
case AzureDiskVolumeFilterType:
return DefaultMaxAzureDiskVolumes
case CinderVolumeFilterType:
return volumeutil.DefaultMaxCinderVolumes

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Jan 16, 2019

Contributor

Unrelated: Why do we have DefaultMaxCinderVolumes coming from volumeutil, whereas rest(like DefaultMaxAzureDiskVolumes etc) are coming from the current file

This comment has been minimized.

@gnufied

gnufied Jan 16, 2019

Author Member

I think it is an oversight. All of these defaults should come from volumeutil. I will move them over.

This comment has been minimized.

@gnufied

gnufied Jan 16, 2019

Author Member

Or more specifically - volumeutil.attach_limit.go was introduced as common lib file that could be shared between plugin and scheduler. Most plugins use this file to derive keys and default limits. But scheduler already had those constants and they weren't removed and replaced when this new file was created.

This comment has been minimized.

@ravisantoshgudimetla
@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor

ravisantoshgudimetla commented Jan 16, 2019

/assign

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jan 16, 2019

cc @dims

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 16, 2019

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 16, 2019

@gnufied i expected BlockStorageOpts would be updated to allow an admin to configure the value (default of 256 is ok!)

@childsb

This comment has been minimized.

Copy link
Member

childsb commented Jan 16, 2019

this is a serious issue for people using cinder. approved from sig-storage
/approve

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jan 16, 2019

/test pull-kubernetes-integration

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor

ravisantoshgudimetla commented Jan 16, 2019

@j-griffith
Copy link

j-griffith left a comment

/lgtm seems worthwhile to get folks using the Cinder provider up to speed with the other cloud providers that already added this predicate.

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor

ravisantoshgudimetla commented Jan 16, 2019

Tracking issue to remove cloud provider specific providers in scheduler.

#72920

@gnufied - Assuming that the above issue will be handled and all the cloud provider specific predicates could be replaced with CSI plugin, I am fine with changes from scheduler side.

/cc @bsalamat

/lgtm

@k8s-ci-robot k8s-ci-robot requested a review from bsalamat Jan 16, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 16, 2019

@ravisantoshgudimetla: GitHub didn't allow me to request PR reviews from the following users: -, fyi.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Tracking issue to remove cloud provider specific providers in scheduler.

#72920

@gnufied - Assuming that the above issue will be handled and all the cloud provider specific predicates could be replaced with CSI plugin, I am fine with changes from scheduler side.

/approve

/cc @bsalamat - fyi

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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: childsb, gnufied, ravisantoshgudimetla

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

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor

ravisantoshgudimetla commented Jan 16, 2019

/kind feature

@k8s-ci-robot k8s-ci-robot merged commit d6b7409 into kubernetes:master Jan 16, 2019

18 checks passed

cla/linuxfoundation gnufied authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
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-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@bsalamat

This comment has been minimized.

Copy link
Contributor

bsalamat commented Jan 16, 2019

I am not really fine with this change. The largest cloud providers today have limits which should be removed. Urgency of a customer issue or other similar reasons are not good justifications to worsen the quality of our code.

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 17, 2019

@bsalamat i had requested a specific change as well and it definitely seems to have been merged in a rush for some reason! i'd support a revert and then a proper follow up

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor

ravisantoshgudimetla commented Jan 17, 2019

The largest cloud providers today have limits which should be removed.

@bsalamat - I understand your concern. As discussed offline, I am going to create a PR which mentions explicitly that the predicates that are cloud provider specific are deprecated in this release and in subsequent releases, we plan to remove the entire cloud provider specific predicates and replace them with csi predicate.

i had requested a specific change as well and it definitely seems to have been merged in a rush for some reason! i'd support a revert and then a proper follow up

@dims - I thought you're fine with a follow-up PR and gave my approval. To be clear, @bsalamat's concern here is adding code for cinder where as IIUC your concern is more on the lines of limits to be coming from cloudprovider's config file. So, do you still want this to be handled within scheduler code considering we are going to deprecate it?

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 17, 2019

@ravisantoshgudimetla Merging a PR within 3 hours in a rush with pending comments does not look good from an external perception. especially with all the lgtm-ers and approve-ers from one company. Also, we should try to conduct business in public channels as well. i don't think i mentioned that i was ok with a follow up PR (but that would have been a matter of record if we were not doing things off-line).

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jan 17, 2019

@dims I hadn't intended this PR to get merged fast, I was still thinking about how best to address your concerns when lgtms and approvals rolled in and the bot took over. Sorry about it.

We are going to talk about this PR in sig-storage community meeting. @dims I will follow up with your concerns - #72988

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 17, 2019

Thanks @gnufied

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Jan 17, 2019

FWIW I am a big fan of using /hold to tell our automation that a PR is not yet ready for merge for reasons that can't be expressed otherwise

@childsb

This comment has been minimized.

Copy link
Member

childsb commented Jan 17, 2019

My review & direct discussion regarding the PR had a couple identified issues being addressed in a follow up PR. Next time I'll make sure to use a hold so that everyone has time to review any direct communications. Also this is both a bug/feature, so we were moving fast to try and fix the bug aspect. Anyways, we should have held the PR- apologies @dims

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 17, 2019

Thanks @childsb I see the follow up PR for my concern from @gnufied in #72988. We should file a follow up issue at least for the predicates as well.

@resouer

This comment has been minimized.

Copy link
Member

resouer commented Jan 17, 2019

Emm ... as Dims commented above, I don't think #72988 reflect the concern from sig-scheduling , @bsalamat , @dims and other folks in this thread. cc @gnufied @childsb

I believe the concern is about we are not tend to add any cloud provider related predicates for reason of customer needs, in which context, I would +1 for revert this change for now cc @dims .

The original issue could be addressed in follow up discussion with sig-scheduling + sig-storage.

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jan 17, 2019

@resouer Yes there are two different things going on. #72988 is trying to address concern @dims raised in #72980 (comment) comment.

In general I agree that cloudprovider specific predicates do not belong in scheduler. But we already have 3 different predicates for Azure, GCE and AWS. Without these predicates running a production workload that uses storage is very difficult.

We discussed this PR and forward plans for deprecating cloudprovider predicates in sig-storage community meeting today. I have opened #72920 to track that work. Conclusion from sig-storage meeting was, we should look into deprecating these predicates in 1.14 but we should also consider how that will interact with CSI migration plan. CSI migration design must take into account how volume limits will be handled for existing in-tree plugins.

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jan 17, 2019

@dims @resouer @bsalamat So to avoid any confusion - can you guys please give us your final opinion on this? Should we revert it or not?

@saad-ali @childsb lets finalize this from sig-storage perspective too.

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 17, 2019

@gnufied Most of the code in this PR is in pkg/scheduler so i'll defer to sig-scheduling folks / @bsalamat for making the call.

cc @kubernetes/sig-scheduling-pr-reviews @k82cn

gnufied added a commit to gnufied/kubernetes that referenced this pull request Jan 17, 2019

Revert "Merge pull request kubernetes#72980 from gnufied/cinder-pod-v…
…olumes"

This reverts commit d6b7409, reversing
changes made to 5818be6.
@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jan 17, 2019

I do not think this should be reverted. The PR makes no impact to quality, it brings a feature up to same quality as the existing public cloud providers. I see no reason why we are advantaging public cloud providers.

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jan 17, 2019

To be clear, I agree with the proposal outlined here:
#72980 (comment)

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jan 17, 2019

I agree that the PR should have been open longer for more comment prior to merge. Apologies for that issue as noted above, but still believe the project should ensure users are broadly successful.

@yastij

This comment has been minimized.

Copy link
Member

yastij commented Jan 17, 2019

I do agree here with @bsalamat - having providers predicates already there is not a reason to add new ones (same as we did for in-tree cloudprovider and storage providers).

cc @kubernetes/sig-cloud-provider for the extraction effort

@bsalamat

This comment has been minimized.

Copy link
Contributor

bsalamat commented Jan 17, 2019

I support removing all cloud provider specific code for a more generic approach. I do think that this PR has lowered the quality of our code base. The larger such "exceptions" are, the harder it will be to vote against adding more of them in the future. So, existence of three of them does not justify adding the fourth one.

All being said, I am fine with leaving this PR merged as long as there is a plan to remove all of the similar cloud provider specific code from our code base. I spoke with @ravisantoshgudimetla offline and he has agreed to come up with a plan for 1.14 to remove the cloud provider specific code.

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor

ravisantoshgudimetla commented Jan 17, 2019

Thanks @bsalamat, the plan @gnufied and I agreed upon is to deprecate the hardcoded cloud provider specific predicates in 1.14 so that people will have time to start using CSI based one and in subsequent releases, we will remove the predicates.

@resouer

This comment has been minimized.

Copy link
Member

resouer commented Jan 17, 2019

I am fine as #72920 is raised.

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 17, 2019

whew! thanks everyone for the discussion, decision and future path. as someone who has been championing OpenStack/Kubernetes integration i am pleased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment