Setting CSI AWS topology label on machine deployment creation to allow scale-up #365

himanshu-kun · 2021-06-21T17:41:53Z

How to categorize this PR?

/area auto-scaling
/kind enhancement
/platform aws

What this PR does / why we need it:
adds driver specific CSI label topology.ebs.csi.aws.com/zone during machineDeployment creation
Which issue(s) this PR fixes:
Fixes #364

Special notes for your reviewer:
also made changes to unit test written for machineDeployment generation by adding relevant variables
Release note:

AWS CSI driver specific topology label  "topology.ebs.csi.aws.com/zone" is set as a label on machine deployments to allow the proper working of cluster-autoscaler during scale-up.

gardener-robot-ci-3 · 2021-06-21T17:42:15Z

Thank you @himanshu-kun 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.

pkg/controller/worker/machines_test.go

pkg/controller/worker/machines.go

prashanth26

Thanks for the PR @himanshu-kun . Some suggested changes.

pkg/controller/worker/machines.go

prashanth26

Thanks for the changes. Just minor comments. /lgtm otherwise.

pkg/controller/worker/machines_test.go

pkg/controller/worker/helper.go

pkg/controller/worker/machines_test.go

pkg/controller/worker/helper.go

Co-authored-by: Prashanth <[email protected]> Co-authored-by: Prashanth <[email protected]> Co-authored-by: Prashanth <[email protected]>

not exporting csi driver key constant Co-authored-by: Vladimir Nachev <[email protected]>

removed unit tests, updated constant name to `awsCSIDriverTopologyKey`

himanshu-kun

I have removed the unit tests which I had added earlier in machines_test.go but have tested the changes on local AWS shoot.
For AWS cluster , CSI is enable from k8s>=1.18, so I have tested with local shoot of k8s=1.17.17 and k8s =1.20.2 and both gave the expected result of not adding and adding the csi label respectively.
Hope thats ok @vpnachev @rfranzke

pkg/controller/worker/machines.go

moved comments Co-authored-by: Vladimir Nachev <[email protected]>

Co-authored-by: Vladimir Nachev <[email protected]>

rfranzke

Looks great now, just a unit test is missing as requested by @vpnachev

gardener-robot · 2021-06-26T03:13:12Z

@himanshu-kun You have pull request review with status CHANGES_REQUESTED, please check

gardener-robot · 2021-06-27T03:06:06Z

@himanshu-kun You have pull request review with status CHANGES_REQUESTED, please check

prashanth26 · 2021-06-28T04:14:52Z

/title Setting CSI AWS topology label on machine deployment creation to allow scale-up

prashanth26

Thanks for the changes and tests @himanshu-kun .
/lgtm
/squash

rfranzke

/lgtm

vpnachev

/lgtm

vpnachev · 2021-06-28T11:12:02Z

/needs cherry-pick

vpnachev · 2021-06-28T11:13:34Z

@prashanth26 @himanshu-kun please open a cherry-pick PR to version 1.25 with this change.

You can see more details in https://github.com/gardener/gardener/blob/master/docs/development/process.md#cherry-picks and get some inspirations how to open the PR.

prashanth26 · 2021-06-28T12:31:17Z

@prashanth26 @himanshu-kun please open a cherry-pick PR to version 1.25 with this change.

You can see more details in https://github.com/gardener/gardener/blob/master/docs/development/process.md#cherry-picks and get some inspirations how to open the PR.

@himanshu-kun will get this done. Thanks.

