Skip to content

Commit 0370690

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 0a30aea commit 0370690

File tree

10 files changed

+101
-157
lines changed

10 files changed

+101
-157
lines changed

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

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,39 +10,38 @@ import (
1010

1111
"github.com/cosi-project/runtime/pkg/controller"
1212
"github.com/cosi-project/runtime/pkg/controller/generic/qtransform"
13+
"github.com/cosi-project/runtime/pkg/safe"
14+
"github.com/cosi-project/runtime/pkg/state"
1315
"github.com/siderolabs/gen/xerrors"
1416
"go.uber.org/zap"
1517

1618
"github.com/siderolabs/omni/client/pkg/omni/resources/infra"
1719
"github.com/siderolabs/omni/client/pkg/omni/resources/omni"
1820
"github.com/siderolabs/omni/client/pkg/omni/resources/siderolink"
19-
"github.com/siderolabs/omni/internal/backend/runtime/omni/controllers/helpers"
2021
)
2122

2223
// BMCConfigController manages BMCConfig resource lifecycle.
2324
type BMCConfigController = qtransform.QController[*omni.InfraMachineBMCConfig, *infra.BMCConfig]
2425

2526
// NewBMCConfigController initializes BMCConfigController.
2627
func NewBMCConfigController() *BMCConfigController {
27-
name := "BMCConfigController"
28-
2928
return qtransform.NewQController(
3029
qtransform.Settings[*omni.InfraMachineBMCConfig, *infra.BMCConfig]{
31-
Name: name,
30+
Name: "BMCConfigController",
3231
MapMetadataFunc: func(config *omni.InfraMachineBMCConfig) *infra.BMCConfig {
3332
return infra.NewBMCConfig(config.Metadata().ID())
3433
},
3534
UnmapMetadataFunc: func(config *infra.BMCConfig) *omni.InfraMachineBMCConfig {
3635
return omni.NewInfraMachineBMCConfig(config.Metadata().ID())
3736
},
38-
TransformExtraOutputFunc: func(ctx context.Context, r controller.ReaderWriter, _ *zap.Logger, userConfig *omni.InfraMachineBMCConfig, config *infra.BMCConfig) error {
39-
link, err := helpers.HandleInput[*siderolink.Link](ctx, r, name, userConfig)
37+
TransformFunc: func(ctx context.Context, r controller.Reader, _ *zap.Logger, userConfig *omni.InfraMachineBMCConfig, config *infra.BMCConfig) error {
38+
link, err := safe.ReaderGetByID[*siderolink.Link](ctx, r, userConfig.Metadata().ID())
4039
if err != nil {
41-
return err
42-
}
40+
if state.IsNotFoundError(err) {
41+
return xerrors.NewTaggedf[qtransform.SkipReconcileTag]("no matching link found")
42+
}
4343

44-
if link == nil {
45-
return xerrors.NewTaggedf[qtransform.SkipReconcileTag]("no matching link found")
44+
return err
4645
}
4746

4847
providerID, ok := link.Metadata().Annotations().Get(omni.LabelInfraProviderID)
@@ -54,13 +53,6 @@ func NewBMCConfigController() *BMCConfigController {
5453

5554
config.TypedSpec().Value.Config = userConfig.TypedSpec().Value
5655

57-
return nil
58-
},
59-
FinalizerRemovalExtraOutputFunc: func(ctx context.Context, writer controller.ReaderWriter, _ *zap.Logger, userConfig *omni.InfraMachineBMCConfig) error {
60-
if _, err := helpers.HandleInput[*siderolink.Link](ctx, writer, name, userConfig); err != nil {
61-
return err
62-
}
63-
6456
return nil
6557
},
6658
},

‎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/cluster_machine_status.go‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,14 @@ func NewClusterMachineStatusController() *ClusterMachineStatusController {
4141
UnmapMetadataFunc: func(clusterMachineStatus *omni.ClusterMachineStatus) *omni.ClusterMachine {
4242
return omni.NewClusterMachine(resources.DefaultNamespace, clusterMachineStatus.Metadata().ID())
4343
},
44-
TransformExtraOutputFunc: func(ctx context.Context, r controller.ReaderWriter, _ *zap.Logger, clusterMachine *omni.ClusterMachine, clusterMachineStatus *omni.ClusterMachineStatus) error {
44+
TransformFunc: func(ctx context.Context, r controller.Reader, _ *zap.Logger, clusterMachine *omni.ClusterMachine, clusterMachineStatus *omni.ClusterMachineStatus) error {
4545
machine, err := safe.ReaderGet[*omni.Machine](ctx, r, resource.NewMetadata(resources.DefaultNamespace, omni.MachineType, clusterMachine.Metadata().ID(), resource.VersionUndefined))
4646
if err != nil && !state.IsNotFoundError(err) {
4747
return err
4848
}
4949

50-
machineSetNode, err := helpers.HandleInput[*omni.MachineSetNode](ctx, r, clusterMachineStatusControllerName, clusterMachine)
51-
if err != nil {
50+
machineSetNode, err := safe.ReaderGetByID[*omni.MachineSetNode](ctx, r, clusterMachine.Metadata().ID())
51+
if err != nil && !state.IsNotFoundError(err) {
5252
return err
5353
}
5454

@@ -224,14 +224,6 @@ func NewClusterMachineStatusController() *ClusterMachineStatusController {
224224
clusterMachineStatus.Metadata().Labels().Set(omni.ClusterMachineStatusLabelNodeName, clusterMachineIdentity.TypedSpec().Value.Nodename)
225225
}
226226

227-
return nil
228-
},
229-
FinalizerRemovalExtraOutputFunc: func(ctx context.Context, rw controller.ReaderWriter, _ *zap.Logger, clusterMachine *omni.ClusterMachine) error {
230-
_, err := helpers.HandleInput[*omni.MachineSetNode](ctx, rw, clusterMachineStatusControllerName, clusterMachine)
231-
if err != nil {
232-
return err
233-
}
234-
235227
return nil
236228
},
237229
},

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

Lines changed: 6 additions & 44 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"
@@ -44,7 +43,11 @@ func NewMachineSetDestroyStatusController() *MachineSetDestroyStatusController {
4443
UnmapMetadataFunc: func(machineSetDestroyStatus *omni.MachineSetDestroyStatus) *omni.MachineSet {
4544
return omni.NewMachineSet(resources.DefaultNamespace, machineSetDestroyStatus.Metadata().ID())
4645
},
47-
TransformExtraOutputFunc: func(ctx context.Context, r controller.ReaderWriter, _ *zap.Logger, machineSet *omni.MachineSet, machineSetDestroyStatus *omni.MachineSetDestroyStatus) error {
46+
TransformFunc: func(ctx context.Context, r controller.Reader, _ *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/controllers/omni/machine_status_snapshot.go‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -106,15 +106,6 @@ func (ctrl *MachineStatusSnapshotController) Settings() controller.QSettings {
106106
func (ctrl *MachineStatusSnapshotController) MapInput(ctx context.Context, _ *zap.Logger,
107107
r controller.QRuntime, ptr controller.ReducedResourceMetadata,
108108
) ([]resource.Pointer, error) {
109-
_, err := r.Get(ctx, ptr)
110-
if err != nil {
111-
if state.IsNotFoundError(err) {
112-
return nil, nil
113-
}
114-
115-
return nil, err
116-
}
117-
118109
switch ptr.Type() {
119110
case omni.ClusterMachineType:
120111
fallthrough
@@ -167,8 +158,8 @@ func (ctrl *MachineStatusSnapshotController) reconcileRunning(ctx context.Contex
167158
}
168159
}
169160

170-
clusterMachine, err := helpers.HandleInput[*omni.ClusterMachine](ctx, r, ctrl.Name(), machine)
171-
if err != nil {
161+
clusterMachine, err := safe.ReaderGetByID[*omni.ClusterMachine](ctx, r, machine.Metadata().ID())
162+
if err != nil && !state.IsNotFoundError(err) {
172163
return err
173164
}
174165

@@ -202,11 +193,6 @@ func (ctrl *MachineStatusSnapshotController) reconcileRunning(ctx context.Contex
202193
func (ctrl *MachineStatusSnapshotController) reconcileTearingDown(ctx context.Context, r controller.QRuntime, logger *zap.Logger, machine *omni.Machine) error {
203194
ctrl.runner.StopTask(logger, machine.Metadata().ID())
204195

205-
_, err := helpers.HandleInput[*omni.ClusterMachine](ctx, r, ctrl.Name(), machine)
206-
if err != nil {
207-
return err
208-
}
209-
210196
md := omni.NewMachineStatusSnapshot(resources.DefaultNamespace, machine.Metadata().ID()).Metadata()
211197

212198
ready, err := helpers.TeardownAndDestroy(ctx, r, md)

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,10 @@ func NewManager(state state.State, logger *zap.Logger) *Manager {
240240
callback: moveClusterTaintFromResourceToLabel,
241241
name: "moveClusterTaintFromResourceToLabel",
242242
},
243+
{
244+
callback: dropExtraInputFinalizers,
245+
name: "dropExtraInputFinalizers",
246+
},
243247
},
244248
}
245249
}

‎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+
}

0 commit comments

Comments
 (0)