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

[AKS] az aks nodepool add/update/upgrade: Add new parameter --drain-timout to slow down the upgrade #27475

Merged
merged 11 commits into from
Oct 10, 2023

Conversation

paulgmiller
Copy link
Member

@paulgmiller paulgmiller commented Sep 26, 2023

Related command
az aks nodepool add|update|upgrade

Description
This allows the customer to cofigure time we wait for a node to drain before failing an upgrade operation

This became available in the 07-01 api.

Testing Guide
Same tests as extension create and update with nodeos upgrade channel. Make sure its preserved. Combine with upgrade chennel

History Notes
[AKS] az aks nodepool add/update/upgrade: Add new parameter --drain-timout to slow down the upgrade


This checklist is used to make sure that common guidelines for a pull request are followed.

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Sep 26, 2023

🔄AzureCLI-FullTest
️✔️acr
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️acs
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️advisor
️✔️latest
️✔️3.11
️✔️3.9
️✔️ams
️✔️latest
️✔️3.11
️✔️3.9
️✔️apim
️✔️latest
️✔️3.11
️✔️3.9
️✔️appconfig
️✔️latest
️✔️3.11
️✔️3.9
️✔️appservice
️✔️latest
️✔️3.11
️✔️3.9
️✔️aro
️✔️latest
️✔️3.11
️✔️3.9
️✔️backup
️✔️latest
️✔️3.11
️✔️3.9
️✔️batch
️✔️latest
️✔️3.11
️✔️3.9
️✔️batchai
️✔️latest
️✔️3.11
️✔️3.9
️✔️billing
️✔️latest
️✔️3.11
️✔️3.9
️✔️botservice
️✔️latest
️✔️3.11
️✔️3.9
️✔️cdn
️✔️latest
️✔️3.11
️✔️3.9
️✔️cloud
️✔️latest
️✔️3.11
️✔️3.9
️✔️cognitiveservices
️✔️latest
️✔️3.11
️✔️3.9
️✔️config
️✔️latest
️✔️3.11
️✔️3.9
️✔️configure
️✔️latest
️✔️3.11
️✔️3.9
️✔️consumption
️✔️latest
️✔️3.11
️✔️3.9
️✔️container
️✔️latest
️✔️3.11
️✔️3.9
🔄containerapp
🔄latest
️✔️3.11
🔄3.9
️✔️core
️✔️2018-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️cosmosdb
️✔️latest
️✔️3.11
️✔️3.9
️✔️databoxedge
️✔️2019-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️dla
️✔️latest
️✔️3.11
️✔️3.9
️✔️dls
️✔️latest
️✔️3.11
️✔️3.9
️✔️dms
️✔️latest
️✔️3.11
️✔️3.9
️✔️eventgrid
️✔️latest
️✔️3.11
️✔️3.9
️✔️eventhubs
️✔️latest
️✔️3.11
️✔️3.9
️✔️feedback
️✔️latest
️✔️3.11
️✔️3.9
️✔️find
️✔️latest
️✔️3.11
️✔️3.9
️✔️hdinsight
️✔️latest
️✔️3.11
️✔️3.9
️✔️identity
️✔️latest
️✔️3.11
️✔️3.9
️✔️iot
️✔️2019-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️keyvault
️✔️2018-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️kusto
️✔️latest
️✔️3.11
️✔️3.9
️✔️lab
️✔️latest
️✔️3.11
️✔️3.9
️✔️managedservices
️✔️latest
️✔️3.11
️✔️3.9
️✔️maps
️✔️latest
️✔️3.11
️✔️3.9
️✔️marketplaceordering
️✔️latest
️✔️3.11
️✔️3.9
️✔️monitor
️✔️latest
️✔️3.11
️✔️3.9
️✔️mysql
️✔️latest
️✔️3.11
️✔️3.9
️✔️netappfiles
️✔️latest
️✔️3.11
️✔️3.9
️✔️network
️✔️2018-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️policyinsights
️✔️latest
️✔️3.11
️✔️3.9
️✔️privatedns
️✔️latest
️✔️3.11
️✔️3.9
️✔️profile
️✔️latest
️✔️3.11
️✔️3.9
️✔️rdbms
️✔️latest
️✔️3.11
️✔️3.9
️✔️redis
️✔️latest
️✔️3.11
️✔️3.9
️✔️relay
️✔️latest
️✔️3.11
️✔️3.9
️✔️resource
️✔️2018-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️role
️✔️latest
️✔️3.11
️✔️3.9
️✔️search
️✔️latest
️✔️3.11
️✔️3.9
️✔️security
️✔️latest
️✔️3.11
️✔️3.9
️✔️servicebus
️✔️latest
️✔️3.11
️✔️3.9
️✔️serviceconnector
️✔️latest
️✔️3.11
️✔️3.9
️✔️servicefabric
️✔️latest
️✔️3.11
️✔️3.9
️✔️signalr
️✔️latest
️✔️3.11
️✔️3.9
️✔️sql
️✔️latest
️✔️3.11
️✔️3.9
️✔️sqlvm
️✔️latest
️✔️3.11
️✔️3.9
️✔️storage
️✔️2018-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️synapse
️✔️latest
️✔️3.11
️✔️3.9
️✔️telemetry
️✔️2018-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9
️✔️util
️✔️latest
️✔️3.11
️✔️3.9
️✔️vm
️✔️2018-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.11
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.11
️✔️3.9
️✔️latest
️✔️3.11
️✔️3.9

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Sep 26, 2023