…gy label on machine deployment creation to allow scale-up (#368) * added csi driver label if not present and tests Co-authored-by: Prashanth <[email protected]> Co-authored-by: Prashanth <[email protected]> Co-authored-by: Prashanth <[email protected]> * Update pkg/controller/worker/helper.go not exporting csi driver key constant Co-authored-by: Vladimir Nachev <[email protected]> * checking if CSI enabled before adding csi label removed unit tests, updated constant name to `awsCSIDriverTopologyKey` * used lambda fn to add aws csi label * Update pkg/controller/worker/machines.go moved comments Co-authored-by: Vladimir Nachev <[email protected]> * Update pkg/controller/worker/machines.go Co-authored-by: Vladimir Nachev <[email protected]> * added test case Co-authored-by: Prashanth <[email protected]> Co-authored-by: Vladimir Nachev <[email protected]>

himanshu-kun requested review from a team as code owners June 21, 2021 17:41

himanshu-kun closed this Jun 21, 2021

himanshu-kun changed the title ~~added csi driver label if not present and tests~~ added csi driver label to machineDeployment, if not present and added tests Jun 21, 2021

himanshu-kun reopened this Jun 21, 2021

gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jun 21, 2021

ialidzhikov removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jun 21, 2021

prashanth26 reviewed Jun 22, 2021

View reviewed changes

pkg/controller/worker/machines_test.go Outdated Show resolved Hide resolved

pkg/controller/worker/machines.go Outdated Show resolved Hide resolved

prashanth26 reviewed Jun 22, 2021

View reviewed changes

pkg/controller/worker/machines.go Outdated Show resolved Hide resolved

prashanth26 reviewed Jun 22, 2021

View reviewed changes

pkg/controller/worker/machines_test.go Outdated Show resolved Hide resolved

pkg/controller/worker/helper.go Outdated Show resolved Hide resolved

himanshu-kun force-pushed the awsCsiLabel branch from 5167569 to 8eb133c Compare June 23, 2021 05:34

himanshu-kun commented Jun 23, 2021

View reviewed changes

pkg/controller/worker/machines_test.go Outdated Show resolved Hide resolved

rfranzke requested changes Jun 23, 2021

View reviewed changes

pkg/controller/worker/helper.go Outdated Show resolved Hide resolved

pkg/controller/worker/helper.go Outdated Show resolved Hide resolved

gardener-robot added the needs/changes Needs (more) changes label Jun 23, 2021

vpnachev suggested changes Jun 23, 2021

View reviewed changes

pkg/controller/worker/helper.go Outdated Show resolved Hide resolved

pkg/controller/worker/helper.go Outdated Show resolved Hide resolved

himanshu-kun and others added 3 commits June 24, 2021 00:36

added csi driver label if not present and tests

c344d8c

Co-authored-by: Prashanth <[email protected]> Co-authored-by: Prashanth <[email protected]> Co-authored-by: Prashanth <[email protected]>

Update pkg/controller/worker/helper.go

44de089

not exporting csi driver key constant Co-authored-by: Vladimir Nachev <[email protected]>

checking if CSI enabled before adding csi label

ab15506

removed unit tests, updated constant name to `awsCSIDriverTopologyKey`

himanshu-kun force-pushed the awsCsiLabel branch from 2a9bf58 to ab15506 Compare June 23, 2021 19:10

gardener-robot added size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Jun 23, 2021

himanshu-kun commented Jun 23, 2021

View reviewed changes

vpnachev reviewed Jun 23, 2021

View reviewed changes

pkg/controller/worker/machines.go Outdated Show resolved Hide resolved

used lambda fn to add aws csi label

df1b320

vpnachev suggested changes Jun 24, 2021

View reviewed changes

pkg/controller/worker/machines.go Show resolved Hide resolved

pkg/controller/worker/machines.go Outdated Show resolved Hide resolved

himanshu-kun and others added 2 commits June 24, 2021 19:17

Update pkg/controller/worker/machines.go

5231046

moved comments Co-authored-by: Vladimir Nachev <[email protected]>

Update pkg/controller/worker/machines.go

8d32318

Co-authored-by: Vladimir Nachev <[email protected]>

rfranzke reviewed Jun 25, 2021

View reviewed changes

added test case

9ae49f7

gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Jun 28, 2021

gardener-robot changed the title ~~added csi driver label to machineDeployment, if not present and added tests~~ Setting CSI AWS topology label on machine deployment creation to allow scale-up Jun 28, 2021

prashanth26 approved these changes Jun 28, 2021

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging merge/squash Should be merged via 'Squash and merge' and removed needs/changes Needs (more) changes needs/review Needs review labels Jun 28, 2021

vpnachev 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 Jun 28, 2021

gardener-robot-ci-1 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 Jun 28, 2021

rfranzke approved these changes Jun 28, 2021

View reviewed changes

vpnachev approved these changes Jun 28, 2021

View reviewed changes

rfranzke merged commit e5ac1c5 into gardener:master Jun 28, 2021

gardener-robot added the needs/cherry-pick Needs to be cherry-picked to older version label Jun 28, 2021

himanshu-kun mentioned this pull request Jun 29, 2021

[release-v1.25] Automated cherry pick of #365: Setting CSI AWS topology label on machine deployment creation to allow scale-up #368

Merged

prashanth26 mentioned this pull request Jul 21, 2021

Scale from zero fails to work on k8s >= 1.18 AWS gardener/autoscaler#83

Closed

himanshu-kun mentioned this pull request Jan 21, 2022

Scale from zero fails to work on k8s >= 1.21 Azure gardener/autoscaler#113

Closed

elankath mentioned this pull request Oct 17, 2023

Scale from Zero doesn't work in case of pods with PVs gardener/autoscaler#245

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting CSI AWS topology label on machine deployment creation to allow scale-up #365

Setting CSI AWS topology label on machine deployment creation to allow scale-up #365

himanshu-kun commented Jun 21, 2021 •

edited by vpnachev

Loading

gardener-robot-ci-3 commented Jun 21, 2021

prashanth26 left a comment

prashanth26 left a comment •

edited

Loading

himanshu-kun left a comment •

edited

Loading

rfranzke left a comment

gardener-robot commented Jun 26, 2021

gardener-robot commented Jun 27, 2021

prashanth26 commented Jun 28, 2021

prashanth26 left a comment •

edited

Loading

rfranzke left a comment

vpnachev left a comment

vpnachev commented Jun 28, 2021

vpnachev commented Jun 28, 2021

prashanth26 commented Jun 28, 2021 •

edited

Loading

Setting CSI AWS topology label on machine deployment creation to allow scale-up #365

Setting CSI AWS topology label on machine deployment creation to allow scale-up #365

Conversation

himanshu-kun commented Jun 21, 2021 • edited by vpnachev Loading

gardener-robot-ci-3 commented Jun 21, 2021

prashanth26 left a comment

Choose a reason for hiding this comment

prashanth26 left a comment • edited Loading

Choose a reason for hiding this comment

himanshu-kun left a comment • edited Loading

Choose a reason for hiding this comment

rfranzke left a comment

Choose a reason for hiding this comment

gardener-robot commented Jun 26, 2021

gardener-robot commented Jun 27, 2021

prashanth26 commented Jun 28, 2021

prashanth26 left a comment • edited Loading

Choose a reason for hiding this comment

rfranzke left a comment

Choose a reason for hiding this comment

vpnachev left a comment

Choose a reason for hiding this comment

vpnachev commented Jun 28, 2021

vpnachev commented Jun 28, 2021

prashanth26 commented Jun 28, 2021 • edited Loading

himanshu-kun commented Jun 21, 2021 •

edited by vpnachev

Loading

prashanth26 left a comment •

edited

Loading

himanshu-kun left a comment •

edited

Loading

prashanth26 left a comment •

edited

Loading

prashanth26 commented Jun 28, 2021 •

edited

Loading