Skip to content

Commit 2db768d

Browse files
smirasmira
committed
refactor: drop extra input finalizers
Simplify controllers, add migrations to clear finalizers which are no longer needed. See cosi-project/runtime#633 Signed-off-by: Andrey Smirnov <[email protected]>
1 parent faf5432 commit 2db768d

File tree

7 files changed

+68
-112
lines changed

7 files changed

+68
-112
lines changed

‎internal/backend/runtime/omni/controllers/omni/cluster_destroy_status.go‎

Lines changed: 6 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ package omni
88
import (
99
"context"
1010
"fmt"
11-
"log"
1211

1312
"github.com/cosi-project/runtime/pkg/controller"
1413
"github.com/cosi-project/runtime/pkg/controller/generic/qtransform"
@@ -44,8 +43,10 @@ func NewClusterDestroyStatusController() *ClusterDestroyStatusController {
4443
UnmapMetadataFunc: func(clusterDestroyStatus *omni.ClusterDestroyStatus) *omni.Cluster {
4544
return omni.NewCluster(resources.DefaultNamespace, clusterDestroyStatus.Metadata().ID())
4645
},
47-
TransformExtraOutputFunc: func(ctx context.Context, r controller.ReaderWriter, _ *zap.Logger, cluster *omni.Cluster, clusterDestroyStatus *omni.ClusterDestroyStatus) error {
48-
remainingMachines := 0
46+
TransformFunc: func(ctx context.Context, r controller.Reader, _ *zap.Logger, cluster *omni.Cluster, clusterDestroyStatus *omni.ClusterDestroyStatus) error {
47+
if cluster.Metadata().Phase() != resource.PhaseTearingDown {
48+
return xerrors.NewTaggedf[qtransform.SkipReconcileTag]("not tearing down")
49+
}
4950

5051
msStatuses, err := r.List(ctx, omni.NewMachineSetStatus(resources.DefaultNamespace, "").Metadata(), state.WithLabelQuery(
5152
resource.LabelEqual(omni.LabelCluster, cluster.Metadata().ID()),
@@ -54,31 +55,6 @@ func NewClusterDestroyStatusController() *ClusterDestroyStatusController {
5455
return fmt.Errorf("failed to list control planes %w", err)
5556
}
5657

57-
remainingMachineSetIDs := make(map[resource.ID]struct{}, len(msStatuses.Items))
58-
for _, status := range msStatuses.Items {
59-
switch status.Metadata().Phase() {
60-
case resource.PhaseRunning:
61-
if !status.Metadata().Finalizers().Has(ClusterDestroyStatusControllerName) {
62-
if err = r.AddFinalizer(ctx, status.Metadata(), ClusterDestroyStatusControllerName); err != nil {
63-
return err
64-
}
65-
}
66-
remainingMachineSetIDs[status.Metadata().ID()] = struct{}{}
67-
case resource.PhaseTearingDown:
68-
if status.Metadata().Finalizers().Has(ClusterDestroyStatusControllerName) {
69-
if len(*status.Metadata().Finalizers()) == 1 {
70-
log.Printf("Removing finalizer for cluster %s", status.Metadata().ID())
71-
if err = r.RemoveFinalizer(ctx, status.Metadata(), ClusterDestroyStatusControllerName); err != nil {
72-
return err
73-
}
74-
75-
continue
76-
}
77-
remainingMachineSetIDs[status.Metadata().ID()] = struct{}{}
78-
}
79-
}
80-
}
81-
8258
cmStatuses, err := r.List(ctx, omni.NewClusterMachineStatus(resources.DefaultNamespace, "").Metadata(),
8359
state.WithLabelQuery(resource.LabelEqual(
8460
omni.LabelCluster, cluster.Metadata().ID()),
@@ -88,44 +64,9 @@ func NewClusterDestroyStatusController() *ClusterDestroyStatusController {
8864
return err
8965
}
9066

91-
incrementRemainingMachines := func(cmStatus resource.Resource) {
92-
if msId, ok := cmStatus.Metadata().Labels().Get(omni.LabelMachineSet); ok {
93-
if _, ok = remainingMachineSetIDs[msId]; ok {
94-
remainingMachines++
95-
}
96-
}
97-
}
98-
99-
for _, cmStatus := range cmStatuses.Items {
100-
switch cmStatus.Metadata().Phase() {
101-
case resource.PhaseRunning:
102-
if !cmStatus.Metadata().Finalizers().Has(ClusterDestroyStatusControllerName) {
103-
if err = r.AddFinalizer(ctx, cmStatus.Metadata(), ClusterDestroyStatusControllerName); err != nil {
104-
return err
105-
}
106-
}
107-
incrementRemainingMachines(cmStatus)
108-
case resource.PhaseTearingDown:
109-
if cmStatus.Metadata().Finalizers().Has(ClusterDestroyStatusControllerName) {
110-
if hasOnlyDestroyStatusFinalizers(cmStatus.Metadata()) {
111-
if err = r.RemoveFinalizer(ctx, cmStatus.Metadata(), ClusterDestroyStatusControllerName); err != nil {
112-
return err
113-
}
114-
115-
continue
116-
}
117-
incrementRemainingMachines(cmStatus)
118-
}
119-
}
120-
}
121-
122-
if cluster.Metadata().Phase() != resource.PhaseTearingDown {
123-
return xerrors.NewTaggedf[qtransform.SkipReconcileTag]("not tearing down")
124-
}
125-
12667
clusterDestroyStatus.TypedSpec().Value.Phase = fmt.Sprintf("Destroying: %s, %s",
127-
pluralize.NewClient().Pluralize("machine set", len(remainingMachineSetIDs), true),
128-
pluralize.NewClient().Pluralize("machine", remainingMachines, true),
68+
pluralize.NewClient().Pluralize("machine set", len(msStatuses.Items), true),
69+
pluralize.NewClient().Pluralize("machine", len(cmStatuses.Items), true),
12970
)
13071

13172
return nil

‎internal/backend/runtime/omni/controllers/omni/cluster_destroy_status_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,12 @@ func TestClusterDestroyStatusController(t *testing.T) {
6969
asrt.Equal("Destroying: 1 machine set, 2 machines", res.TypedSpec().Value.Phase)
7070
})
7171

72+
rtestutils.Destroy[*omni.ClusterMachineStatus](ctx, t, st, []string{"cm0", "cm1"})
73+
74+
rtestutils.AssertResource(ctx, t, st, c.Metadata().ID(), func(res *omni.ClusterDestroyStatus, asrt *assert.Assertions) {
75+
asrt.Equal("Destroying: 1 machine set, 0 machines", res.TypedSpec().Value.Phase)
76+
})
77+
7278
rtestutils.Destroy[*omni.MachineSetStatus](ctx, t, st, []string{machineSet.Metadata().ID()})
7379

7480
rtestutils.AssertResource(ctx, t, st, c.Metadata().ID(), func(res *omni.ClusterDestroyStatus, asrt *assert.Assertions) {

‎internal/backend/runtime/omni/controllers/omni/machine_set_destroy_status.go‎

Lines changed: 5 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ package omni
88
import (
99
"context"
1010
"fmt"
11-
"slices"
1211

1312
"github.com/cosi-project/runtime/pkg/controller"
1413
"github.com/cosi-project/runtime/pkg/controller/generic/qtransform"
@@ -45,6 +44,10 @@ func NewMachineSetDestroyStatusController() *MachineSetDestroyStatusController {
4544
return omni.NewMachineSet(resources.DefaultNamespace, machineSetDestroyStatus.Metadata().ID())
4645
},
4746
TransformExtraOutputFunc: func(ctx context.Context, r controller.ReaderWriter, _ *zap.Logger, machineSet *omni.MachineSet, machineSetDestroyStatus *omni.MachineSetDestroyStatus) error {
47+
if machineSet.Metadata().Phase() != resource.PhaseTearingDown {
48+
return xerrors.NewTaggedf[qtransform.SkipReconcileTag]("not tearing down")
49+
}
50+
4851
cmStatuses, err := r.List(ctx, omni.NewClusterMachineStatus(resources.DefaultNamespace, "").Metadata(),
4952
state.WithLabelQuery(resource.LabelEqual(
5053
omni.LabelMachineSet, machineSet.Metadata().ID()),
@@ -54,35 +57,7 @@ func NewMachineSetDestroyStatusController() *MachineSetDestroyStatusController {
5457
return err
5558
}
5659

57-
remainingMachines := 0
58-
for _, cmStatus := range cmStatuses.Items {
59-
switch cmStatus.Metadata().Phase() {
60-
case resource.PhaseRunning:
61-
if !cmStatus.Metadata().Finalizers().Has(MachineSetDestroyStatusControllerName) {
62-
if err = r.AddFinalizer(ctx, cmStatus.Metadata(), MachineSetDestroyStatusControllerName); err != nil {
63-
return err
64-
}
65-
}
66-
remainingMachines++
67-
case resource.PhaseTearingDown:
68-
if cmStatus.Metadata().Finalizers().Has(MachineSetDestroyStatusControllerName) {
69-
if hasOnlyDestroyStatusFinalizers(cmStatus.Metadata()) {
70-
if err = r.RemoveFinalizer(ctx, cmStatus.Metadata(), MachineSetDestroyStatusControllerName); err != nil {
71-
return err
72-
}
73-
74-
continue
75-
}
76-
remainingMachines++
77-
}
78-
}
79-
}
80-
81-
if machineSet.Metadata().Phase() != resource.PhaseTearingDown {
82-
return xerrors.NewTaggedf[qtransform.SkipReconcileTag]("not tearing down")
83-
}
84-
85-
machineSetDestroyStatus.TypedSpec().Value.Phase = fmt.Sprintf("Destroying: %s", pluralize.NewClient().Pluralize("machine", remainingMachines, true))
60+
machineSetDestroyStatus.TypedSpec().Value.Phase = fmt.Sprintf("Destroying: %s", pluralize.NewClient().Pluralize("machine", len(cmStatuses.Items), true))
8661

8762
return nil
8863
},
@@ -91,16 +66,3 @@ func NewMachineSetDestroyStatusController() *MachineSetDestroyStatusController {
9166
qtransform.WithIgnoreTeardownUntil(),
9267
)
9368
}
94-
95-
// hasOnlyDestroyStatusFinalizers reports if ClusterMachineStatus resource has only specified DestroyStatusControllers* as finalizer.
96-
func hasOnlyDestroyStatusFinalizers(clusterMachineStatusMD *resource.Metadata) bool {
97-
destroyStatusControllers := []string{ClusterDestroyStatusControllerName, MachineSetDestroyStatusControllerName}
98-
99-
for _, fin := range *clusterMachineStatusMD.Finalizers() {
100-
if !slices.Contains(destroyStatusControllers, fin) { // there is a finalizer that is not a destroy status controller
101-
return false
102-
}
103-
}
104-
105-
return true
106-
}

‎internal/backend/runtime/omni/controllers/omni/machine_set_destroy_status_test.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,6 @@ func TestMachineSetDestroyStatusController(t *testing.T) {
6363
asrt.Equal("Destroying: 2 machines", res.TypedSpec().Value.Phase)
6464
})
6565

66-
rtestutils.AssertResource(ctx, t, st, "cm0", func(res *omni.ClusterMachineStatus, asrt *assert.Assertions) {
67-
asrt.True(res.Metadata().Finalizers().Has(omnictrl.NewMachineSetDestroyStatusController().Name()))
68-
})
69-
7066
rtestutils.Destroy[*omni.ClusterMachineStatus](ctx, t, st, []string{"cm0"})
7167

7268
rtestutils.AssertResource(ctx, t, st, machineSet.Metadata().ID(), func(res *omni.MachineSetDestroyStatus, asrt *assert.Assertions) {

‎internal/backend/runtime/omni/migration/manager.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,10 @@ func NewManager(state state.State, logger *zap.Logger) *Manager {
236236
callback: populateNodeUniqueTokens,
237237
name: "populateNodeUniqueTokens",
238238
},
239+
{
240+
callback: dropExtraInputFinalizers,
241+
name: "dropExtraInputFinalizers",
242+
},
239243
},
240244
}
241245
}

‎internal/backend/runtime/omni/migration/migration.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ package migration
88

99
import (
1010
"context"
11+
"fmt"
1112

13+
"github.com/cosi-project/runtime/pkg/controller/generic"
1214
"github.com/cosi-project/runtime/pkg/resource"
1315
"github.com/cosi-project/runtime/pkg/safe"
1416
"github.com/cosi-project/runtime/pkg/state"
@@ -50,3 +52,32 @@ func createOrUpdate[T resource.Resource](ctx context.Context, s state.State, res
5052

5153
return nil
5254
}
55+
56+
func dropFinalizers[R generic.ResourceWithRD](ctx context.Context, st state.State, finalizers ...resource.Finalizer) error {
57+
list, err := safe.StateListAll[R](ctx, st)
58+
if err != nil {
59+
return fmt.Errorf("failed to list resources: %w", err)
60+
}
61+
62+
for res := range list.All() {
63+
hasAny := false
64+
65+
for _, finalizer := range finalizers {
66+
if res.Metadata().Finalizers().Has(finalizer) {
67+
hasAny = true
68+
69+
break
70+
}
71+
}
72+
73+
if !hasAny {
74+
continue
75+
}
76+
77+
if err = st.RemoveFinalizer(ctx, res.Metadata(), finalizers...); err != nil {
78+
return fmt.Errorf("failed to remove finalizers from %s: %w", res.Metadata().ID(), err)
79+
}
80+
}
81+
82+
return nil
83+
}

‎internal/backend/runtime/omni/migration/migrations.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1918,3 +1918,19 @@ func populateNodeUniqueTokens(ctx context.Context, st state.State, _ *zap.Logger
19181918

19191919
return err
19201920
}
1921+
1922+
func dropExtraInputFinalizers(ctx context.Context, st state.State, logger *zap.Logger, _ migrationContext) error {
1923+
logger.Info("dropping extra finalizers from MachineSetStatus resources")
1924+
1925+
if err := dropFinalizers[*omni.MachineSetStatus](ctx, st, "ClusterDestroyStatusController"); err != nil {
1926+
return fmt.Errorf("failed to remove finalizers from MachineSetStatus resources: %w", err)
1927+
}
1928+
1929+
logger.Info("dropping extra finalizers from ClusterMachineStatus resources")
1930+
1931+
if err := dropFinalizers[*omni.ClusterMachineStatus](ctx, st, "ClusterDestroyStatusController", "MachineSetDestroyStatusController"); err != nil {
1932+
return fmt.Errorf("failed to remove finalizers from ClusterMachineStatus resources: %w", err)
1933+
}
1934+
1935+
return nil
1936+
}

0 commit comments

Comments
 (0)