Skip to content

Commit 6862d17

Browse files
ryanzhang-ossryanzhang-oss
authored
fix: continue to propagate PVC when workload is not enabled (#368)
1 parent 58b6de2 commit 6862d17

File tree

6 files changed

+89
-19
lines changed

6 files changed

+89
-19
lines changed

‎cmd/hubagent/workload/setup.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,7 @@ func SetupControllers(ctx context.Context, wg *sync.WaitGroup, mgr ctrl.Manager,
168168
UncachedReader: mgr.GetAPIReader(),
169169
ResourceSnapshotCreationMinimumInterval: opts.ResourceSnapshotCreationMinimumInterval,
170170
ResourceChangesCollectionDuration: opts.ResourceChangesCollectionDuration,
171+
EnableWorkload: opts.EnableWorkload,
171172
}
172173

173174
rateLimiter := options.DefaultControllerRateLimiter(opts.RateLimiterOpts)
@@ -507,6 +508,7 @@ func SetupControllers(ctx context.Context, wg *sync.WaitGroup, mgr ctrl.Manager,
507508
SkippedNamespaces: skippedNamespaces,
508509
ConcurrentPlacementWorker: int(math.Ceil(float64(opts.MaxConcurrentClusterPlacement) / 10)),
509510
ConcurrentResourceChangeWorker: opts.ConcurrentResourceChangeSyncs,
511+
EnableWorkload: opts.EnableWorkload,
510512
}
511513

512514
if err := mgr.Add(resourceChangeDetector); err != nil {

‎pkg/controllers/placement/controller.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ type Reconciler struct {
9494

9595
// ResourceChangesCollectionDuration is the duration for collecting resource changes into one snapshot.
9696
ResourceChangesCollectionDuration time.Duration
97+
98+
// EnableWorkload indicates whether workloads are allowed to run on the hub cluster.
99+
EnableWorkload bool
97100
}
98101

99102
func (r *Reconciler) Reconcile(ctx context.Context, key controller.QueueKey) (ctrl.Result, error) {

‎pkg/controllers/placement/resource_selector.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ func (r *Reconciler) shouldPropagateObj(namespace, placementName string, obj run
294294
return false, nil
295295
}
296296

297-
shouldInclude, err := utils.ShouldPropagateObj(r.InformerManager, uObj)
297+
shouldInclude, err := utils.ShouldPropagateObj(r.InformerManager, uObj, r.EnableWorkload)
298298
if err != nil {
299299
klog.ErrorS(err, "Cannot determine if we should propagate an object", "namespace", namespace, "placement", placementName, "object", uObjKObj)
300300
return false, err

‎pkg/resourcewatcher/change_dector.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ type ChangeDetector struct {
8484
// ConcurrentResourceChangeWorker is the number of resource change work that are
8585
// allowed to sync concurrently.
8686
ConcurrentResourceChangeWorker int
87+
88+
// EnableWorkload indicates whether workloads are allowed to run on the hub cluster.
89+
EnableWorkload bool
8790
}
8891

8992
// Start runs the detector, never stop until stopCh closed. This is called by the controller manager.
@@ -189,7 +192,7 @@ func (d *ChangeDetector) dynamicResourceFilter(obj interface{}) bool {
189192
}
190193

191194
if unstructuredObj, ok := obj.(*unstructured.Unstructured); ok {
192-
shouldPropagate, err := utils.ShouldPropagateObj(d.InformerManager, unstructuredObj.DeepCopy())
195+
shouldPropagate, err := utils.ShouldPropagateObj(d.InformerManager, unstructuredObj.DeepCopy(), d.EnableWorkload)
193196
if err != nil || !shouldPropagate {
194197
klog.V(5).InfoS("Skip watching resource in namespace", "namespace", cwKey.Namespace,
195198
"group", cwKey.Group, "version", cwKey.Version, "kind", cwKey.Kind, "object", cwKey.Name)

‎pkg/utils/common.go‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -505,29 +505,30 @@ func CheckCRDInstalled(discoveryClient discovery.DiscoveryInterface, gvk schema.
505505
return err
506506
}
507507

508-
// ShouldPropagateObj decides if one should propagate the object
509-
func ShouldPropagateObj(informerManager informer.Manager, uObj *unstructured.Unstructured) (bool, error) {
508+
// ShouldPropagateObj decides if one should propagate the object.
509+
// PVCs are only propagated when enableWorkload is false (workloads not allowed on hub).
510+
func ShouldPropagateObj(informerManager informer.Manager, uObj *unstructured.Unstructured, enableWorkload bool) (bool, error) {
510511
// TODO: add more special handling for different resource kind
511512
switch uObj.GroupVersionKind() {
512513
case appv1.SchemeGroupVersion.WithKind(ReplicaSetKind):
513-
// Skip ReplicaSets if they are managed by Deployments (have owner references)
514-
// Standalone ReplicaSets (without owners) can be propagated
514+
// Skip ReplicaSets if they are managed by Deployments (have owner references).
515+
// Standalone ReplicaSets (without owners) can be propagated.
515516
if len(uObj.GetOwnerReferences()) > 0 {
516517
return false, nil
517518
}
518519
case appv1.SchemeGroupVersion.WithKind("ControllerRevision"):
519-
// Skip ControllerRevisions if they are managed by DaemonSets/StatefulSets (have owner references)
520-
// These are automatically created by controllers and will be recreated on member clusters
520+
// Skip ControllerRevisions if they are managed by DaemonSets/StatefulSets (have owner references).
521+
// Standalone ControllerRevisions (without owners) can be propagated.
521522
if len(uObj.GetOwnerReferences()) > 0 {
522523
return false, nil
523524
}
524525
case corev1.SchemeGroupVersion.WithKind(ConfigMapKind):
525-
// Skip the built-in custom CA certificate created in the namespace
526+
// Skip the built-in custom CA certificate created in the namespace.
526527
if uObj.GetName() == "kube-root-ca.crt" {
527528
return false, nil
528529
}
529530
case corev1.SchemeGroupVersion.WithKind("ServiceAccount"):
530-
// Skip the default service account created in the namespace
531+
// Skip the default service account created in the namespace.
531532
if uObj.GetName() == "default" {
532533
return false, nil
533534
}
@@ -542,8 +543,11 @@ func ShouldPropagateObj(informerManager informer.Manager, uObj *unstructured.Uns
542543
return false, nil
543544
}
544545
case corev1.SchemeGroupVersion.WithKind("PersistentVolumeClaim"):
545-
// Skip PersistentVolumeClaims to avoid conflicts with the PVCs created by statefulset controller
546-
return false, nil
546+
// Skip PersistentVolumeClaims by default to avoid conflicts with the PVCs created by statefulset controller.
547+
// This only happens if the workloads are allowed to run on the hub cluster.
548+
if enableWorkload {
549+
return false, nil
550+
}
547551
case corev1.SchemeGroupVersion.WithKind("Endpoints"):
548552
// we assume that all endpoints with the same name of a service is created by the service controller
549553
if _, err := informerManager.Lister(ServiceGVR).ByNamespace(uObj.GetNamespace()).Get(uObj.GetName()); err != nil {

‎pkg/utils/common_test.go‎

Lines changed: 65 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,11 +1191,12 @@ func TestIsDiffedResourcePlacementEqual(t *testing.T) {
11911191
}
11921192
}
11931193

1194-
func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
1194+
func TestShouldPropagateObj(t *testing.T) {
11951195
tests := []struct {
11961196
name string
11971197
obj map[string]interface{}
11981198
ownerReferences []metav1.OwnerReference
1199+
enableWorkload bool
11991200
want bool
12001201
}{
12011202
{
@@ -1209,6 +1210,21 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
12091210
},
12101211
},
12111212
ownerReferences: nil,
1213+
enableWorkload: true,
1214+
want: true,
1215+
},
1216+
{
1217+
name: "standalone replicaset without ownerReferences should propagate if workload is disabled",
1218+
obj: map[string]interface{}{
1219+
"apiVersion": "apps/v1",
1220+
"kind": "ReplicaSet",
1221+
"metadata": map[string]interface{}{
1222+
"name": "standalone-rs",
1223+
"namespace": "default",
1224+
},
1225+
},
1226+
ownerReferences: nil,
1227+
enableWorkload: false,
12121228
want: true,
12131229
},
12141230
{
@@ -1222,6 +1238,7 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
12221238
},
12231239
},
12241240
ownerReferences: nil,
1241+
enableWorkload: true,
12251242
want: true,
12261243
},
12271244
{
@@ -1242,7 +1259,8 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
12421259
UID: "12345",
12431260
},
12441261
},
1245-
want: false,
1262+
enableWorkload: true,
1263+
want: false,
12461264
},
12471265
{
12481266
name: "pod owned by replicaset - passes ShouldPropagateObj but filtered by resource config",
@@ -1262,7 +1280,8 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
12621280
UID: "67890",
12631281
},
12641282
},
1265-
want: true, // ShouldPropagateObj doesn't filter Pods - they're filtered by NewResourceConfig
1283+
enableWorkload: false,
1284+
want: true, // ShouldPropagateObj doesn't filter Pods - they're filtered by NewResourceConfig
12661285
},
12671286
{
12681287
name: "controllerrevision owned by daemonset should NOT propagate",
@@ -1282,7 +1301,8 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
12821301
UID: "abcdef",
12831302
},
12841303
},
1285-
want: false,
1304+
enableWorkload: false,
1305+
want: false,
12861306
},
12871307
{
12881308
name: "controllerrevision owned by statefulset should NOT propagate",
@@ -1302,7 +1322,8 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
13021322
UID: "fedcba",
13031323
},
13041324
},
1305-
want: false,
1325+
enableWorkload: false,
1326+
want: false,
13061327
},
13071328
{
13081329
name: "standalone controllerrevision without owner should propagate",
@@ -1315,10 +1336,25 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
13151336
},
13161337
},
13171338
ownerReferences: nil,
1339+
enableWorkload: false,
1340+
want: true,
1341+
},
1342+
{
1343+
name: "PVC should propagate when workload is disabled",
1344+
obj: map[string]interface{}{
1345+
"apiVersion": "v1",
1346+
"kind": "PersistentVolumeClaim",
1347+
"metadata": map[string]interface{}{
1348+
"name": "test-pvc",
1349+
"namespace": "default",
1350+
},
1351+
},
1352+
ownerReferences: nil,
1353+
enableWorkload: false,
13181354
want: true,
13191355
},
13201356
{
1321-
name: "PersistentVolumeClaim should NOT propagate",
1357+
name: "PVC should NOT propagate when workload is enabled",
13221358
obj: map[string]interface{}{
13231359
"apiVersion": "v1",
13241360
"kind": "PersistentVolumeClaim",
@@ -1328,8 +1364,30 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
13281364
},
13291365
},
13301366
ownerReferences: nil,
1367+
enableWorkload: true,
13311368
want: false,
13321369
},
1370+
{
1371+
name: "PVC with ownerReferences should NOT propagate when workload is enabled",
1372+
obj: map[string]interface{}{
1373+
"apiVersion": "v1",
1374+
"kind": "PersistentVolumeClaim",
1375+
"metadata": map[string]interface{}{
1376+
"name": "data-statefulset-0",
1377+
"namespace": "default",
1378+
},
1379+
},
1380+
ownerReferences: []metav1.OwnerReference{
1381+
{
1382+
APIVersion: "apps/v1",
1383+
Kind: "StatefulSet",
1384+
Name: "statefulset",
1385+
UID: "sts-uid",
1386+
},
1387+
},
1388+
enableWorkload: true,
1389+
want: false,
1390+
},
13331391
}
13341392

13351393
for _, tt := range tests {
@@ -1339,7 +1397,7 @@ func TestShouldPropagateObj_PodAndReplicaSet(t *testing.T) {
13391397
uObj.SetOwnerReferences(tt.ownerReferences)
13401398
}
13411399

1342-
got, err := ShouldPropagateObj(nil, uObj)
1400+
got, err := ShouldPropagateObj(nil, uObj, tt.enableWorkload)
13431401
if err != nil {
13441402
t.Errorf("ShouldPropagateObj() error = %v", err)
13451403
return

0 commit comments

Comments
 (0)