❌AzureCLI-BreakingChangeTest
❌acs
rule cmd_name rule_message suggest_message
1010 - ParaPropUpdate aks nodepool add cmd aks nodepool add update parameter spot_max_price: updated property default from nan to nan please change property default from nan to nan for parameter spot_max_price of cmd aks nodepool add
⚠️ 1006 - ParaAdd aks nodepool add cmd aks nodepool add added parameter drain_timeout
⚠️ 1006 - ParaAdd aks nodepool update cmd aks nodepool update added parameter drain_timeout
⚠️ 1006 - ParaAdd aks nodepool upgrade cmd aks nodepool upgrade added parameter drain_timeout

@yonzhan
Copy link
Collaborator

yonzhan commented Sep 26, 2023

Thank you for your contribution! We will review the pull request and get back to you soon.

@paulgmiller paulgmiller changed the title first attempt just mimic max-surge [AKS] az aks nodepool add/update/upgreade: Add new parameter --drain-timout to slow down the upgrade Sep 26, 2023
@paulgmiller paulgmiller marked this pull request as ready for review September 26, 2023 22:44
@yonzhan yonzhan requested a review from FumingZhang September 27, 2023 00:28
@paulgmiller paulgmiller changed the title [AKS] az aks nodepool add/update/upgreade: Add new parameter --drain-timout to slow down the upgrade [AKS] az aks nodepool add/update/upgrade: Add new parameter --drain-timout to slow down the upgrade Sep 27, 2023
Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

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

LGTM

Please fix the style issues

/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/_help.py:1485:92: W291 trailing whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/_help.py:1485:92: W291 trailing whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:933:1: W293 blank line contains whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:1491:1: W293 blank line contains whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:1754:58: E261 at least two spaces before inline comment
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:1754:59: E262 inline comment should start with '# '
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:1760:1: W293 blank line contains whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:933:1: W293 blank line contains whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:1491:1: W293 blank line contains whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:1754:58: E261 at least two spaces before inline comment
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:1754:59: E262 inline comment should start with '# '
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/agentpool_decorator.py:1760:1: W293 blank line contains whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/custom.py:2345:5: E303 too many blank lines (2)
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/custom.py:2345:5: E303 too many blank lines (2)

@paulgmiller
Copy link
Member Author

LGTM

Please fix the style issues

fixed (forgot style was seperate from lint. miss gofmt)

@FumingZhang
Copy link
Member

LGTM
Please fix the style issues

fixed (forgot style was seperate from lint. miss gofmt)

@paulgmiller still have some style issues

@paulgmiller
Copy link
Member Author

paulgmiller commented Sep 29, 2023

LGTM
Please fix the style issues

fixed (forgot style was seperate from lint. miss gofmt)

@paulgmiller still have some style issues

Sorry apologize @FumingZhang for the multiple iterations. On my local machine azdev style acs flake8 spits out thosands of errors so have to use the online build to catch these which is slow.

All checks are green now.

@paulgmiller
Copy link
Member Author

@zhoxing-ms are you or @yonzhan able to complete this? @FumingZhang is out for a week but already gave his lgtm and max reviewed from aks side and all checks are passing.

andyliuliming
andyliuliming previously approved these changes Oct 8, 2023
if (
self.agentpool and
self.agentpool.upgrade_settings and
self.agentpool.upgrade_settings.drain_timeout is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

@paulgmiller @FumingZhang ^^^ shouldn't that be

self.agentpool.upgrade_settings.drain_timeout_in_minutes is not None

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sigh I miss static typing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants