Replace Paid SKU tier with Standard #4045

maciaszczykm · 2023-09-27T11:41:13Z

What type of PR is this?
/kind bug

What this PR does / why we need it: Current SKU tiers seem to be not compatible with the API.

When trying to create Azure managed cluster with SKU tier set to Paid I got:

E0927 11:16:27.943586       1 controller.go:324]  "msg"="Reconciler error" "error"="error creating AzureManagedControlPlane bootstrap/x: failed to reconcile AzureManagedControlPlane service managedcluster: failed to create or update resource x/x (service: managedcluster): PUT https://management.azure.com/subscriptions/x/resourceGroups/x/providers/Microsoft.ContainerService/managedClusters/x\n--------------------------------------------------------------------------------\nRESPONSE 400: 400 Bad Request\nERROR CODE: SkuNotAvailable\n--------------------------------------------------------------------------------\n{\n  \"code\": \"SkuNotAvailable\",\n  \"details\": null,\n  \"message\": \"Paid managed cluster SKU tier is invalid. 'Paid' has been replaced by 'Standard' since v2023-02-01. \",\n  \"subcode\": \"\"\n}\n--------------------------------------------------------------------------------\n" "AzureManagedControlPlane"={"name":"x","namespace":"bootstrap"} "controller"="azuremanagedcontrolplane" "controllerGroup"="infrastructure.cluster.x-k8s.io" "controllerKind"="AzureManagedControlPlane" "name"="x" "namespace"="bootstrap" "reconcileID"="x"

Setting it to Standard is not allowed at the moment:

2023/09/27 12:56:58 failed to create resource: AzureManagedControlPlane.infrastructure.cluster.x-k8s.io "x" is invalid: spec.sku.tier: Unsupported value: "Standard": supported values: "Free", "Paid"

It makes the field unusable.

This PR updates Paid SKU tier to Standard.

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

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Replace Paid SKU tier with Standard

maciaszczykm · 2023-09-27T11:49:15Z

/kind api-change cancel

k8s-ci-robot · 2023-09-27T11:49:17Z

@maciaszczykm: The label(s) kind/cancel cannot be applied, because the repository doesn't have them.

In response to this:

/kind api-change cancel

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.

maciaszczykm · 2023-09-27T11:50:24Z

/remove-kind api-change

maciaszczykm · 2023-09-27T11:57:52Z

As pull-cluster-api-provider-azure-apidiff failed should I update just the variable value and not the name?

codecov · 2023-09-27T11:58:19Z

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (619dfd0) 56.63% compared to head (74251b1) 56.65%.
Report is 33 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4045      +/-   ##
==========================================
+ Coverage   56.63%   56.65%   +0.02%     
==========================================
  Files         187      187              
  Lines       19131    19143      +12     
==========================================
+ Hits        10834    10846      +12     
  Misses       7668     7668              
  Partials      629      629

Files	Coverage Δ
api/v1beta1/azuremanagedcontrolplane_types.go	`20.00% <ø> (ø)`
api/v1beta1/azuremanagedcontrolplane_webhook.go	`88.64% <100.00%> (+0.08%)`	⬆️
azure/defaults.go	`49.10% <100.00%> (ø)`
controllers/nodelister_mock.go	`100.00% <100.00%> (ø)`
internal/test/env/env.go	`53.03% <0.00%> (ø)`
azure/services/privateendpoints/spec.go	`85.11% <78.94%> (ø)`
azure/scope/machinepool.go	`29.18% <0.00%> (ø)`

... and 3 files with indirect coverage changes

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

maciaszczykm · 2023-09-27T13:17:16Z

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

nojnhuh · 2023-09-27T15:31:27Z

config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedcontrolplanes.yaml

@@ -420,7 +420,7 @@ spec:
                    description: Tier - Tier of an AKS cluster.
                    enum:
                    - Free
-                    - Paid
+                    - Standard


@CecileRobertMichon Would it make sense here for the API to accept all of Free, Standard, and Paid and translate Paid to Standard under the hood? That way other users wouldn't have to immediately change.

+1

We can even use a webhook warning to warn the user that "Paid" is deprecated and has been replaced by "Standard" now that those are available

Done. Updated validation and webhook. Added deprecation comment to PaidManagedControlPlaneTier.

Also modified TestDefaultingWebhook to cover webhook SKU tier update.

nojnhuh · 2023-09-28T15:34:42Z

api/v1beta1/azuremanagedcontrolplane_webhook.go

