Expose AKS preview features #4617

willie-yao · 2024-03-01T21:52:26Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR enables users to configure AKS managed clusters with preview features to take advantage of the latest and greatest functionality available in AKS.

Big thanks to @nojnhuh for laying out the groundwork for this feature!

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

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Expose AKS preview features

k8s-ci-robot · 2024-03-01T21:52:30Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2024-03-04T17:43:11Z

Codecov Report

Attention: Patch coverage is 60.41667% with 76 lines in your changes are missing coverage. Please review.

Project coverage is 62.73%. Comparing base (2251753) to head (ae1e38d).
Report is 2 commits behind head on main.

Files	Patch %	Lines
azure/services/managedclusters/spec.go	37.70%	33 Missing and 5 partials ⚠️
azure/services/agentpools/spec.go	54.54%	11 Missing and 4 partials ⚠️
azure/services/agentpools/agentpools.go	58.82%	5 Missing and 2 partials ⚠️
azure/services/managedclusters/managedclusters.go	58.82%	5 Missing and 2 partials ⚠️
controllers/azuremanagedmachinepool_reconciler.go	42.85%	2 Missing and 2 partials ⚠️
azure/converters/managedagentpool.go	94.11%	1 Missing and 1 partial ⚠️
azure/scope/managedcontrolplane.go	66.66%	2 Missing ⚠️
azure/services/aso/service.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4617      +/-   ##
==========================================
+ Coverage   62.71%   62.73%   +0.01%     
==========================================
  Files         192      192              
  Lines       15487    15641     +154     
==========================================
+ Hits         9713     9812      +99     
- Misses       5107     5146      +39     
- Partials      667      683      +16

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

azure/converters/managedagentpool.go

azure/scope/managedcontrolplane.go

azure/scope/managedmachinepool.go

azure/services/agentpools/agentpools.go

azure/services/managedclusters/managedclusters.go

azure/services/managedclusters/spec.go

nojnhuh · 2024-03-04T21:50:52Z

azure/services/managedclusters/spec.go

+				return nil, err
+			}
+			stable := &asocontainerservicev1.ManagedCluster{}
+			if err := stable.ConvertFrom(hub); err != nil {


Maybe we could leave out this particular conversion and only operate against the hub-typed resource until the end where we ultimately convert to preview or stable based on the CAPZ spec? Like you pointed out, that would probably help reduce the agent pool duplicated code. Let's maybe do that in a follow-up PR though.

nojnhuh · 2024-03-04T22:23:06Z

azure/services/managedclusters/managedclusters.go

+			return err
+		}
+		prev := &asocontainerservicev1.ManagedCluster{}
+		if err := prev.ConvertFrom(hub); err != nil {


Like in Parameters, maybe we don't need to convert to any particular version again and we can stick with using the hub type here.

Do we wanna make this change before merging?

I wouldn't block on this.

nojnhuh · 2024-03-04T22:48:05Z

azure/services/aso/aso.go

@@ -252,7 +249,8 @@ func applyPatches[T deepCopier[T]](scheme *runtime.Scheme, spec azure.ASOResourc
 	if err != nil {
 		return zero, errors.Wrap(err, "failed to get GroupVersionKind for object")
 	}
-	parameters.SetGroupVersionKind(gvk)
+
+	(genruntime.MetaObject)(parameters).(interface{ SetGroupVersionKind(schema.GroupVersionKind) }).SetGroupVersionKind(gvk)


A slightly nicer version of this we could do in a follow up might look like this:

Suggested change

(genruntime.MetaObject)(parameters).(interface{ SetGroupVersionKind(schema.GroupVersionKind) }).SetGroupVersionKind(gvk)

type gvkSetter interface{ SetGroupVersionKind(schema.GroupVersionKind) }

var a any = parameters

a.(gvkSetter).SetGroupVersionKind(gvk)

nojnhuh · 2024-03-05T07:06:54Z

interested in getting one more data point to look at tomorrow:
/test pull-cluster-api-provider-azure-e2e-aks

willie-yao · 2024-03-05T17:30:54Z

/retest

jackfrancis · 2024-03-05T18:09:56Z

/test pull-cluster-api-provider-azure-apidiff

jackfrancis · 2024-03-05T20:04:36Z

controllers/azuremanagedmachinepool_reconciler.go

-	agentPoolName := agentPool.AzureName()
+	var agentPoolName string
+	if s.scope.IsPreviewEnabled() {
+		agentPoolTyped := agentPool.(*asocontainerservicev1preview.ManagedClustersAgentPool)


nit: do we need this extra agentPoolTyped var or can we just do:

agentPoolName = agentPool.(*asocontainerservicev1preview.ManagedClustersAgentPool).AzureName()

and ditto below

nojnhuh · 2024-03-05T21:50:46Z

/hold for squash

jackfrancis · 2024-03-06T00:04:08Z

azure/services/managedclusters/spec.go

+			existing = existingObj.(*asocontainerservicev1.ManagedCluster)
+		}
+	}
+


@nojnhuh it appears that the // Additional tags managed separately comment refers to functionality that this PR has broken (until we fix it, that is!)

This should be fixed by this commit: 5ff390c#diff-d7496d9845d55dda5142aab2970a5a9aa1012fbb52d2c462cc5b220682ca520aR821

The issue was caused by not updating the type of the TagsGetterSetter to genruntime.MetaObject, and handling tags differently based on the api version.

willie-yao · 2024-03-06T04:21:34Z

Looks like the job passed with the tags fix and preview enabled! Will retest with preview disabled https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/4617/pull-cluster-api-provider-azure-e2e-aks/1765178975744692224

willie-yao · 2024-03-06T04:21:52Z

/retest

api/v1beta1/azuremanagedcontrolplane_webhook.go

nojnhuh · 2024-03-06T04:28:05Z

azure/services/managedclusters/spec_test.go

@@ -291,6 +292,266 @@ func TestParameters(t *testing.T) {
 		g.Expect(cmp.Diff(actual, expected)).To(BeEmpty())
 	})

