[AKS] az aks nodepool upgrade: Fix --max-unavailable being silently ignored - #33215
Julie Zhu (yanzhudd) merged 2 commits into
Conversation
…ignored The `--max-unavailable` flag was accepted but never applied to the agent pool upgrade settings before the PUT request, causing it to be silently dropped. Add the missing assignment. Fixes Azure#33195 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
There was a problem hiding this comment.
Pull request overview
Fixes az aks nodepool upgrade --max-unavailable being accepted/validated but not applied to the AgentPool PUT payload, so the setting was silently dropped.
Changes:
- Assign
max_unavailableintoinstance.upgrade_settings.max_unavailableduring nodepool upgrade.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
FumingZhang
left a comment
There was a problem hiding this comment.
lgtm, could you add a unit test case for the change? Also, please remember to update the PR title so it passes the CI checks.
--max-unavailable being silently ignored
--max-unavailable being silently ignored--max-unavailable being silently ignored
--max-unavailable being silently ignoredaz aks nodepool upgrade --max-unavailable being silently ignored
Aditya Pujara (a0x1ab)
left a comment
There was a problem hiding this comment.
🤖 AI Agent | Automated Quality Gate
✅ Test Results — Issue #33195 / PR #33215
Test environment: Local unit test run on PR branch fix/nodepool-upgrade-max-unavailable
| Test Suite | Result |
|---|---|
test_custom.py::AksAgentpoolUpgradeTest::test_aks_agentpool_upgrade_sets_max_unavailable |
✅ PASSED |
test_custom.py (full suite) |
✅ 38 passed, 1 skipped |
Validation: The new test confirms that max_unavailable is correctly assigned to instance.upgrade_settings.max_unavailable during aks_agentpool_upgrade(). The silent-drop bug is fixed.
Minor observation (non-blocking): The condition if max_unavailable: is consistent with how max_surge is handled in the same function and is fine for string CLI args (e.g. "0%" or "1" are truthy). No change needed.
No regressions detected.
Note: Azure VM provisioning was unavailable in this environment; live E2E tests against a real AKS cluster could not be run. Unit test coverage of the changed code path is complete.>
Aditya Pujara (a0x1ab)
left a comment
There was a problem hiding this comment.
Review: PR #33215 — Fix az aks nodepool upgrade --max-unavailable being silently ignored
✅ CI Checks — All Passed
| Check | Result |
|---|---|
| license/cla | ✅ success |
| azdev-linter | ✅ success |
| azdev-style | ✅ success |
🔍 Code Review
Fix in custom.py (correct):
The root cause was that max_unavailable was accepted and validated but never assigned to instance.upgrade_settings before the PUT request. The one-line fix is minimal, targeted, and follows the exact same guard pattern (if param:) used by the adjacent node_soak_duration and undrainable_node_behavior assignments.
Test in test_custom.py (adequate):
AksAgentpoolUpgradeTest.test_aks_agentpool_upgrade_sets_max_unavailable correctly:
- Constructs a real
AgentPoolUpgradeSettingsobject (not a plain mock) so the attribute assignment is meaningful. - Mocks
client.getandsdk_no_waitto avoid network calls. - Asserts
instance.upgrade_settings.max_unavailable == 5directly after the call.
Minor note (non-blocking): if max_unavailable: will skip assignment when the value is integer 0. In practice --max-unavailable is always passed as a string (e.g. 0, 33%), so this is consistent with the rest of the function and is not a regression.
🧪 Live Tests
Live-test workflow on Azure/issue-sentinel was not reachable (workflow not found). Unit test coverage is sufficient for this change given its simplicity.
✅ Verdict
The fix is correct, minimal, consistent with surrounding code, and well-tested. No changes requested. Ready for human maintainer review.
Posted by agent-assist (autonomous bug-fix pipeline).
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Jenniferyingni please fix failed CI check |
az aks nodepool upgrade --max-unavailable being silently ignoredaz aks nodepool upgrade --max-unavailable`` being silently ignored
az aks nodepool upgrade --max-unavailable`` being silently ignored--max-unavailable" being silently ignored
|
/azp run |
|
Commenter does not have sufficient privileges for PR 33215 in repo Azure/azure-cli |
|
FumingZhang fixed the PR title, please rerun the command, thanks! |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
--max-unavailable" being silently ignoredaz aks nodepool upgrade: Fix --max-unavailable being silently ignored
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
az aks nodepool upgrade --max-unavailablebeing silently ignored--max-unavailableparameter was accepted and validated but never assigned to the agent pool'supgrade_settingsbefore the PUT requestinstance.upgrade_settings.max_unavailable = max_unavailableFixes #33195
Related command
Description
Testing Guide
Run "az aks nodepool upgrade
--max-unavailable" and verify the value is appliedHistory Notes
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.