+	// PaidManagedControlPlaneTier has been replaced with StandardManagedControlPlaneTier since v2023-02-01.
+	if m.Spec.SKU != nil && m.Spec.SKU.Tier == PaidManagedControlPlaneTier {
+		m.Spec.SKU.Tier = StandardManagedControlPlaneTier
+		ctrl.Log.WithName("AzureManagedControlPlaneWebHookLogger").Info("Paid SKU tier is deprecated and has been replaced by Standard")


We can even use a webhook warning to warn the user that "Paid" is deprecated and has been replaced by "Standard" now that those are available

I think what Cecile was referring to here was to include this message in the admission.Warnings returned by ValidateCreate. It looks like controller-runtime doesn't allow us to return those warnings from the defaulting webhook.

That's right. However, if we're changing the value in the defaulting webhook, we'll never print the warning since the validation webhook runs after defaulting (so by the time we reach ValidateCreate we would have already changed the value).

I think the way it's implemented now is probably good enough, wdyt @nojnhuh ?

It seems like it wouldn't be too messy to do the validation in the webhook and the Paid->Standard translation further down the road, but I agree keeping everything in the defaulting webhook works just as well.

CecileRobertMichon

/lgtm

k8s-ci-robot · 2023-09-28T16:25:31Z

LGTM label has been added.

Git tree hash: d12f64a5d3a0487b48f8bf21c39788284e67d465

CecileRobertMichon · 2023-09-28T16:25:43Z

should we backport this?

maciaszczykm · 2023-09-28T16:46:36Z

Yes, I would like to use it ASAP.

CecileRobertMichon · 2023-09-28T17:00:18Z

/cherry-pick release-1.11

I'm assuming this doesn't need to get cherry-picked to release-1.10 since only 1.11 has the SDK changes

k8s-infra-cherrypick-robot · 2023-09-28T17:00:20Z

@CecileRobertMichon: once the present PR merges, I will cherry-pick it on top of release-1.11 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.11

I'm assuming this doesn't need to get cherry-picked to release-1.10 since only 1.11 has the SDK changes

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.

nojnhuh

/lgtm
/approve

k8s-ci-robot · 2023-09-28T17:20:02Z

[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

k8s-infra-cherrypick-robot · 2023-09-28T17:21:14Z

@CecileRobertMichon: new pull request created: #4050

In response to this:

/cherry-pick release-1.11

I'm assuming this doesn't need to get cherry-picked to release-1.10 since only 1.11 has the SDK changes

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.

k8s-ci-robot requested review from CecileRobertMichon and mboersma September 27, 2023 11:41

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 27, 2023

k8s-ci-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Sep 27, 2023

maciaszczykm mentioned this pull request Sep 27, 2023

feat: Switch to official v1.11 CAPZ image pluralsh/plural-artifacts#852

Merged

25 tasks

nojnhuh reviewed Sep 27, 2023

View reviewed changes

maciaszczykm force-pushed the standard-sku-tier branch 2 times, most recently from b5124f4 to fa80059 Compare September 28, 2023 07:31

replace Paid SKU tier with Standard

74251b1

maciaszczykm force-pushed the standard-sku-tier branch from fa80059 to 74251b1 Compare September 28, 2023 07:34

nojnhuh reviewed Sep 28, 2023

View reviewed changes

CecileRobertMichon reviewed Sep 28, 2023

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Sep 28, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2023

nojnhuh reviewed Sep 28, 2023

View reviewed changes

k8s-ci-robot assigned nojnhuh Sep 28, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2023

k8s-ci-robot merged commit c4d959b into kubernetes-sigs:main Sep 28, 2023

k8s-ci-robot added this to the v1.12 milestone Sep 28, 2023

k8s-infra-cherrypick-robot mentioned this pull request Sep 28, 2023

[release-1.11] Replace Paid SKU tier with Standard #4050

Merged

maciaszczykm deleted the standard-sku-tier branch October 31, 2023 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Paid SKU tier with Standard #4045

Replace Paid SKU tier with Standard #4045

maciaszczykm commented Sep 27, 2023 •

edited

Loading

maciaszczykm commented Sep 27, 2023

k8s-ci-robot commented Sep 27, 2023

maciaszczykm commented Sep 27, 2023

maciaszczykm commented Sep 27, 2023 •

edited

Loading

codecov bot commented Sep 27, 2023 •

edited

Loading

maciaszczykm commented Sep 27, 2023

nojnhuh Sep 27, 2023

CecileRobertMichon Sep 27, 2023

maciaszczykm Sep 28, 2023

maciaszczykm Sep 28, 2023

nojnhuh Sep 28, 2023

CecileRobertMichon Sep 28, 2023

nojnhuh Sep 28, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Sep 28, 2023

CecileRobertMichon commented Sep 28, 2023

maciaszczykm commented Sep 28, 2023

CecileRobertMichon commented Sep 28, 2023

k8s-infra-cherrypick-robot commented Sep 28, 2023

nojnhuh left a comment

k8s-ci-robot commented Sep 28, 2023

k8s-infra-cherrypick-robot commented Sep 28, 2023

Replace Paid SKU tier with Standard #4045

Replace Paid SKU tier with Standard #4045

Conversation

maciaszczykm commented Sep 27, 2023 • edited Loading

maciaszczykm commented Sep 27, 2023

k8s-ci-robot commented Sep 27, 2023

maciaszczykm commented Sep 27, 2023

maciaszczykm commented Sep 27, 2023 • edited Loading

codecov bot commented Sep 27, 2023 • edited Loading

Codecov Report

maciaszczykm commented Sep 27, 2023

nojnhuh Sep 27, 2023

Choose a reason for hiding this comment

CecileRobertMichon Sep 27, 2023

Choose a reason for hiding this comment

maciaszczykm Sep 28, 2023

Choose a reason for hiding this comment

maciaszczykm Sep 28, 2023

Choose a reason for hiding this comment

nojnhuh Sep 28, 2023

Choose a reason for hiding this comment

CecileRobertMichon Sep 28, 2023

Choose a reason for hiding this comment

nojnhuh Sep 28, 2023

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 28, 2023

CecileRobertMichon commented Sep 28, 2023

maciaszczykm commented Sep 28, 2023

CecileRobertMichon commented Sep 28, 2023

k8s-infra-cherrypick-robot commented Sep 28, 2023

nojnhuh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 28, 2023

k8s-infra-cherrypick-robot commented Sep 28, 2023

maciaszczykm commented Sep 27, 2023 •

edited

Loading

maciaszczykm commented Sep 27, 2023 •

edited

Loading

codecov bot commented Sep 27, 2023 •

edited

Loading