+	t.Run("no existing preview managed cluster", func(t *testing.T) {


Can we pare down this test case any more so it's only testing preview-specific things? One test case with everything is enough I think. The bare minimum to show that the result is the preview type instead of the stable type should be enough. And we probably don't even need separate create and update test cases to feel good about that.

azure/services/agentpools/agentpools_test.go

azure/scope/managedmachinepool_test.go

azure/scope/managedmachinepool.go

azure/scope/managedcontrolplane_test.go

azure/scope/managedcontrolplane.go

azure/services/virtualmachines/mock_virtualmachines/client_mock.go

Co-authored-by: Jon Huhn <[email protected]>

k8s-ci-robot · 2024-03-06T20:43:30Z

@willie-yao: 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-apidiff	`ae1e38d`	link	false	`/test pull-cluster-api-provider-azure-apidiff`

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.

willie-yao · 2024-03-06T22:37:23Z

/hold cancel

Squashed! PTAL @jackfrancis @nojnhuh

jackfrancis · 2024-03-06T23:41:22Z

/lgtm

Great work @willie-yao

k8s-ci-robot · 2024-03-06T23:41:27Z

LGTM label has been added.

Git tree hash: 6bb8d42bbaec8035af34c5284062c99a88a99140

nojnhuh · 2024-03-07T16:28:34Z

FYI it doesn't look like any of your contribution here isn't included in the git history, which I think is worth fixing:

% git log -1
commit ae1e38def01a4436d849d0b19682d25dc828e255 (HEAD -> pr/4617, willie/aks-preview-v3)
Author: Jon Huhn <[email protected]>
Date:   Wed Feb 28 20:01:30 2024 -0600

    Expose AKS Preview Features

    Co-authored-by: Jon Huhn <[email protected]>

git commit --reset-author should do the trick.

nojnhuh

/lgtm
/approve

Thank you for sticking with this!

nojnhuh · 2024-03-07T16:29:49Z

api/v1beta1/azuremanagedcontrolplane_webhook.go

@@ -81,6 +81,7 @@ func (mw *azureManagedControlPlaneWebhook) Default(ctx context.Context, obj runt
 	setDefault[*Identity](&m.Spec.Identity, &Identity{
 		Type: ManagedControlPlaneIdentityTypeSystemAssigned,
 	})
+	setDefault[*bool](&m.Spec.EnablePreviewFeatures, ptr.To(false))


For next time, the compiler can usually figure out what the type parameters are so they don't always need to be included.

Suggested change

setDefault[*bool](&m.Spec.EnablePreviewFeatures, ptr.To(false))

setDefault(&m.Spec.EnablePreviewFeatures, ptr.To(false))

k8s-ci-robot · 2024-03-07T16:34:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nojnhuh

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~~ [nojnhuh]

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

nojnhuh · 2024-03-07T16:37:07Z

FYI it doesn't look like any of your contribution here isn't included in the git history, which I think is worth fixing:
% git log -1
commit ae1e38def01a4436d849d0b19682d25dc828e255 (HEAD -> pr/4617, willie/aks-preview-v3)
Author: Jon Huhn <[email protected]>
Date:   Wed Feb 28 20:01:30 2024 -0600

    Expose AKS Preview Features

    Co-authored-by: Jon Huhn <[email protected]>
git commit --reset-author should do the trick.

/hold

nojnhuh · 2024-03-07T16:41:29Z

/hold

Sorry, not quick enough on the draw apparently. :(

willie-yao · 2024-03-07T17:06:57Z

FYI it doesn't look like any of your contribution here isn't included in the git history, which I think is worth fixing:

Whoops that's weird! I think it might've happened when I squashed the commits and I didn't realize 😅

k8s-ci-robot requested review from jackfrancis and Jont828 March 1, 2024 21:52

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 1, 2024

willie-yao mentioned this pull request Mar 1, 2024

WIP: Expose AKS preview features #4596

Closed

4 tasks

willie-yao marked this pull request as ready for review March 1, 2024 21:53

k8s-ci-robot requested a review from marosset March 1, 2024 21:53

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 1, 2024

willie-yao changed the title ~~WIP: Expose AKS preview features~~ Expose AKS preview features Mar 2, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 2, 2024

willie-yao force-pushed the aks-preview-v3 branch from 15025ef to 4dd5fcd Compare March 4, 2024 17:36

mboersma reviewed Mar 4, 2024

View reviewed changes

nojnhuh reviewed Mar 4, 2024

View reviewed changes

jackfrancis reviewed Mar 5, 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 Mar 5, 2024

jackfrancis reviewed Mar 6, 2024

View reviewed changes

nojnhuh reviewed Mar 6, 2024

View reviewed changes

jackfrancis reviewed Mar 6, 2024

View reviewed changes

azure/services/virtualmachines/mock_virtualmachines/client_mock.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 6, 2024

willie-yao force-pushed the aks-preview-v3 branch from 7f2ffff to 5ff9539 Compare March 6, 2024 19:30

Expose AKS Preview Features

ae1e38d

Co-authored-by: Jon Huhn <[email protected]>

willie-yao force-pushed the aks-preview-v3 branch from 5ff9539 to ae1e38d Compare March 6, 2024 19:31

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 6, 2024

k8s-ci-robot assigned jackfrancis Mar 6, 2024

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

nojnhuh approved these changes Mar 7, 2024

View reviewed changes

k8s-ci-robot assigned nojnhuh Mar 7, 2024

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

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, 2024

k8s-ci-robot merged commit adfaabc into kubernetes-sigs:main Mar 7, 2024
20 of 21 checks passed

k8s-ci-robot added this to the v1.14 milestone Mar 7, 2024

dtzar mentioned this pull request Mar 26, 2024

Add artifact streaming support for managed AKS #4414

Closed

willie-yao mentioned this pull request Apr 3, 2024

Follow-up tasks for enabling AKS preview features #4700

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose AKS preview features #4617

Expose AKS preview features #4617

willie-yao commented Mar 1, 2024 •

edited

Loading

k8s-ci-robot commented Mar 1, 2024

codecov bot commented Mar 4, 2024 •

edited

Loading

nojnhuh Mar 4, 2024

nojnhuh Mar 4, 2024

jackfrancis Mar 6, 2024

nojnhuh Mar 6, 2024

nojnhuh Mar 4, 2024

nojnhuh commented Mar 5, 2024

willie-yao commented Mar 5, 2024

jackfrancis commented Mar 5, 2024

jackfrancis Mar 5, 2024

nojnhuh commented Mar 5, 2024

jackfrancis Mar 6, 2024

willie-yao Mar 6, 2024

willie-yao commented Mar 6, 2024

willie-yao commented Mar 6, 2024

nojnhuh Mar 6, 2024

k8s-ci-robot commented Mar 6, 2024 •

edited

Loading

willie-yao commented Mar 6, 2024

jackfrancis commented Mar 6, 2024

k8s-ci-robot commented Mar 6, 2024

nojnhuh commented Mar 7, 2024

nojnhuh left a comment

nojnhuh Mar 7, 2024

k8s-ci-robot commented Mar 7, 2024

nojnhuh commented Mar 7, 2024

nojnhuh commented Mar 7, 2024

willie-yao commented Mar 7, 2024

-	(genruntime.MetaObject)(parameters).(interface{ SetGroupVersionKind(schema.GroupVersionKind) }).SetGroupVersionKind(gvk)
+	type gvkSetter interface{ SetGroupVersionKind(schema.GroupVersionKind) }
+	var a any = parameters
+	a.(gvkSetter).SetGroupVersionKind(gvk)

	setDefault[*bool](&m.Spec.EnablePreviewFeatures, ptr.To(false))
	setDefault(&m.Spec.EnablePreviewFeatures, ptr.To(false))

Expose AKS preview features #4617

Expose AKS preview features #4617

Conversation

willie-yao commented Mar 1, 2024 • edited Loading

k8s-ci-robot commented Mar 1, 2024

codecov bot commented Mar 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojnhuh commented Mar 5, 2024

willie-yao commented Mar 5, 2024

jackfrancis commented Mar 5, 2024

Choose a reason for hiding this comment

nojnhuh commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willie-yao commented Mar 6, 2024

willie-yao commented Mar 6, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 6, 2024 • edited Loading

willie-yao commented Mar 6, 2024

jackfrancis commented Mar 6, 2024

k8s-ci-robot commented Mar 6, 2024

nojnhuh commented Mar 7, 2024

nojnhuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 7, 2024

nojnhuh commented Mar 7, 2024

nojnhuh commented Mar 7, 2024

willie-yao commented Mar 7, 2024

willie-yao commented Mar 1, 2024 •

edited

Loading

codecov bot commented Mar 4, 2024 •

edited

Loading

k8s-ci-robot commented Mar 6, 2024 •

edited

Loading