Skip to content

Commit a26629d

Browse files
authored
Merge pull request #3492 from SimonTheLeg/deletion-relationship-workspace-logicalcluster
Make deletion relationship between workspace and logicalcluster one way
2 parents 79715d6 + f0f8ae4 commit a26629d

File tree

4 files changed

+364
-62
lines changed

4 files changed

+364
-62
lines changed

pkg/reconciler/core/logicalclusterdeletion/logicalcluster_deletion_controller.go

Lines changed: 8 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -20,16 +20,13 @@ import (
2020
"context"
2121
"errors"
2222
"fmt"
23-
"strings"
2423
"time"
2524

2625
apierrors "k8s.io/apimachinery/pkg/api/errors"
2726
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2827
"k8s.io/apimachinery/pkg/labels"
29-
"k8s.io/apimachinery/pkg/runtime/schema"
3028
utilerrors "k8s.io/apimachinery/pkg/util/errors"
3129
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
32-
"k8s.io/apimachinery/pkg/util/sets"
3330
"k8s.io/apimachinery/pkg/util/wait"
3431
"k8s.io/client-go/rest"
3532
"k8s.io/client-go/tools/cache"
@@ -262,7 +259,7 @@ func (c *Controller) process(ctx context.Context, key string) error {
262259
deleteErr = c.deleter.Delete(ctx, logicalClusterCopy)
263260
if deleteErr == nil {
264261
logger.V(4).Info("finished deleting logical cluster content", "duration", time.Since(startTime))
265-
return c.finalizeWorkspace(ctx, logicalClusterCopy)
262+
return c.finalizeLogicalCluster(ctx, logicalClusterCopy)
266263
}
267264

268265
errs := []error{deleteErr}
@@ -276,13 +273,13 @@ func (c *Controller) process(ctx context.Context, key string) error {
276273
return utilerrors.NewAggregate(errs)
277274
}
278275

279-
// finalizeNamespace removes the specified finalizer and finalizes the logical cluster.
280-
func (c *Controller) finalizeWorkspace(ctx context.Context, ws *corev1alpha1.LogicalCluster) error {
276+
// finalizeLogicalCluster removes the specified finalizer and finalizes the logical cluster.
277+
func (c *Controller) finalizeLogicalCluster(ctx context.Context, lc *corev1alpha1.LogicalCluster) error {
281278
logger := klog.FromContext(ctx)
282-
for i := range ws.Finalizers {
283-
if ws.Finalizers[i] == deletion.LogicalClusterDeletionFinalizer {
284-
ws.Finalizers = append(ws.Finalizers[:i], ws.Finalizers[i+1:]...)
285-
clusterName := logicalcluster.From(ws)
279+
for i := range lc.Finalizers {
280+
if lc.Finalizers[i] == deletion.LogicalClusterDeletionFinalizer {
281+
lc.Finalizers = append(lc.Finalizers[:i], lc.Finalizers[i+1:]...)
282+
clusterName := logicalcluster.From(lc)
286283

287284
// TODO(hasheddan): ClusterRole and ClusterRoleBinding cleanup
288285
// should be handled by garbage collection when the controller is
@@ -296,52 +293,8 @@ func (c *Controller) finalizeWorkspace(ctx context.Context, ws *corev1alpha1.Log
296293
return fmt.Errorf("could not delete clusterrolebindings for logical cluster %s: %w", clusterName, err)
297294
}
298295

299-
if ws.Spec.Owner != nil {
300-
gvr := schema.GroupVersionResource{
301-
Resource: ws.Spec.Owner.Resource,
302-
}
303-
comps := strings.SplitN(ws.Spec.Owner.APIVersion, "/", 2)
304-
if len(comps) == 2 {
305-
gvr.Group = comps[0]
306-
gvr.Version = comps[1]
307-
} else {
308-
gvr.Version = comps[0]
309-
}
310-
uid := ws.Spec.Owner.UID
311-
logger = logger.WithValues("owner.gvr", gvr, "owner.uid", uid, "owner.name", ws.Spec.Owner.Name, "owner.namespace", ws.Spec.Owner.Namespace, "owner.cluster", ws.Spec.Owner.Cluster)
312-
313-
// remove finalizer from owner
314-
logger.Info("checking owner for finalizer")
315-
clusterPath := logicalcluster.NewPath(ws.Spec.Owner.Cluster)
316-
obj, err := c.dynamicFrontProxyClient.Cluster(clusterPath).Resource(gvr).Namespace(ws.Spec.Owner.Namespace).Get(ctx, ws.Spec.Owner.Name, metav1.GetOptions{})
317-
if err != nil && !apierrors.IsNotFound(err) {
318-
return fmt.Errorf("could not get owner %s %s/%s in cluster %s: %w", gvr, ws.Spec.Owner.Namespace, ws.Spec.Owner.Name, ws.Spec.Owner.Cluster, err)
319-
} else if err == nil && obj.GetUID() != uid {
320-
logger.Info("owner has changed, skipping finalizer removal")
321-
return fmt.Errorf("could not get owner %s %s/%s in cluster %s is of wrong UID: %w", gvr, ws.Spec.Owner.Namespace, ws.Spec.Owner.Name, ws.Spec.Owner.Cluster, err)
322-
} else if err == nil {
323-
finalizers := sets.New[string](obj.GetFinalizers()...)
324-
if finalizers.Has(corev1alpha1.LogicalClusterFinalizer) {
325-
logger.Info("removing finalizer from owner")
326-
finalizers.Delete(corev1alpha1.LogicalClusterFinalizer)
327-
obj.SetFinalizers(sets.List[string](finalizers))
328-
if obj, err = c.dynamicFrontProxyClient.Cluster(clusterPath).Resource(gvr).Namespace(ws.Spec.Owner.Namespace).Update(ctx, obj, metav1.UpdateOptions{}); err != nil {
329-
return fmt.Errorf("could not remove finalizer from owner %s %s/%s in cluster %s: %w", gvr, ws.Spec.Owner.Namespace, ws.Spec.Owner.Name, ws.Spec.Owner.Cluster, err)
330-
}
331-
}
332-
333-
// delete owner
334-
if obj.GetDeletionTimestamp().IsZero() && ws.Spec.DirectlyDeletable {
335-
logger.Info("deleting owner")
336-
if err := c.dynamicFrontProxyClient.Cluster(clusterPath).Resource(gvr).Namespace(ws.Spec.Owner.Namespace).Delete(ctx, ws.Spec.Owner.Name, metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &uid}}); err != nil && !apierrors.IsNotFound(err) {
337-
return fmt.Errorf("could not delete owner %s %s/%s in cluster %s: %w", gvr, ws.Spec.Owner.Namespace, ws.Spec.Owner.Name, ws.Spec.Owner.Cluster, err)
338-
}
339-
}
340-
}
341-
}
342-
343296
logger.V(2).Info("removing finalizer from LogicalCluster")
344-
_, err := c.kcpClusterClient.CoreV1alpha1().LogicalClusters().Cluster(clusterName.Path()).Update(ctx, ws, metav1.UpdateOptions{})
297+
_, err := c.kcpClusterClient.CoreV1alpha1().LogicalClusters().Cluster(clusterName.Path()).Update(ctx, lc, metav1.UpdateOptions{})
345298
return err
346299
}
347300
}

pkg/reconciler/tenancy/workspace/workspace_reconcile_deletion.go

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,10 @@ func (r *deletionReconciler) reconcile(ctx context.Context, workspace *tenancyv1
4949
return reconcileStatusContinue, nil
5050
}
5151

52-
if sets.New[string](workspace.Finalizers...).Delete(corev1alpha1.LogicalClusterFinalizer).Len() > 0 {
52+
finSet := sets.New(workspace.Finalizers...)
53+
54+
// we want our finalizer to be removed last, so check if other finalizers exist
55+
if finSet.Has(corev1alpha1.LogicalClusterFinalizer) && finSet.Len() > 1 {
5356
return reconcileStatusContinue, nil
5457
}
5558

@@ -58,10 +61,10 @@ func (r *deletionReconciler) reconcile(ctx context.Context, workspace *tenancyv1
5861
clusterName := logicalcluster.Name(workspace.Spec.Cluster)
5962
if workspace.Status.Phase == corev1alpha1.LogicalClusterPhaseScheduling {
6063
a, ok := workspace.Annotations[workspaceClusterAnnotationKey]
64+
// if the logicalcluster was never created, we can directly remove the
65+
// workspace finalizer
6166
if !ok {
62-
finalizers := sets.New[string](workspace.Finalizers...)
63-
finalizers.Delete(corev1alpha1.LogicalClusterFinalizer)
64-
workspace.Finalizers = sets.List[string](finalizers)
67+
workspace.Finalizers = sets.List(finSet.Delete(corev1alpha1.LogicalClusterFinalizer))
6568
return reconcileStatusContinue, nil
6669
}
6770

@@ -97,10 +100,9 @@ func (r *deletionReconciler) reconcile(ctx context.Context, workspace *tenancyv1
97100
// fall-through
98101
}
99102
if apierrors.IsNotFound(getErr) {
100-
finalizers := sets.New[string](workspace.Finalizers...)
101-
if finalizers.Has(corev1alpha1.LogicalClusterFinalizer) {
103+
if finSet.Has(corev1alpha1.LogicalClusterFinalizer) {
102104
logger.Info(fmt.Sprintf("Removing finalizer %s", corev1alpha1.LogicalClusterFinalizer))
103-
workspace.Finalizers = sets.List[string](finalizers.Delete(corev1alpha1.LogicalClusterFinalizer))
105+
workspace.Finalizers = sets.List(finSet.Delete(corev1alpha1.LogicalClusterFinalizer))
104106
return reconcileStatusStopAndRequeue, nil // spec change
105107
}
106108
return reconcileStatusContinue, nil
Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
/*
2+
Copyright 2025 The KCP Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package workspace
18+
19+
import (
20+
"context"
21+
"slices"
22+
"testing"
23+
24+
"github.com/go-logr/logr"
25+
"github.com/google/go-cmp/cmp"
26+
27+
apierrors "k8s.io/apimachinery/pkg/api/errors"
28+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
29+
"k8s.io/apimachinery/pkg/runtime/schema"
30+
"k8s.io/utils/ptr"
31+
32+
"github.com/kcp-dev/logicalcluster/v3"
33+
34+
corev1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1"
35+
tenancyv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/tenancy/v1alpha1"
36+
kcpclientset "github.com/kcp-dev/kcp/sdk/client/clientset/versioned/cluster"
37+
)
38+
39+
type fakeDeletionReconciler struct {
40+
clusters map[string]*corev1alpha1.LogicalCluster
41+
deletionReconciler
42+
}
43+
44+
func (f *fakeDeletionReconciler) getLogicalCluster() func(ctx context.Context, cluster logicalcluster.Path) (*corev1alpha1.LogicalCluster, error) {
45+
return func(ctx context.Context, cluster logicalcluster.Path) (*corev1alpha1.LogicalCluster, error) {
46+
c, ok := f.clusters[cluster.String()]
47+
if !ok {
48+
return nil, apierrors.NewNotFound(schema.GroupResource{}, cluster.String())
49+
}
50+
return c, nil
51+
}
52+
}
53+
54+
func (f *fakeDeletionReconciler) deleteLogicalCluster() func(ctx context.Context, cluster logicalcluster.Path) error {
55+
return func(ctx context.Context, cluster logicalcluster.Path) error {
56+
delete(f.clusters, cluster.String())
57+
return nil
58+
}
59+
}
60+
61+
func (f *fakeDeletionReconciler) getShardByHash() func(hash string) (*corev1alpha1.Shard, error) {
62+
return func(hash string) (*corev1alpha1.Shard, error) {
63+
// for now we can work with a fixed shard
64+
s := &corev1alpha1.Shard{}
65+
return s, nil
66+
}
67+
}
68+
69+
func (f *fakeDeletionReconciler) kcpLogicalClusterAdminClientFor() func(shard *corev1alpha1.Shard) (kcpclientset.ClusterInterface, error) {
70+
return func(shard *corev1alpha1.Shard) (kcpclientset.ClusterInterface, error) {
71+
return nil, nil
72+
}
73+
}
74+
75+
func TestReconcile(t *testing.T) {
76+
tt := []struct {
77+
name string
78+
expStatus reconcileStatus
79+
workspace *tenancyv1alpha1.Workspace
80+
logicalclusters map[string]*corev1alpha1.LogicalCluster
81+
expFinalizers []string
82+
expLogicalClusters map[string]*corev1alpha1.LogicalCluster
83+
}{
84+
{
85+
name: "no deletion timestamp",
86+
expStatus: reconcileStatusContinue,
87+
workspace: &tenancyv1alpha1.Workspace{},
88+
},
89+
{
90+
name: "another finalizer is still set",
91+
expStatus: reconcileStatusContinue,
92+
workspace: &tenancyv1alpha1.Workspace{
93+
ObjectMeta: metav1.ObjectMeta{
94+
DeletionTimestamp: ptr.To(metav1.Now()),
95+
Finalizers: []string{
96+
corev1alpha1.LogicalClusterFinalizer,
97+
"other-finalizer",
98+
},
99+
},
100+
},
101+
expFinalizers: []string{
102+
corev1alpha1.LogicalClusterFinalizer,
103+
"other-finalizer",
104+
},
105+
},
106+
{
107+
name: "workspace is still initializing, no logicalcluster exists",
108+
expStatus: reconcileStatusContinue,
109+
workspace: &tenancyv1alpha1.Workspace{
110+
ObjectMeta: metav1.ObjectMeta{
111+
DeletionTimestamp: ptr.To(metav1.Now()),
112+
Finalizers: []string{
113+
corev1alpha1.LogicalClusterFinalizer,
114+
},
115+
},
116+
Status: tenancyv1alpha1.WorkspaceStatus{
117+
Phase: corev1alpha1.LogicalClusterPhaseScheduling,
118+
},
119+
},
120+
// we expect no finalizers here, as we can take a shortcut and directly
121+
// delete the workspace without needing to delete the logicalcluster first
122+
expFinalizers: []string{},
123+
},
124+
{
125+
name: "workspace is still initializing, logicalcluster not marked for deletion yet",
126+
expStatus: reconcileStatusContinue,
127+
workspace: &tenancyv1alpha1.Workspace{
128+
ObjectMeta: metav1.ObjectMeta{
129+
DeletionTimestamp: ptr.To(metav1.Now()),
130+
Finalizers: []string{
131+
corev1alpha1.LogicalClusterFinalizer,
132+
},
133+
Annotations: map[string]string{
134+
workspaceClusterAnnotationKey: "test",
135+
},
136+
},
137+
Status: tenancyv1alpha1.WorkspaceStatus{
138+
Phase: corev1alpha1.LogicalClusterPhaseScheduling,
139+
},
140+
},
141+
logicalclusters: map[string]*corev1alpha1.LogicalCluster{
142+
"test": {},
143+
},
144+
// we do not remove our finalizers in this case, as we wait
145+
// for another reconciliation loop
146+
expFinalizers: []string{
147+
corev1alpha1.LogicalClusterFinalizer,
148+
},
149+
// we do expect our logicalCluster to be removed
150+
expLogicalClusters: map[string]*corev1alpha1.LogicalCluster{},
151+
},
152+
{
153+
name: "workspace is marked for deletion, logicalcluster is already deleted",
154+
expStatus: reconcileStatusStopAndRequeue,
155+
workspace: &tenancyv1alpha1.Workspace{
156+
ObjectMeta: metav1.ObjectMeta{
157+
DeletionTimestamp: ptr.To(metav1.Now()),
158+
Finalizers: []string{
159+
corev1alpha1.LogicalClusterFinalizer,
160+
},
161+
},
162+
},
163+
// finalizer should be removed
164+
expFinalizers: []string{},
165+
},
166+
{
167+
name: "workspace is marked for deletion, finalizer is already removed (edge case)",
168+
expStatus: reconcileStatusContinue,
169+
workspace: &tenancyv1alpha1.Workspace{
170+
ObjectMeta: metav1.ObjectMeta{
171+
DeletionTimestamp: ptr.To(metav1.Now()),
172+
},
173+
},
174+
},
175+
}
176+
177+
// swallow any log output, so we don't pollute test results
178+
ctx := logr.NewContext(context.Background(), logr.Discard())
179+
180+
for _, tc := range tt {
181+
t.Run(tc.name, func(t *testing.T) {
182+
fdr := fakeDeletionReconciler{
183+
clusters: tc.logicalclusters,
184+
}
185+
fdr.deletionReconciler.getLogicalCluster = fdr.getLogicalCluster()
186+
fdr.deletionReconciler.deleteLogicalCluster = fdr.deleteLogicalCluster()
187+
fdr.deletionReconciler.getShardByHash = fdr.getShardByHash()
188+
fdr.deletionReconciler.kcpLogicalClusterAdminClientFor = fdr.kcpLogicalClusterAdminClientFor()
189+
190+
status, err := fdr.deletionReconciler.reconcile(ctx, tc.workspace)
191+
192+
if status != tc.expStatus {
193+
t.Errorf("Exp status %v, got %v", tc.expStatus, status)
194+
}
195+
196+
if !slices.Equal(tc.expFinalizers, tc.workspace.Finalizers) {
197+
t.Errorf("Exp finalizers %v, got %v", tc.expFinalizers, tc.workspace.Finalizers)
198+
}
199+
200+
if diff := cmp.Diff(tc.expLogicalClusters, tc.logicalclusters); diff != "" {
201+
t.Errorf("logicalcluster mismatch: %s", diff)
202+
}
203+
204+
if err != nil {
205+
t.Errorf("did not expect an error, but got %b", err)
206+
}
207+
})
208+
}
209+
}

0 commit comments

Comments
 (0)