Skip to content

AKS template cleanup #2143

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

Merged

Conversation

CecileRobertMichon
Copy link
Contributor

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

  • remove the multi-tenancy AKS flavor: the default for all clusters is to use multi-tenancy (AzureClusterIdentity), so the separate flavor is no longer relevant. In fact, the "aks" flavor is already using multi-tenancy.
  • added a patch for the third agentpool to the test dir since it only meant for testing and should not be part of the AKS flavor we publish as a release asset
  • removed the orphaned AAD AKS flavor which has no flavor directory (and is only showing a specific feature, since we want to get away from the "one feature, one flavor" model ref [docs/UX] should we reduce the number of flavors #1456)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Removed aks-multi-tenancy flavor and the aks-aad flavors

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2022
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Mar 3, 2022
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2022
@CecileRobertMichon
Copy link
Contributor Author

/assign @jackfrancis

@CecileRobertMichon
Copy link
Contributor Author

/retest

flake is known #2120

@CecileRobertMichon
Copy link
Contributor Author

same issue

/retest

kind: AzureClusterIdentity
metadata:
labels:
clusterctl.cluster.x-k8s.io/move-hierarchy: "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the significance of this additional label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's what tells clusterctl to include the AzureClusterIdentity when moving a CAPZ cluster from one management cluster to another (clusterctl move). It's present in the other templates and was missing in this one https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/templates/cluster-template.yaml#L217

Copy link
Contributor

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2022
@CecileRobertMichon
Copy link
Contributor Author

/hold

for #1145

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2022
@CecileRobertMichon
Copy link
Contributor Author

/hold cancel

rebased on top of #2095

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2022
Copy link
Contributor

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2022
@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-azure-ci-entrypoint 053d4db link false /test pull-cluster-api-provider-azure-ci-entrypoint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@CecileRobertMichon
Copy link
Contributor Author

/retest

"error"="failed to reconcile cluster services: failed to reconcile route table: failed to create resource capz-qadgta/capz-qadgta-node-routetable (service: routetables): network.RouteTablesClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code=\"ResourceGroupBeingDeleted\" 

Looks like we had either a naming collision or something/someone deleted the RG out of band during the test

@CecileRobertMichon
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2022
@k8s-ci-robot k8s-ci-robot merged commit 8b5aee1 into kubernetes-sigs:main Mar 7, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.3 milestone Mar 7, 2022
@CecileRobertMichon CecileRobertMichon deleted the aks-template-cleanup branch February 17, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants