-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Add --network-dataplane to az aks update command #6446
Conversation
Hi @wedaly, |
Hi @wedaly, |
AKS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, however, since the command is currently not practically available, do you really want to merge the PR?
Thanks! I think it's okay to merge it ahead of the rollout, but I'm also okay waiting if that's what we usually do. |
92c2e02
to
4d71786
Compare
Updated to include a live test running in eastus. |
@FumingZhang this should be ready for review now, could you please take a look? Thanks! |
|
rule | cmd_name | rule_message | suggest_message |
---|---|---|---|
aks update | cmd aks update added parameter network_dataplane |
Queued live test pipeline to validate the change, test passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, live test validation passed but CI failed, maybe update the recording file (from pipeline artifact) may fix the issue as I couldn't check detailed error message at this moment.
4d71786
to
810feb5
Compare
rebased onto main and used test_aks_migrate_cluster_to_cilium_dataplane.yaml downloaded from the live test pipeline artifacts. Let's see if CI passes... |
hmm, still failing. The errors I'm seeing in the build output are:
I think these test cases were expecting These failures look unrelated to my changes? Not sure how to fix them, would appreciate any advice. |
Yeah, some recordings are broken by API version bump of some dependent commands. Fixed in PR #6557. Please rebase from main once the PR is merged. |
810feb5
to
9de5943
Compare
@zhoxing-ms this PR is ready for review, could you please take a look? Thanks! |
@FumingZhang Could you please help review this PR again? |
The change looks good to me |
@zhoxing-ms @FumingZhang would either of you be able to merge the PR? I don't have permission to merge it. Thanks! |
--network-dataplane
to az aks update command
--network-dataplane
to az aks update command
Add the
--network-dataplane
flag to theaz aks update
command. This will allow customers to update an existing cluster to enable Azure CNI Powered by CiliumThis checklist is used to make sure that common guidelines for a pull request are followed.
Related command
az aks update --network-dataplane=cilium
General Guidelines
azdev style <YOUR_EXT>
locally? (pip install azdev
required)python scripts/ci/test_index.py -q
locally?For new extensions:
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
.