add AKS adoption e2e test and docs #4697

nojnhuh · 2024-04-01T18:08:06Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR tests and documents adopting an existing AKS cluster under management by CAPZ using features that already exist in CAPZ.

Thank you to @mtougeron for blazing this trail!

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 #1173

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Added documentation describing how to adopt an existing AKS cluster.

codecov · 2024-04-01T18:19:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.35%. Comparing base (f71c5ed) to head (a982815).
Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4697   +/-   ##
=======================================
  Coverage   62.35%   62.35%           
=======================================
  Files         196      196           
  Lines       16192    16192           
=======================================
  Hits        10097    10097           
  Misses       5362     5362           
  Partials      733      733

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

willie-yao

This will be great to test and document! I left some comments regarding the test, and I'll work on following this doc to see if I can adopt an existing cluster using those steps.

willie-yao · 2024-04-03T21:46:00Z

test/e2e/aks_adopt.go

+	updateResource := []any{"30s", "5s"}
+
+	waitForNoBlockMove := func(obj client.Object) {
+		waitForBlockMoveGone := []any{"30s", "5s"}


Should these intervals be moved to the intervals section in azure-dev? https://github.com/willie-yao/cluster-api-provider-azure/blob/main/test/e2e/config/azure-dev.yaml#L231

These values are pretty specific to this test only, so to me it seems like it's fine to keep them local to this file. Otherwise I always feel like I have a hard time tracking down what these values are.

willie-yao · 2024-04-03T21:48:29Z

test/e2e/aks_adopt.go

+	removeFinalizers(cluster)
+	shouldNotExist(cluster)
+
+	clusterctl.ApplyClusterTemplateAndWait(ctx, input.ApplyInput, input.ApplyResult)


I'm a bit confused on how this test is laid out. Do we orphan this cluster and then adopt it back as a CAPZ cluster? I'm a bit confused where the adoption occurs since the logic here seems to be only related to orphaning the cluster.

It could help to add a code comment to separate the logic between orphaning/adopting the cluster.

Do we orphan this cluster and then adopt it back as a CAPZ cluster?

Yes, that's how I tried to describe it in the doc comment for this function.

I'm a bit confused where the adoption occurs since the logic here seems to be only related to orphaning the cluster.

The ApplyAndWait at the very end is the adoption. "Adoption" isn't really its own separate thing, it's only applying templates that happen to match existing resources.

willie-yao · 2024-04-04T19:35:46Z

docs/book/src/topics/managedcluster.md

+</aside>
+
+CAPZ can adopt some AKS clusters created by other means under its management. This works by crafting CAPI and
+CAPZ manifests which describe the existing cluster and creating those resources on the CAPI management


Is there an example of how this is done? Do we just need to match the metadata of the existing cluster and make sure to add the tag? It could help to give an example of what a manually created CAPZ manifest would look like for a default AKS cluster.

I'm a bit hesitant to put a full example in the docs if we can't automatically verify it stays correct over time. Definitely could add one anyway, but I'd be interested in getting more user feedback that indicates a full example is worthwhile here before committing to maintaining one.

docs/book/src/topics/managedcluster.md

test/e2e/aks_adopt.go

nojnhuh · 2024-04-17T17:11:12Z

/hold for squash

willie-yao

/lgtm

k8s-ci-robot · 2024-04-25T17:53:28Z

LGTM label has been added.

Git tree hash: b3f940811a807b13fc885b023b32d9543d11f6fa

mboersma

/lgtm
/approve

k8s-ci-robot · 2024-04-25T19:10:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

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:

~~OWNERS~~ [mboersma]

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

mboersma · 2024-04-25T19:22:05Z

@nojnhuh squash away!

Co-authored-by: Mike Tougeron <[email protected]>

nojnhuh · 2024-04-25T19:50:41Z

squashed!
/hold cancel

Had to rebase to pickup the broken link fix, didn't hit any conflicts though.

mboersma

/lgtm

k8s-ci-robot · 2024-04-25T19:51:41Z

LGTM label has been added.

Git tree hash: 26e56c326d31b3e629cfd8b49d66c61a8ebbdd8f

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 1, 2024

k8s-ci-robot requested review from jackfrancis and Jont828 April 1, 2024 18:08

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 1, 2024

nojnhuh force-pushed the aks-adopt branch from 2ad6b86 to 5f7bc1d Compare April 1, 2024 19:45

willie-yao reviewed Apr 3, 2024

View reviewed changes

dtzar added this to the v1.15 milestone Apr 4, 2024

willie-yao reviewed Apr 4, 2024

View reviewed changes

mboersma reviewed Apr 17, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2024

willie-yao approved these changes Apr 25, 2024

View reviewed changes

k8s-ci-robot assigned willie-yao Apr 25, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2024

mboersma approved these changes Apr 25, 2024

View reviewed changes

k8s-ci-robot assigned mboersma Apr 25, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2024

add AKS adoption e2e test and docs

a982815

Co-authored-by: Mike Tougeron <[email protected]>

nojnhuh force-pushed the aks-adopt branch from 4a21971 to a982815 Compare April 25, 2024 19:50

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2024

k8s-ci-robot requested review from mboersma and willie-yao April 25, 2024 19:50

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2024

mboersma approved these changes Apr 25, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2024

k8s-ci-robot merged commit cdf5992 into kubernetes-sigs:main Apr 26, 2024
20 checks passed

nojnhuh deleted the aks-adopt branch April 26, 2024 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add AKS adoption e2e test and docs #4697

add AKS adoption e2e test and docs #4697

nojnhuh commented Apr 1, 2024

codecov bot commented Apr 1, 2024 •

edited

Loading

willie-yao left a comment

willie-yao Apr 3, 2024

nojnhuh Apr 5, 2024

willie-yao Apr 3, 2024

willie-yao Apr 3, 2024

nojnhuh Apr 5, 2024

willie-yao Apr 4, 2024

nojnhuh Apr 5, 2024

nojnhuh commented Apr 17, 2024

willie-yao left a comment

k8s-ci-robot commented Apr 25, 2024

mboersma left a comment

k8s-ci-robot commented Apr 25, 2024

mboersma commented Apr 25, 2024

nojnhuh commented Apr 25, 2024

mboersma left a comment

k8s-ci-robot commented Apr 25, 2024

add AKS adoption e2e test and docs #4697

add AKS adoption e2e test and docs #4697

Conversation

nojnhuh commented Apr 1, 2024

codecov bot commented Apr 1, 2024 • edited Loading

Codecov Report

willie-yao left a comment

Choose a reason for hiding this comment

willie-yao Apr 3, 2024

Choose a reason for hiding this comment

nojnhuh Apr 5, 2024

Choose a reason for hiding this comment

willie-yao Apr 3, 2024

Choose a reason for hiding this comment

willie-yao Apr 3, 2024

Choose a reason for hiding this comment

nojnhuh Apr 5, 2024

Choose a reason for hiding this comment

willie-yao Apr 4, 2024

Choose a reason for hiding this comment

nojnhuh Apr 5, 2024

Choose a reason for hiding this comment

nojnhuh commented Apr 17, 2024

willie-yao left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 25, 2024

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 25, 2024

mboersma commented Apr 25, 2024

nojnhuh commented Apr 25, 2024

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 25, 2024

codecov bot commented Apr 1, 2024 •

edited

Loading