feat: AKS node pool KubeletConfig

[jackfrancis]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds the AKS node pool feature "KubeletConfig", based on this:

• https://learn.microsoft.com/en-us/rest/api/aks/managed-clusters/create-or-update?tabs=HTTP#kubeletconfig

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

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:

enable AKS node pool KubeletConfig

[jackfrancis]

/retest

[nawazkh]

Just a thought below.
Also, thanks for putting this together!

[nawazkh]

Just a thought, Is there a way we can pull out this code to a common, shared lib so that we don't need to maintain three implementations of the similar definition going ahead?
I am pointing to the getKubletConfig func tagged here, at azure/converters/managedagentpool.go:55, and azure/services/agentpools/spec.go:286

[jackfrancis]

each transformation is actually slightly different! @nojnhuh may have some ideas, I'm confident generics would help us but that would require a farily significant refactor (I think)

[jackfrancis]

also, I've been aiming to just get this changeset to a functional place, definitely open to suggestions about how to do these struct transformations in less painful way

[nawazkh]

each transformation is actually slightly different!

Ah yes!

if len(k.AllowedUnsafeSysctls) > 0 {
  ret.AllowedUnsafeSysctls = &k.AllowedUnsafeSysctls
 }

vs

if k.AllowedUnsafeSysctls != nil {
  ret.AllowedUnsafeSysctls = k.AllowedUnsafeSysctls
}

Sorry, I didn't catch this!

[jackfrancis]

By the way, some follow-up, this translation wasn't required at all, not sure what I did wrong the first time around!

cc @nojnhuh

[zioproto]

I am testing this PR but I have some issues:

I am testing with make tilt-up and then I had to update the crd:

kubectl apply -f https://raw.githubusercontent.com/kubernetes-sigs/cluster-api-provider-azure/8723cdd0418778ac846205aec0d73f470235023c/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml

when I try to apply the AzureManagedMachinePool I get the following error:

 k apply -f - <<EOF
heredoc> ---
apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: AzureManagedMachinePool
metadata:
  name: pool0
  namespace: default
spec:
  enableNodePublicIP: false
  mode: System
  osDiskSizeGB: 30
  sku: Standard_DC4s_v2
  additionalTags:
    owner: "saverio"
  kubeletConfig:
    cpuManagerPolicy: "static"
    cpuCfsQuota: true
    cpuCfsQuotaPeriod: "110ms"
    imageGcHighThreshold: 70
    imageGcLowThreshold: 50
    topologyManagerPolicy: "best-effort"
    allowedUnsafeSysctls:
      - "net.*"
      - "kernel.msg*"
    failSwapOn: false
    containerLogMaxSizeMB: 500
    containerLogMaxFiles: 50
    podMaxPids: 2048
heredoc> EOF
The AzureManagedMachinePool "pool0" is invalid: spec.kubeletConfig.allowedUnsafeSysctls: Unsupported value: []interface {}{"net.*", "kernel.msg*"}: supported values: "kernel.shm*", "kernel.msg*", "kernel.sem", "fs.mqueue.*", "net.*"

I got the kubeletConfig values from the example in this same PR:

cluster-api-provider-azure/docs/book/src/topics/managedcluster.md

Lines 419 to 432 in 8723cdd

	kubeletConfig:
	cpuManagerPolicy: "static"
	cpuCfsQuota: true
	cpuCfsQuotaPeriod: "110ms"
	imageGcHighThreshold: 70
	imageGcLowThreshold: 50
	topologyManagerPolicy: "best-effort"
	allowedUnsafeSysctls:
	- "net.*"
	- "kernel.msg*"
	failSwapOn: false
	containerLogMaxSizeMB: 500
	containerLogMaxFiles: 50
	podMaxPids: 2048

Not sure why the yaml produces this difference:

• Unsupported value: []interface {}{"net.*", "kernel.msg*"}
• supported values: "kernel.shm*", "kernel.msg*", "kernel.sem", "fs.mqueue.*", "net.*"

[zioproto]

Not sure if this is related:
kubernetes-sigs/controller-tools#316
I need someone with more experience on the validation code to have a look.

It seems the root cause is in the enum being an array ?

cluster-api-provider-azure/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml

Lines 230 to 238 in 8723cdd

	enum:
	- kernel.shm*
	- kernel.msg*
	- kernel.sem
	- fs.mqueue.*
	- net.*
	items:
	type: string
	type: array

[jackfrancis]

/assign @nawazkh

@nawazkh can you take another review look at this, thank you!

[nawazkh]

Added some thoughts. Thanks for updating the flavors yamls, made my testing much easier!

[nawazkh]

Thanks! 🚀
/lgtm

[nawazkh]

Tested it again, works as expected.

$ az aks nodepool show -g nhk-cluster-4 --cluster-name aks-24111  -n pool1 -o json | jq .kubeletConfig
{
  "allowedUnsafeSysctls": [
    "net.*",
    "kernel.msg*"
  ],
  "containerLogMaxFiles": 50,
  "containerLogMaxSizeMb": 500,
  "cpuCfsQuota": true,
  "cpuCfsQuotaPeriod": "110ms",
  "cpuManagerPolicy": "static",
  "failSwapOn": false,
  "imageGcHighThreshold": 70,
  "imageGcLowThreshold": 50,
  "podMaxPids": 2048,
  "topologyManagerPolicy": "best-effort"
}

/lgtm

[jackfrancis]

@nawazkh for final lgtm after rebase, thank you!

[nawazkh]

/lgtm
🚀

[jackfrancis]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

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 [jackfrancis]

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

[CecileRobertMichon]

Do these values match the AKS default or best practices? If not, what's the intention behind including them in the AKS flavor reference template?

(just curious, not blocking)

[jackfrancis]

Good question:

1. prefer a value that isn't the AKS default
2. so long as that value isn't too interesting (and could thus interfere with other features and/or E2E tests)

Basically: wanting to make sure that we engage the actual feature in some way (for AKS to actually build a "configured" node pool) without providing a materially different operational outcome.

Are we concerned that the non-tested "reference" template is getting too interesting? Or do we prefer to expose as much configuration as possible as a feature reference? We can always simplify these configurations for the reference templates and triage them to the test/ci templates only...

[CecileRobertMichon]

Are we concerned that the non-tested "reference" template is getting too interesting? Or do we prefer to expose as much configuration as possible as a feature reference?

Yes that's my main concern as far as I know some users are actually using that reference template as base and then applying their own kustomizations to it so I would consider those templates we publish as assets to be "best practice" examples and therefore we should avoid using them to configure random values for testing purposes as it may not be clear to a user what is there because it's needed for proper configuration vs. what was added solely for testing purposes

We can always simplify these configurations for the reference templates and triage them to the test/ci templates only

That's seems reasonable

[jackfrancis]

How do we feel about doing that independent of this PR (cleaning up the reference AKS template so it only has the minimum, required configuration)?

[CecileRobertMichon]

That's fine (I see it's already merged) but let's add that to the milestone to make sure we do it before the template gets published as part of the release

[jackfrancis]

I think this is what we want: #2913

[jackfrancis]

/retest

[jackfrancis]

/retest