Availability set deprecation #1025

kon-angelo · 2024-11-29T08:42:38Z

How to categorize this PR?

/area control-plane
/kind enhancement
/platform azure

What this PR does / why we need it:

Deprecate Availability-Set based shoots
Make VMSS-FLex (VMO) the default for non-zonal clusters
Create a migration path based on https://learn.microsoft.com/en-us/azure/virtual-network/ip-services/public-ip-basic-upgrade-guidance to migrate AS to VMO shoots.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Disable the creation of Availability-Set-based shoots.

VMSS-Flex based shoots are not the default deployment for non-zonal shoots.

Introduce an annotation to migrate the availability-set shoots to VMSS-Flex shoots.

kon-angelo · 2024-11-29T08:42:44Z

/test

testmachinery · 2024-11-29T08:42:48Z

Testrun: e2e-xn69g
Workflow: e2e-xn69g-wf
Phase: Succeeded

+---------------------+---------------------------+-----------+----------+
|        NAME         |           STEP            |   PHASE   | DURATION |
+---------------------+---------------------------+-----------+----------+
| infrastructure-test | infra-flow-test           | Succeeded | 21m14s   |
| infrastructure-test | infra-flow-migration-test | Succeeded | 26m21s   |
| bastion-test        | bastion-test              | Succeeded | 9m4s     |
| infrastructure-test | infrastructure-test       | Succeeded | 23m8s    |
+---------------------+---------------------------+-----------+----------+

AndreasBurger · 2024-11-29T09:25:55Z

pkg/controller/infrastructure/infraflow/ensurer.go

+		if err := fctx.client.Get(ctx, k8sclient.ObjectKey{
+			Namespace: fctx.infra.Namespace,
+			Name:      azure.CloudControllerManagerName,
+		}, deployment); k8sclient.IgnoreNotFound(err) != nil {
+			return err
+		} else if k8sclient.IgnoreNotFound(err) != nil {
+			return nil
+		}


Suggested change

if err := fctx.client.Get(ctx, k8sclient.ObjectKey{

Namespace: fctx.infra.Namespace,

Name: azure.CloudControllerManagerName,

}, deployment); k8sclient.IgnoreNotFound(err) != nil {

return err

} else if k8sclient.IgnoreNotFound(err) != nil {

return nil

}

if err := fctx.client.Get(ctx, k8sclient.ObjectKey{

Namespace: fctx.infra.Namespace,

Name: azure.CloudControllerManagerName,

}, deployment); k8sclient.IgnoreNotFound(err) != nil {

return err

}

Not sure if this is the right one to keep

AndreasBurger · 2024-11-29T09:36:25Z

pkg/controller/infrastructure/infraflow/flow_context.go

@@ -28,6 +29,14 @@ const (
 	defaultLongTimeout = 4 * time.Minute
 )

+var setSeparator = sync.OnceFunc(func() {
+	shared.Separator = "|"


I'm confused, Why do you change the separator here?
Also, is setSeparator the set separator or does it set the separator?

The setSeparator is supposed to change the separator used in the state once. The separator is changed from "/" to "|" because some of the strings that we save do use "/" internally and when the state is restored the slashes interfere with really restoring the state.

We can refactor the strings that we save, or escape the strings or just change the separator. Previous to this PR (and I believe after removing much of the code post-deprecation) we didn't really need to restore things from the state which is why this is only introduced now.

Also, is setSeparator the set separator or does it set the separator?

Is the function to set the separator once. It is executed on package import with the init couple lines below.

AndreasBurger · 2024-11-29T09:38:03Z

pkg/azure/client/loadbalancers.go

+func (c *LoadBalancersClient) Get(ctx context.Context, resourceGroupName, name string) (*armnetwork.LoadBalancer, error) {
+	res, err := c.client.Get(ctx, resourceGroupName, name, nil)
+	if err != nil {
+		return nil, FilterNotFoundError(err)


Haven't checked around, but is this how we usually do it? This get method will always shadow notFound, I think this should be done when calling this function (if the caller wants it)

This is also how the other client implementations work. The comment is fair, but for local consistency it would be applied in a separate PR.

hebelsan · 2024-11-29T11:15:59Z

pkg/controller/infrastructure/infraflow/ensurer.go

@@ -171,7 +177,16 @@ func (fctx *FlowContext) EnsureAvailabilitySet(ctx context.Context) error {
 		return nil
 	}

-	avset, err := fctx.ensureAvailabilitySet(ctx, log, *avsetCfg)
+	// complete AS migration.
+	if v := fctx.whiteboard.GetChild("migration").GetChild(KindAvailabilitySet.String()).Get("complete"); v != nil && *v == "true" {


Nit: I think it would be nice we define such keys as "migration" and "complete" somewhere as constants.

hebelsan · 2024-11-29T13:21:29Z

pkg/controller/infrastructure/infraflow/ensurer.go

+	// return early if the migration has already been complete
+	if v := fctx.whiteboard.GetChild("migration").GetChild(KindAvailabilitySet.String()).Get("complete"); v != nil && *v == "true" {
+		return nil
+	}
+	// return early if the cluster does not have AS.
+	if fctx.whiteboard.GetChild(ChildKeyIDs).Get(KindAvailabilitySet.String()) == nil {
+		return nil
+	}
+
+	// IF VMOs are not needed, or the migration is already done, return early.
+	if !helper.HasShootVmoMigrationAnnotation(fctx.cluster.Shoot.GetAnnotations()) {
+		return nil
+	}


Nit: Aren't these all dependencies for the migration task - maybe we could put them into a separate function and use this function as task dependency.

These checks as a group are used in one place. The separate function will just be giving a name to preflight checks but I can oblige

Rethinking your question:

Do you mean as a shared.DoIf ?
It's a question of when are the dependencies of a task evaluated (at build or at runtime) and AFAIK they are on build time. At that point you may not have the KindAvailabilitySet in the state for example.

Do you mean as a shared.Dependency ?
I am actually not sure if this will block a child Task in the graph from being executed. I would need to test the scemantics. It may either mark the skipped parent as completed (so the childen are always executed), or it may really prune the tree (what you suggested I believe). But I need to check first.

Based on https://github.com/gardener/gardener/blob/84184385dbb9616c2c838e4f24487cae2be75da0/pkg/utils/flow/flow.go#L303 I think it marks the skipped nodes as completed

hebelsan · 2024-11-29T13:48:16Z

pkg/controller/infrastructure/infraflow/infra_adapter.go

+}
+
+// IsVmoRequiredForInfrastructure determines if VMO is required.
+func (ia *InfrastructureAdapter) IsVmoRequiredForInfrastructure() bool {


I don't see where this function is used?

old artifact - thank you

hebelsan · 2024-11-29T14:06:26Z

pkg/controller/infrastructure/infraflow/migrations.go

+	if lb.Properties == nil || len(lb.Properties.FrontendIPConfigurations) == 0 {
+		return nil
+	}


duplicated check

implement migration

039256c

kon-angelo requested review from a team as code owners November 29, 2024 08:42

gardener-robot added the needs/review Needs review label Nov 29, 2024

kon-angelo changed the title ~~Availability set depraction~~ Availability set deprecation Nov 29, 2024

AndreasBurger reviewed Nov 29, 2024

View reviewed changes

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 29, 2024

fix ccm not found error handling

ba7a8a1

kon-angelo force-pushed the avset-deprecation3 branch from dbc9ae3 to ba7a8a1 Compare November 29, 2024 09:47

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 29, 2024

hebelsan reviewed Nov 29, 2024

View reviewed changes

redundant checks and consts for strings

4b3f021

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Availability set deprecation #1025

Availability set deprecation #1025

kon-angelo commented Nov 29, 2024

kon-angelo commented Nov 29, 2024

testmachinery bot commented Nov 29, 2024 •

edited

Loading

AndreasBurger Nov 29, 2024

AndreasBurger Nov 29, 2024

kon-angelo Nov 29, 2024

kon-angelo Nov 29, 2024

AndreasBurger Nov 29, 2024

kon-angelo Nov 29, 2024

hebelsan Nov 29, 2024

hebelsan Nov 29, 2024

kon-angelo Nov 29, 2024

kon-angelo Nov 29, 2024 •

edited

Loading

kon-angelo Nov 29, 2024

hebelsan Nov 29, 2024

kon-angelo Nov 29, 2024

hebelsan Nov 29, 2024

Availability set deprecation #1025

Are you sure you want to change the base?

Availability set deprecation #1025

Conversation

kon-angelo commented Nov 29, 2024

kon-angelo commented Nov 29, 2024

testmachinery bot commented Nov 29, 2024 • edited Loading

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

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

kon-angelo Nov 29, 2024 • edited Loading

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

testmachinery bot commented Nov 29, 2024 •

edited

Loading

kon-angelo Nov 29, 2024 •

edited

Loading