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 --node-soak-duration #6865

Merged
merged 23 commits into from
Nov 7, 2023

Conversation

MaxHorstmann
Copy link
Contributor

@MaxHorstmann MaxHorstmann commented Oct 17, 2023

Related command

az aks nodepool add/update/upgrade --node-soak-duration

nodeSoakDuration was added to 2023-09-02-preview

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

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

azure-client-tools-bot-prd bot commented Oct 17, 2023

❌Azure CLI Extensions Breaking Change Test
❌aks-preview
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 add cmd aks nodepool add added parameter node_soak_duration
⚠️ 1006 - ParaAdd aks nodepool update cmd aks nodepool update added parameter drain_timeout
⚠️ 1006 - ParaAdd aks nodepool update cmd aks nodepool update added parameter node_soak_duration
⚠️ 1006 - ParaAdd aks nodepool upgrade cmd aks nodepool upgrade added parameter drain_timeout
⚠️ 1006 - ParaAdd aks nodepool upgrade cmd aks nodepool upgrade added parameter node_soak_duration

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

Hi @MaxHorstmann,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Collaborator

yonzhan commented Oct 17, 2023

AKS

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 also add a live test (e2e test) case corresponding to the newly added option, may refer to examples in test_aks_commands.py. Don't forget to commit the recording file for the new test case. Get the recording file by running the test case in live mode locally or rerunning the pipeline (set pipeline line variable EXT_TEST_COVERAGE to your test case name, if there are multiple, separate them with spaces) once test passed, recording file will be uploaded to pipeline artifact.

Comment on lines 1225 to 1227
'Conflicting flags. Unable to specify max-surge/drain-timeout/node-soak-duration with node-image-only.'
'If you want to use max-surge/drain-timeout/node-soak-duration with a node image upgrade, please first '
'update max-surge/drain-timeout/node-soak-duration using "az aks nodepool update --max-surge/--drain-timeout/--node-soak-duration".'
Copy link
Member

Choose a reason for hiding this comment

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

This error message should be the description of the exception on line 1232?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! Fixed

)

# Note: we exclude this option because node image upgrade can't accept nodepool put fields like max surge
if max_surge and node_image_only:
if (max_surge or drain_timeout or node_soak_duration) and node_image_only::
Copy link
Member

Choose a reason for hiding this comment

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

extra trailing colon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1837,6 +1837,45 @@ def test_aks_create_add_nodepool_with_custom_ca_trust_certificates(self, resourc
self.cmd(
'aks delete -g {resource_group} -n {name} --yes --no-wait', checks=[self.is_empty()])

@AllowLargeResponse()
@AKSCustomResourceGroupPreparer(random_name_length=17, name_prefix='clitest', location='westus2')
def test_aks_nodepool_node_soak_duration(self, resource_group, resource_group_location):
Copy link
Member

Choose a reason for hiding this comment

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

Queued live test to validate the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @FumingZhang! Checking why it failed.

Btw

echo 'Live test failed!'
echo 'Please refer to this wiki (https://dev.azure.com/msazure/CloudNativeCompute/_wiki/wikis/CloudNativeCompute.wiki/156735/AZCLI-AKS-Live-Unit-Test-Pipeline) for troubleshooting guidelines.'

Looks like that wiki link is outdated. I updated the WIKI_LINK pipeline variable here, was that the correct url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FumingZhang so, it looks like the live test is currently failing

Query 'upgradeSettings.nodeSoakDurationInMinutes' doesn't yield expected value '5', instead the actual value is 'None'

I noticed that your 2023-09-02-preview branch (which contains the Swagger change for this feature) in Azure/azure-rest-api-specs hasn't been merged yet - I guess that explains it? Let's re-run the test after the Swagger change is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, swagger/sdk has not been updated, so the deserialized response will not include this property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FumingZhang I saw that you merged your 2023-09-02-preview Swagger PR Azure/azure-rest-api-specs#26313

so I synced this with the Azure/azure-cli-extensions upstream in order to pull in the SDK update

and queued up another live test

am I doing this right? :)

Copy link
Member

Choose a reason for hiding this comment

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

SDK not updated yet, rebase/merge from main once this PR #6904 got merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx - looks like that PR just got merged.

synced/merged this branch.

queued up new live test

Copy link
Member

Choose a reason for hiding this comment

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

Seems live test passed, please commit the recording files corresponding to the newly added test cases (find from pipeline artifact).

Please also fix errors in Static Analysis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems live test passed, please commit the recording files corresponding to the newly added test cases (find from pipeline artifact)

done

Please also fix errors in Static Analysis.

done

@MaxHorstmann MaxHorstmann marked this pull request as ready for review October 27, 2023 13:33
@MaxHorstmann
Copy link
Contributor Author

@FumingZhang any idea why some of these checks failed? The only error message I see is

Bash exited with code '1'.

@FumingZhang
Copy link
Member

@FumingZhang any idea why some of these checks failed? The only error message I see is

Bash exited with code '1'.

The failure is about test case

FAILED src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py::AzureKubernetesServiceScenarioTest::test_aks_nodepool_drain_timeout

and seems the recording file corresponding to this case is not committed.

Queue new live test for this test case.

@MaxHorstmann
Copy link
Contributor Author

and seems the recording file corresponding to this case is not committed.

Queue new live test for this test case.

Thanks @FumingZhang, I added the recording. Let's see if all the checks pass now.

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

@MaxHorstmann
Copy link
Contributor Author

Thanks @FumingZhang! Anything else I need to do to get this merged?

@yonzhan
Copy link
Collaborator

yonzhan commented Nov 6, 2023

Please fix conflicting files.

@MaxHorstmann
Copy link
Contributor Author

@yonzhan @FumingZhang

Please fix conflicting files.

done

Comment on lines +16 to +19
0.5.169
+++++++
* Add `--node-soak-duration` to the `az aks nodepool add/update/upgrade` commands.
* Add `--drain-timeout` to the `az aks nodepool add/update/upgrade` commands (already in [azure-cli](https://github.com/Azure/azure-cli/pull/27475)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as #6881 (comment)

@zhoxing-ms zhoxing-ms merged commit b46a12d into Azure:main Nov 7, 2023
14 checks passed
@azclibot
Copy link
Collaborator

azclibot commented Nov 7, 2023

[Release] Update index.json for extension [ aks-preview ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=104441&view=results

@MaxHorstmann MaxHorstmann deleted the maxhorstmann/node-soak-duration branch November 7, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AKS Auto-Assign Auto assign by bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants