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

added nodeVolumeAttachLimit #160

Merged
merged 1 commit into from
Oct 21, 2020
Merged

added nodeVolumeAttachLimit #160

merged 1 commit into from
Oct 21, 2020

Conversation

mganter
Copy link
Contributor

@mganter mganter commented Oct 9, 2020

How to categorize this PR?

/area robustness
/area storage
/kind enhancement
/priority normal
/platform openstack

What this PR does / why we need it:
The introduced flag tells the CSI-Driver, that there is a upper limit for mounted volumes on a node.

Release note:

Added possibility to set nodeVolumeAttachLimit within the cloud profile.

@mganter mganter requested review from a team as code owners October 9, 2020 11:14
@gardener-robot gardener-robot added the kind/api-change API change with impact on API users label Oct 9, 2020
@CLAassistant
Copy link

CLAassistant commented Oct 9, 2020

CLA assistant check
All committers have signed the CLA.

@gardener-robot gardener-robot added needs/second-opinion Needs second review by someone else area/robustness Robustness, reliability, resilience related area/storage Storage related kind/enhancement Enhancement, improvement, extension platform/openstack OpenStack platform/infrastructure priority/normal labels Oct 9, 2020
@gardener-robot
Copy link

@mganter Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Oct 9, 2020
@gardener-robot-ci-3
Copy link
Contributor

Thank you @mganter for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@dkistner
Copy link
Member

Hi @mganter.
Thanks you very much for the PR.
I will have a look soon.

Copy link
Member

@dkistner dkistner left a comment

Choose a reason for hiding this comment

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

Thanks @mganter
I had a brief look. Can you elaborate a little bit on the reasons why the amount of volumes per node need to be limited?
Should that not rather be configured for each Shoot cluster individually instead of globally via the CloudProfile?

@mganter
Copy link
Contributor Author

mganter commented Oct 16, 2020

The limit exists inside of OpenStack. With this option you can tell the scheduler, that he shall not schedule more cinder volumes to a specific node than the specified amount.

If the scheduler does't know it, he will schedule pods to nodes where they can't be satisfied in terms of volumes. So pods will remain in container creation state.

Copy link
Member

@dkistner dkistner left a comment

Choose a reason for hiding this comment

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

Thanks for explanation. I played a little bit around with it and configured a limit of just two disks per node.
If I try to schedule more pods with disks to a node the scheduler tells:

I1019 19:50:13.426597       1 factory.go:445] "Unable to schedule pod; no fit; waiting" pod="default/some-persistence-2" err="0/1 nodes are available: 1 node(s) exceed max volume count."

Also there is an event which indicate the same:

2m34s       Warning   FailedScheduling          pod/some-persistence-2                                            0/1 nodes are available: 1 node(s) exceed max volume count.

The pr looks overall very nice. The tests for the valuesprovider/controlplane controller needs to be adapted. They fail because the new property nodeVolumeAttachLimit is missing here: https://github.com/gardener/gardener-extension-provider-openstack/blob/master/pkg/controller/controlplane/valuesprovider_test.go#L247-L262

We should also mention in the operator docs that this feature will only be available when using CSI.

@gardener-robot gardener-robot added needs/changes Needs (more) changes and removed needs/review Needs review labels Oct 19, 2020
@mganter
Copy link
Contributor Author

mganter commented Oct 20, 2020

Added docs. Updated test.

dkistner
dkistner previously approved these changes Oct 20, 2020
rfranzke
rfranzke previously approved these changes Oct 20, 2020
@rfranzke
Copy link
Member

/reviewed ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 21, 2020
@rfranzke
Copy link
Member

rfranzke commented Oct 21, 2020

@mganter can you check the failing CI? probably you need to run make generate
/status author-action

@gardener-robot gardener-robot added the status/author-action Issue requires issue author action label Oct 21, 2020
@gardener-robot
Copy link

@mganter The pull request was assigned to you under author-action. Please unassign yourself when you are done. Thank you.

@mganter mganter dismissed stale reviews from rfranzke and dkistner via 88b3ebf October 21, 2020 11:43
@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Oct 21, 2020
@mganter
Copy link
Contributor Author

mganter commented Oct 21, 2020

@mganter can you check the failing CI? probably you need to run make generate
/status author-action

make generate results in no change of code. Removing controller-registration.yaml results in recreating it exactly with no diff. I rebased it to the latest rev.

Some system information:
I'm running Manjaro Kernel: Linux 5.8.11-1-MANJARO
ldd (GNU libc) 2.32

Do you know if there are any issues with that?

@rfranzke
Copy link
Member

Now there is a conflict.

Do you know if there are any issues with that?

Hm, good question, can you run make install-requirements and try again?

@gardener-robot gardener-robot removed the needs/rebase Needs git rebase label Oct 21, 2020
@mganter
Copy link
Contributor Author

mganter commented Oct 21, 2020

The conflict is resolved now. (Didn't have the changes from 1 hour ago)
And its re-generated.
I also re-run make install-requirements.

@rfranzke rfranzke added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2020
@rfranzke rfranzke merged commit 19f5108 into gardener:master Oct 21, 2020
@rfranzke
Copy link
Member

Thanks @mganter!

@mganter
Copy link
Contributor Author

mganter commented Oct 21, 2020

Thank you very much @rfranzke @dkistner

@mganter mganter deleted the feature/volumeAttachLimit branch October 21, 2020 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/robustness Robustness, reliability, resilience related area/storage Storage related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/second-opinion Needs second review by someone else platform/openstack OpenStack platform/infrastructure size/s Size of pull request is small (see gardener-robot robot/bots/size.py) status/author-action Issue requires issue author action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants