Skip to content
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

Add support for kubeletDiskType #2938

Merged
merged 1 commit into from
Jan 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions azure/converters/managedagentpool.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func AgentPoolToManagedClusterAgentPoolProfile(pool containerservice.AgentPool)
NodePublicIPPrefixID: properties.NodePublicIPPrefixID,
ScaleSetPriority: properties.ScaleSetPriority,
Tags: properties.Tags,
KubeletDiskType: properties.KubeletDiskType,
}
if properties.KubeletConfig != nil {
agentPool.KubeletConfig = properties.KubeletConfig
Expand Down
1 change: 1 addition & 0 deletions azure/scope/managedmachinepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ func buildAgentPoolSpec(managedControlPlane *infrav1exp.AzureManagedControlPlane
NodePublicIPPrefixID: managedMachinePool.Spec.NodePublicIPPrefixID,
ScaleSetPriority: managedMachinePool.Spec.ScaleSetPriority,
AdditionalTags: managedMachinePool.Spec.AdditionalTags,
KubeletDiskType: managedMachinePool.Spec.KubeletDiskType,
}

if managedMachinePool.Spec.OSDiskSizeGB != nil {
Expand Down
101 changes: 101 additions & 0 deletions azure/scope/managedmachinepool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,101 @@ func TestManagedMachinePoolScope_OSDiskType(t *testing.T) {
}
}

func TestManagedMachinePoolScope_KubeletDiskType(t *testing.T) {
scheme := runtime.NewScheme()
_ = expv1.AddToScheme(scheme)
_ = infrav1exp.AddToScheme(scheme)

cases := []struct {
Name string
Input ManagedMachinePoolScopeParams
Expected azure.ResourceSpecGetter
}{
{
Name: "Without KubeletDiskType",
Input: ManagedMachinePoolScopeParams{
Cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster1",
Namespace: "default",
},
},
ControlPlane: &infrav1exp.AzureManagedControlPlane{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster1",
Namespace: "default",
},
Spec: infrav1exp.AzureManagedControlPlaneSpec{
SubscriptionID: "00000000-0000-0000-0000-000000000000",
},
},
ManagedMachinePool: ManagedMachinePool{
MachinePool: getMachinePool("pool0"),
InfraMachinePool: getAzureMachinePool("pool0", infrav1exp.NodePoolModeSystem),
},
},
Expected: &agentpools.AgentPoolSpec{
Name: "pool0",
SKU: "Standard_D2s_v3",
Replicas: 1,
Mode: "System",
Cluster: "cluster1",
VnetSubnetID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups//providers/Microsoft.Network/virtualNetworks//subnets/",
Headers: map[string]string{},
},
},
{
Name: "With KubeletDiskType",
Input: ManagedMachinePoolScopeParams{
Cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster1",
Namespace: "default",
},
},
ControlPlane: &infrav1exp.AzureManagedControlPlane{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster1",
Namespace: "default",
},
Spec: infrav1exp.AzureManagedControlPlaneSpec{
SubscriptionID: "00000000-0000-0000-0000-000000000000",
},
},
ManagedMachinePool: ManagedMachinePool{
MachinePool: getMachinePool("pool1"),
InfraMachinePool: getAzureMachinePoolWithKubeletDiskType("pool1", (*infrav1exp.KubeletDiskType)(to.StringPtr("Temporary"))),
},
},
Expected: &agentpools.AgentPoolSpec{
Name: "pool1",
SKU: "Standard_D2s_v3",
Mode: "User",
Cluster: "cluster1",
Replicas: 1,
KubeletDiskType: (*infrav1exp.KubeletDiskType)(to.StringPtr("Temporary")),
VnetSubnetID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups//providers/Microsoft.Network/virtualNetworks//subnets/",
Headers: map[string]string{},
},
},
}

for _, c := range cases {
c := c
t.Run(c.Name, func(t *testing.T) {
g := NewWithT(t)
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(c.Input.MachinePool, c.Input.InfraMachinePool, c.Input.ControlPlane).Build()
c.Input.Client = fakeClient
s, err := NewManagedMachinePoolScope(context.TODO(), c.Input)
g.Expect(err).To(Succeed())
agentPool := s.AgentPoolSpec()
if !reflect.DeepEqual(c.Expected, agentPool) {
t.Errorf("Got difference between expected result and result:\n%s", cmp.Diff(c.Expected, agentPool))
}
})
}
}

func getAzureMachinePool(name string, mode infrav1exp.NodePoolMode) *infrav1exp.AzureManagedMachinePool {
return &infrav1exp.AzureManagedMachinePool{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -675,6 +770,12 @@ func getAzureMachinePoolWithOsDiskType(name string, osDiskType string) *infrav1e
return managedPool
}

func getAzureMachinePoolWithKubeletDiskType(name string, kubeletDiskType *infrav1exp.KubeletDiskType) *infrav1exp.AzureManagedMachinePool {
managedPool := getAzureMachinePool(name, infrav1exp.NodePoolModeUser)
managedPool.Spec.KubeletDiskType = kubeletDiskType
return managedPool
}

func getAzureMachinePoolWithLabels(name string, nodeLabels map[string]string) *infrav1exp.AzureManagedMachinePool {
managedPool := getAzureMachinePool(name, infrav1exp.NodePoolModeSystem)
managedPool.Spec.NodeLabels = nodeLabels
Expand Down
2 changes: 2 additions & 0 deletions azure/services/agentpools/agentpools_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"sigs.k8s.io/cluster-api-provider-azure/azure"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/agentpools/mock_agentpools"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async"
infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1"
gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock"
)

Expand All @@ -54,6 +55,7 @@ var (
EnableUltraSSD: to.BoolPtr(true),
OSType: to.StringPtr("fake-os-type"),
Headers: map[string]string{"fake-header": "fake-value"},
KubeletDiskType: (*infrav1exp.KubeletDiskType)(to.StringPtr("fake-kubelet-disk-type")),
}

internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: http.StatusInternalServerError}, "Internal Server Error")
Expand Down
5 changes: 5 additions & 0 deletions azure/services/agentpools/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
"sigs.k8s.io/cluster-api-provider-azure/azure"
"sigs.k8s.io/cluster-api-provider-azure/azure/converters"
infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1"
azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure"
)

Expand Down Expand Up @@ -130,6 +131,9 @@ type AgentPoolSpec struct {
// KubeletConfig specifies the kubelet configurations for nodes.
KubeletConfig *KubeletConfig `json:"kubeletConfig,omitempty"`

// KubeletDiskType specifies the kubelet disk type for each node in the pool. Allowed values are 'OS' and 'Temporary'
KubeletDiskType *infrav1exp.KubeletDiskType `json:"kubeletDiskType,omitempty"`

// AdditionalTags is an optional set of tags to add to Azure resources managed by the Azure provider, in addition to the ones added by default.
AdditionalTags infrav1.Tags
}
Expand Down Expand Up @@ -278,6 +282,7 @@ func (s *AgentPoolSpec) Parameters(existing interface{}) (params interface{}, er
EnableAutoScaling: s.EnableAutoScaling,
EnableUltraSSD: s.EnableUltraSSD,
KubeletConfig: kubeletConfig,
KubeletDiskType: containerservice.KubeletDiskType(to.String((*string)(s.KubeletDiskType))),
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this wasn't added to the normalizedProfile used to calculate updates here: https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2938/files#diff-2c4585415b3fabfd1913f95eb20bc57daac00c2facd3ffa45d3bb00bb1d41b63R189

My understanding is that it's intentional because the property is immutable but I just want to confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the link might be pointing to the wrong place, but I don't think this was intentional. I'm not exactly familiar with the normalizedProfile. Would this field be left out since it's immutable?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe @nojnhuh or @jackfrancis can help answer this

Copy link
Contributor

Choose a reason for hiding this comment

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

GitHub is having trouble rendering that link for me too, but I'm guessing it's here?

KubeletConfig: existingPool.KubeletConfig,

@CecileRobertMichon That's not the same field being added in this change, and I see that's conditionally added to normalizedProfile below, so that seems fine at first glance to me, albeit slightly asymmetrical with everything else:
normalizedProfile.KubeletConfig = &containerservice.KubeletConfig{

To your question @willie-yao, yes, if this field is immutable, then we don't need to include it in existingProfile or normalizedProfile. A diff between those two objects is what ultimately triggers a PUT request to make an update, but the webhook enforces that this field can't change.

MaxCount: s.MaxCount,
MaxPods: s.MaxPods,
MinCount: s.MinCount,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,14 @@ spec:
- single-numa-node
type: string
type: object
kubeletDiskType:
description: 'KubeletDiskType specifies the kubelet disk type. Default
to OS. Possible values include: ''OS'', ''Temporary''. Requires
kubeletDisk preview feature to be set.'
enum:
- OS
- Temporary
type: string
maxPods:
description: MaxPods specifies the kubelet --max-pods configuration
for the node pool.
Expand Down
24 changes: 24 additions & 0 deletions docs/book/src/topics/managedcluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,30 @@ spec:
osDiskType: "Ephemeral"
```

## AKS Node Pool KubeletDiskType configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

This made me realize I haven't been adding docs for new features at all, so thanks for adding this! I opened an issue to make sure we get caught up there: #2948


You can configure the `KubeletDiskType` value for each AKS node pool (`AzureManagedMachinePool`) that you define in your spec (see [here](https://learn.microsoft.com/en-us/rest/api/aks/agent-pools/create-or-update?tabs=HTTP#kubeletdisktype) for the official AKS documentation). There are two options to choose from: `"OS"` or `"Temporary"`.

Before this feature can be used, you must register the `KubeletDisk` feature on your Azure subscription with the following az cli command.

```bash
az feature register --namespace Microsoft.ContainerService --name KubeletDisk
```

Below an example `kubeletDiskType` configuration is assigned to `agentpool0`, specifying that the emptyDir volumes, container runtime data root, and Kubelet ephemeral storage will be stored on the temporary disk:

```yaml
apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: AzureManagedMachinePool
metadata:
name: agentpool0
spec:
mode: System
osDiskSizeGB: 30
sku: Standard_D2s_v3
kubeletDiskType: "Temporary"
```

### AKS Node Pool Taints

You can configure the `Taints` value for each AKS node pool (`AzureManagedMachinePool`) that you define in your spec.
Expand Down
1 change: 1 addition & 0 deletions exp/api/v1alpha3/azuremanagedmachinepool_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ func (src *AzureManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.NodePublicIPPrefixID = restored.Spec.NodePublicIPPrefixID
dst.Spec.ScaleSetPriority = restored.Spec.ScaleSetPriority
dst.Spec.AdditionalTags = restored.Spec.AdditionalTags
dst.Spec.KubeletDiskType = restored.Spec.KubeletDiskType
if restored.Spec.KubeletConfig != nil {
dst.Spec.KubeletConfig = restored.Spec.KubeletConfig
}
Expand Down
1 change: 1 addition & 0 deletions exp/api/v1alpha3/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions exp/api/v1alpha4/azuremanagedmachinepool_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ func (src *AzureManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.NodePublicIPPrefixID = restored.Spec.NodePublicIPPrefixID
dst.Spec.ScaleSetPriority = restored.Spec.ScaleSetPriority
dst.Spec.AdditionalTags = restored.Spec.AdditionalTags
dst.Spec.KubeletDiskType = restored.Spec.KubeletDiskType
if restored.Spec.KubeletConfig != nil {
dst.Spec.KubeletConfig = restored.Spec.KubeletConfig
}
Expand Down
1 change: 1 addition & 0 deletions exp/api/v1alpha4/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions exp/api/v1beta1/azuremanagedmachinepool_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@ const (
// TopologyManagerPolicy enumerates the values for KubeletConfig.TopologyManagerPolicy.
type TopologyManagerPolicy string

// KubeletDiskType enumerates the values for the agent pool's KubeletDiskType.
type KubeletDiskType string

const (
// KubeletDiskTypeOS ...
KubeletDiskTypeOS KubeletDiskType = "OS"
// KubeletDiskTypeTemporary ...
KubeletDiskTypeTemporary KubeletDiskType = "Temporary"
)

const (
// TopologyManagerPolicyNone ...
TopologyManagerPolicyNone TopologyManagerPolicy = "none"
Expand Down Expand Up @@ -189,6 +199,12 @@ type AzureManagedMachinePoolSpec struct {
// KubeletConfig specifies the kubelet configurations for nodes.
// +optional
KubeletConfig *KubeletConfig `json:"kubeletConfig,omitempty"`

// KubeletDiskType specifies the kubelet disk type. Default to OS. Possible values include: 'OS', 'Temporary'.
// Requires kubeletDisk preview feature to be set.
// +kubebuilder:validation:Enum=OS;Temporary
// +optional
KubeletDiskType *KubeletDiskType `json:"kubeletDiskType,omitempty"`
}

// ManagedMachinePoolScaling specifies scaling options.
Expand Down
7 changes: 7 additions & 0 deletions exp/api/v1beta1/azuremanagedmachinepool_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,13 @@ func (m *AzureManagedMachinePool) ValidateUpdate(oldRaw runtime.Object, client c
allErrs = append(allErrs, err)
}

if err := webhookutils.ValidateImmutable(
field.NewPath("Spec", "KubeletDiskType"),
old.Spec.KubeletDiskType,
m.Spec.KubeletDiskType); err != nil {
allErrs = append(allErrs, err)
}

if len(allErrs) != 0 {
return apierrors.NewInvalid(GroupVersion.WithKind("AzureManagedMachinePool").GroupKind(), m.Name, allErrs)
}
Expand Down
5 changes: 5 additions & 0 deletions exp/api/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.