From de4c096a9b99c110565ce02d6cde16fc61f8c711 Mon Sep 17 00:00:00 2001 From: Artem Chernyshev Date: Wed, 10 Apr 2024 19:45:07 +0300 Subject: [PATCH] fix: ignore not existing cluster in `MachineSet` teardown flow If the cluster is not found it's not critical for teardown flow, so we shouldn't skip reconcile in that case. Signed-off-by: Artem Chernyshev --- .../machineset/control_planes_handler_test.go | 1 + .../omni/internal/machineset/operations.go | 18 ++++++++--- .../internal/machineset/operations_test.go | 4 +++ .../machineset/reconciliation_context.go | 32 +++++++++++++------ .../machineset/reconciliation_context_test.go | 1 + .../machineset/status_handler_test.go | 2 ++ .../machineset/workers_handler_test.go | 1 + 7 files changed, 44 insertions(+), 15 deletions(-) diff --git a/internal/backend/runtime/omni/controllers/omni/internal/machineset/control_planes_handler_test.go b/internal/backend/runtime/omni/controllers/omni/internal/machineset/control_planes_handler_test.go index 3bab6c25..1dd01393 100644 --- a/internal/backend/runtime/omni/controllers/omni/internal/machineset/control_planes_handler_test.go +++ b/internal/backend/runtime/omni/controllers/omni/internal/machineset/control_planes_handler_test.go @@ -187,6 +187,7 @@ func TestControlPlanesHandler(t *testing.T) { require := require.New(t) machineSet.TypedSpec().Value = tt.machineSet + machineSet.Metadata().Labels().Set(omni.LabelCluster, tt.name) cluster := omni.NewCluster(resources.DefaultNamespace, tt.name) cluster.TypedSpec().Value.TalosVersion = "v1.5.4" diff --git a/internal/backend/runtime/omni/controllers/omni/internal/machineset/operations.go b/internal/backend/runtime/omni/controllers/omni/internal/machineset/operations.go index e100b17a..2342e7ce 100644 --- a/internal/backend/runtime/omni/controllers/omni/internal/machineset/operations.go +++ b/internal/backend/runtime/omni/controllers/omni/internal/machineset/operations.go @@ -47,11 +47,16 @@ func (c *Create) Apply(ctx context.Context, r controller.ReaderWriter, logger *z helpers.UpdateInputsVersions(clusterMachine, configPatches...) setPatches(clusterMachineConfigPatches, configPatches) - clusterMachine.TypedSpec().Value.KubernetesVersion = rc.GetCluster().TypedSpec().Value.KubernetesVersion + var err error + + clusterMachine.TypedSpec().Value.KubernetesVersion, err = rc.GetKubernetesVersion() + if err != nil { + return err + } logger.Info("create cluster machine", zap.String("machine", c.ID)) - if err := r.Create(ctx, clusterMachine); err != nil { + if err = r.Create(ctx, clusterMachine); err != nil { return err } @@ -78,7 +83,7 @@ func (d *Teardown) Apply(ctx context.Context, r controller.ReaderWriter, logger return fmt.Errorf( "error tearing down machine %q in cluster %q: %w", d.ID, - rc.GetCluster().Metadata().ID(), + rc.GetClusterName(), err, ) } @@ -135,14 +140,17 @@ func (u *Update) Apply(ctx context.Context, r controller.ReaderWriter, logger *z return safe.WriterModify(ctx, r, clusterMachine, func(res *omni.ClusterMachine) error { // don't update the ClusterMachine if it's still owned by another cluster currentClusterName, ok := res.Metadata().Labels().Get(omni.LabelCluster) - if ok && currentClusterName != rc.cluster.Metadata().ID() { + if ok && currentClusterName != rc.GetClusterName() { return nil } helpers.CopyAllAnnotations(clusterMachine, res) if res.TypedSpec().Value.KubernetesVersion == "" { - res.TypedSpec().Value.KubernetesVersion = rc.GetCluster().TypedSpec().Value.KubernetesVersion + res.TypedSpec().Value.KubernetesVersion, err = rc.GetKubernetesVersion() + if err != nil { + return err + } } return nil diff --git a/internal/backend/runtime/omni/controllers/omni/internal/machineset/operations_test.go b/internal/backend/runtime/omni/controllers/omni/internal/machineset/operations_test.go index 9902b55b..a2e2a99c 100644 --- a/internal/backend/runtime/omni/controllers/omni/internal/machineset/operations_test.go +++ b/internal/backend/runtime/omni/controllers/omni/internal/machineset/operations_test.go @@ -85,6 +85,7 @@ func TestCreate(t *testing.T) { cluster.TypedSpec().Value.KubernetesVersion = "v1.6.2" machineSet := omni.NewMachineSet(resources.DefaultNamespace, "machineset") + machineSet.Metadata().Labels().Set(omni.LabelCluster, cluster.Metadata().ID()) create := machineset.Create{ID: "aa"} @@ -157,6 +158,7 @@ func TestUpdate(t *testing.T) { cluster.TypedSpec().Value.KubernetesVersion = "v1.6.4" machineSet := omni.NewMachineSet(resources.DefaultNamespace, "machineset") + machineSet.Metadata().Labels().Set(omni.LabelCluster, cluster.Metadata().ID()) quota := &machineset.ChangeQuota{ Update: 2, @@ -292,6 +294,7 @@ func TestTeardown(t *testing.T) { cluster.TypedSpec().Value.KubernetesVersion = "v1.6.3" machineSet := omni.NewMachineSet(resources.DefaultNamespace, "machineset") + machineSet.Metadata().Labels().Set(omni.LabelCluster, cluster.Metadata().ID()) quota := machineset.ChangeQuota{ Teardown: 1, @@ -358,6 +361,7 @@ func TestDestroy(t *testing.T) { cluster.TypedSpec().Value.KubernetesVersion = "v1.6.3" machineSet := omni.NewMachineSet(resources.DefaultNamespace, "machineset") + machineSet.Metadata().Labels().Set(omni.LabelCluster, cluster.Metadata().ID()) require := require.New(t) diff --git a/internal/backend/runtime/omni/controllers/omni/internal/machineset/reconciliation_context.go b/internal/backend/runtime/omni/controllers/omni/internal/machineset/reconciliation_context.go index 3150b4e7..50492162 100644 --- a/internal/backend/runtime/omni/controllers/omni/internal/machineset/reconciliation_context.go +++ b/internal/backend/runtime/omni/controllers/omni/internal/machineset/reconciliation_context.go @@ -7,14 +7,13 @@ package machineset import ( "context" + "errors" "fmt" "github.com/cosi-project/runtime/pkg/controller" - "github.com/cosi-project/runtime/pkg/controller/generic/qtransform" "github.com/cosi-project/runtime/pkg/resource" "github.com/cosi-project/runtime/pkg/safe" "github.com/cosi-project/runtime/pkg/state" - "github.com/siderolabs/gen/xerrors" "github.com/siderolabs/gen/xslices" "github.com/siderolabs/omni/client/api/omni/specs" @@ -76,6 +75,8 @@ type ReconciliationContext struct { clusterMachineConfigPatchesMap map[resource.ID]*omni.ClusterMachineConfigPatches clusterMachineStatusesMap map[resource.ID]*omni.ClusterMachineStatus + clusterName string + runningMachineSetNodesSet Set[string] idsTearingDown Set[string] idsUnconfigured Set[string] @@ -104,11 +105,7 @@ func BuildReconciliationContext( } cluster, err := safe.ReaderGetByID[*omni.Cluster](ctx, r, clusterName) - if err != nil { - if state.IsNotFoundError(err) { - return nil, xerrors.NewTagged[qtransform.SkipReconcileTag](err) - } - + if err != nil && !state.IsNotFoundError(err) { return nil, err } @@ -179,9 +176,15 @@ func NewReconciliationContext( clusterMachineConfigPatches []*omni.ClusterMachineConfigPatches, clusterMachineStatuses []*omni.ClusterMachineStatus, ) (*ReconciliationContext, error) { + clusterName, ok := machineSet.Metadata().Labels().Get(omni.LabelCluster) + if !ok { + return nil, fmt.Errorf("failed to determine the cluster of the machine set %q", machineSet.Metadata().ID()) + } + rc := &ReconciliationContext{ machineSet: machineSet, cluster: cluster, + clusterName: clusterName, patchesByMachine: map[resource.ID][]*omni.ConfigPatch{}, } @@ -345,9 +348,18 @@ func (rc *ReconciliationContext) GetOutdatedMachines() Set[string] { return rc.idsOutdated } -// GetCluster reads the related cluster resource. -func (rc *ReconciliationContext) GetCluster() *omni.Cluster { - return rc.cluster +// GetKubernetesVersion reads kubernetes version from the related cluster if the cluster exists. +func (rc *ReconciliationContext) GetKubernetesVersion() (string, error) { + if rc.cluster == nil { + return "", errors.New("failed to get kubernetes version for the machine set as the cluster couldn't be found") + } + + return rc.cluster.TypedSpec().Value.KubernetesVersion, nil +} + +// GetClusterName reads cluster name from the context. +func (rc *ReconciliationContext) GetClusterName() string { + return rc.clusterName } // GetMachineSet reads the related machine set resource. diff --git a/internal/backend/runtime/omni/controllers/omni/internal/machineset/reconciliation_context_test.go b/internal/backend/runtime/omni/controllers/omni/internal/machineset/reconciliation_context_test.go index 4fbd49fc..1ccbf10c 100644 --- a/internal/backend/runtime/omni/controllers/omni/internal/machineset/reconciliation_context_test.go +++ b/internal/backend/runtime/omni/controllers/omni/internal/machineset/reconciliation_context_test.go @@ -312,6 +312,7 @@ func TestReconciliationContext(t *testing.T) { machineSet := omni.NewMachineSet(resources.DefaultNamespace, tt.name) machineSet.TypedSpec().Value = tt.machineSet + machineSet.Metadata().Labels().Set(omni.LabelCluster, tt.name) cluster := omni.NewCluster(resources.DefaultNamespace, tt.name) cluster.TypedSpec().Value.TalosVersion = "v1.6.4" diff --git a/internal/backend/runtime/omni/controllers/omni/internal/machineset/status_handler_test.go b/internal/backend/runtime/omni/controllers/omni/internal/machineset/status_handler_test.go index 5f07820e..851e9c18 100644 --- a/internal/backend/runtime/omni/controllers/omni/internal/machineset/status_handler_test.go +++ b/internal/backend/runtime/omni/controllers/omni/internal/machineset/status_handler_test.go @@ -329,6 +329,8 @@ func TestStatusHandler(t *testing.T) { machineSet = omni.NewMachineSet(resources.DefaultNamespace, "test") } + machineSet.Metadata().Labels().Set(omni.LabelCluster, "test") + require := require.New(t) clusterMachineConfigStatuses := make([]*omni.ClusterMachineConfigStatus, 0, len(tt.clusterMachines)) diff --git a/internal/backend/runtime/omni/controllers/omni/internal/machineset/workers_handler_test.go b/internal/backend/runtime/omni/controllers/omni/internal/machineset/workers_handler_test.go index 1e8a00e4..f8ca976b 100644 --- a/internal/backend/runtime/omni/controllers/omni/internal/machineset/workers_handler_test.go +++ b/internal/backend/runtime/omni/controllers/omni/internal/machineset/workers_handler_test.go @@ -173,6 +173,7 @@ func TestWorkersHandler(t *testing.T) { require := require.New(t) machineSet.TypedSpec().Value = tt.machineSet + machineSet.Metadata().Labels().Set(omni.LabelCluster, tt.name) cluster := omni.NewCluster(resources.DefaultNamespace, tt.name) cluster.TypedSpec().Value.TalosVersion = "v1.6.0"