[AKS] Adds ignore-pod-disruption-budget flag to aks nodepool delete command #30196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
October 23, 2024 07:42
tonychen15
requested review from
zhoxing-ms,
jsntcy and
yanzhudd
as code owners
❌AzureCLI-FullTest
|
azure-client-tools-bot-prd
bot
added this to the November 2024 (2024-11-19)❗ Breaking change release ❗ milestone
|
Hi @tonychen15, |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| aks nodepool delete | cmd aks nodepool delete added parameter ignore_pod_disruption_budget |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
yonzhan and
FumingZhang
microsoft-github-policy-service
bot
added
Auto-Assign
tonychen15
changed the title
tonychen15
changed the title
tonychen15
changed the title
FumingZhang
reviewed
Oct 28, 2024
Comment on lines
1507
to
1519
| def get_ignore_pod_disruption_budget(self) -> bool: | ||
| return self._get_ignore_pod_disruption_budget() | ||
|
|
||
| def _get_ignore_pod_disruption_budget(self) -> bool: | ||
| """Obtain the value of ignore_pod_disruption_budget, default value is False. | ||
|
|
||
| :return: bool | ||
| """ | ||
| # read the original value passed by the command | ||
| ignore_pod_disruption_budget = self.raw_param.get("ignore_pod_disruption_budget", False) | ||
|
|
||
| # This parameter does not need dynamic completion. | ||
| return ignore_pod_disruption_budget |
There was a problem hiding this comment.
I think you don't need this as this is not an option for az aks nodepool add/update
There was a problem hiding this comment.
I see. I overlooked your comment in the code. Removed.
Comment on lines
1497
to
1508
| def common_get_ignore_pod_disruption_budget(self): | ||
| ctx_1 = AKSAgentPoolContext( | ||
| self.cmd, | ||
| AKSAgentPoolParamDict({ | ||
| "ignore_pod_disruption_budget": True, | ||
| }), | ||
| self.models, | ||
| DecoratorMode.DELETE, | ||
| self.agentpool_decorator_mode, | ||
| ) | ||
| self.assertEqual(ctx_1.get_ignore_pod_disruption_budget(), True) | ||
|
|
There was a problem hiding this comment.
And there's no need for the test here.
There was a problem hiding this comment.
Removed, thanks.
| with self.argument_context("aks nodepool delete") as c: | ||
| c.argument( | ||
| "ignore_pod_disruption_budget", | ||
| options_list=["--ignore-pod-disruption-budget"], |
There was a problem hiding this comment.
If the only option form is --ignore-pod-disruption-budget, you could ignore this line. In cli-extensions/aks-preview, it has another shorthand option name -i, do you want to keep it?
There was a problem hiding this comment.
Got it. I intentionally remove that to make sure CX fully understand what they're doing.
There was a problem hiding this comment.
Need to add ignore-pdb as ignore-pod-disruption-budget exceeds the maximum length 22.
tonychen15
force-pushed
the
dev-ipdb
branch
3 times, most recently
from
dc906f7 to
e48de17
Compare
|
It seems both Python39 and Python312 failed at test case aks_agentpool_create_scale_delete with version 2020-09-01 (https://dev.azure.com/azclitools/public/_build/results?buildId=201476&view=logs&j=4e4d232d-230a-55c9-0730-4c1a9c36b9e5&t=6d1bdb99-245b-5117-da18-01d8df6f72c0&l=1356). Is this failed test case a known issue? |
FumingZhang
reviewed
Nov 11, 2024
| with self.argument_context("aks nodepool delete") as c: | ||
| c.argument( | ||
| "ignore_pod_disruption_budget", | ||
| options_list=["--ignore-pdb"], |
There was a problem hiding this comment.
Suggested change
| options_list=["--ignore-pdb"], | |
| options_list=["--ignore-pdb", "--ignore-pod-disruption-budget"], |
please also include the default option name
There was a problem hiding this comment.
Added back, hope that pylint not to report length too long error.
|
Requeued live test. The following cases failed in replay mode as you've changed the behavior of
There's a PR #30290 that bumped the default API version merged earlier today. Please rebase from dev branch, requeue the tests and them commit the updated recording files. |
FumingZhang
reviewed
Nov 11, 2024
| @@ -1614,6 +1614,14 @@ | |||
| helps['aks nodepool delete'] = """ | |||
| type: command | |||
| short-summary: Delete the agent pool in the managed Kubernetes cluster. | |||
| parameters: | |||
| - name: --ignore-pdb | |||
There was a problem hiding this comment.
- FAIL - HIGH severity: unrecognized_help_parameter_rule
Help-Entry:aks nodepool delete- The following parameter help names are invalid: --ignore-pdb
Try the following, one of them would be correct
Suggested change
| - name: --ignore-pdb | |
| - name: --ignore-pdb --ignore-pod-disruption-budget |
Suggested change
| - name: --ignore-pdb | |
| - name: --ignore-pod-disruption-budget --ignore-pdb |
added 2 commits
November 11, 2024 08:46
zhoxing-ms
reviewed
Nov 11, 2024
zhoxing-ms
reviewed
Nov 11, 2024
Comment on lines
+2608
to
+2609
| active_cloud = get_active_cloud(cmd.cli_ctx) | ||
| if active_cloud.profile != "latest": |
There was a problem hiding this comment.
Will this logic also be used in old profiles env such as Azure Stack?
There was a problem hiding this comment.
No, this feature is only used in AKS. @zhoxing-ms
zhoxing-ms
modified the milestones:
November 2024 (2024-11-19)❗ Breaking change release ❗,
December 2024 (2024-12-10)
|
@FumingZhang and @zhoxing-ms I have rebased to the latest dev branch, and updated per your comments, somehow that full test is still failed. As ignore-pdb is not a required parameter to "az aks nodepool delete", so it should not have the impact to other aks cli command. I'm quite confused about why those aks tests failed for it. Any more suggestions on this? Thanks. |
FumingZhang
reviewed
Nov 12, 2024
|
Failures in Automation Full Test XXX are not related to your changes, I think the corresponding team is working on the fix. As for failures in linter, may try the other way https://github.com/Azure/azure-cli/pull/30196/files#r1837556755
|
Co-authored-by: FumingZhang <[email protected]>
So, we will wait for another team's fix, then merge this PR? If so, I will retest it later. |
@FumingZhang @zhoxing-ms any way we can move this PR forward? |
|
@tonychen15 please fix the failed CI check. Take Azure.azure-cli Full Test (Automation Full Test Python312 Profile Latest instance6) as an example, the following cases failed
and the error is
As I've mentioned
I just requeued the live test for you. Please commit the updated recording files once those tests passed. |
|
I see. I need to manually rerun those failed test cases locally and upload their logs. I thought the pipeline may rerun them on the server side. |
added 2 commits
November 25, 2024 08:29
|
I noticed that you did not upload the new recording files corresponding to the above-mentioned test cases. The live test I queued passed, and you could find them from pipeline artifacts. 
|
zhoxing-ms
modified the milestones:
January 2025 (2025-01-14),
February 2025 (2025-02-11)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related command
az aks nodepool delete
Description
Add
--ignore-pod-disruption-budgetoption for ignoring PodDisruptionBudget. This will expedite the nodepool deletion which may be delayed or blocked by PDB.Testing Guide
Use the command
az aks nodepool delete --ignore-pod-disruption-budgetto send the request to aks.History Notes
[AKS]
az aks nodepool delete: Add--ignore-pod-disruption-budgetoption for ignoring PodDisruptionBudgetThis 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.