From 41cc709b7c4cf588d97884a982dc73babf3cfff4 Mon Sep 17 00:00:00 2001 From: "yuyinglu.yyl" Date: Wed, 25 Mar 2026 11:25:15 +0800 Subject: [PATCH 1/2] fix(synccontrols): implement proper lifecycle for XSet deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 修改 BatchDeleteTargetsByLabel 函数，使其遵循与 scale-in 相同的生命周期模式： 1. 触发 TargetOpsLifecycle (opslifecycle.Begin) 2. 等待操作许可 (opslifecycle.AllowOps) 3. 直接删除目标 (DeleteTarget) 4. 清理 PVC 资源 之前该函数仅添加 to-delete 标签，需要额外的控制器来处理删除。 现在直接处理删除，与 scale-in 保持一致。 --- synccontrols/sync_control.go | 77 +++++++++++++++++++++++++++++++++--- 1 file changed, 72 insertions(+), 5 deletions(-) diff --git a/synccontrols/sync_control.go b/synccontrols/sync_control.go index 3d4ac07..98cf586 100644 --- a/synccontrols/sync_control.go +++ b/synccontrols/sync_control.go @@ -1057,16 +1057,83 @@ func targetDuringReplace(labelMgr api.XSetLabelAnnotationManager, target client. return replaceIndicate || replaceOriginTarget || replaceNewTarget } -// BatchDeleteTargetsByLabel try to trigger target deletion by to-delete label +// BatchDeleteTargetsByLabel triggers target deletion following the same lifecycle pattern as scale-in. +// It triggers TargetOpsLifecycle, waits for permission, then directly deletes the targets. func (r *RealSyncControl) BatchDeleteTargetsByLabel(ctx context.Context, targetControl xcontrol.TargetControl, needDeleteTargets []client.Object) error { + logger := logr.FromContext(ctx) + + // Step 1: Trigger TargetOpsLifecycle for targets not already in lifecycle _, err := controllerutils.SlowStartBatch(len(needDeleteTargets), controllerutils.SlowStartInitialBatchSize, false, func(i int, _ error) error { target := needDeleteTargets[i] - if _, exist := r.xsetLabelAnnoMgr.Get(target, api.XDeletionIndicationLabelKey); !exist { - patch := client.RawPatch(types.MergePatchType, []byte(fmt.Sprintf(`{"metadata":{"labels":{"%s":"%d"}}}`, r.xsetLabelAnnoMgr.Value(api.XDeletionIndicationLabelKey), time.Now().UnixNano()))) // nolint - if err := targetControl.PatchTarget(ctx, target, patch); err != nil { - return fmt.Errorf("failed to delete target when syncTargets %s/%s/%w", target.GetNamespace(), target.GetName(), err) + + // Skip if already being deleted + if target.GetDeletionTimestamp() != nil { + return nil + } + + // Check if already during scale-in ops (has preparing-delete label) + if _, duringOps := r.xsetLabelAnnoMgr.Get(target, api.PreparingDeleteLabel); duringOps { + return nil + } + + // Trigger TargetOpsLifecycle with scaleIn OperationType + logger.V(1).Info("try to begin TargetOpsLifecycle for deleting Target in XSet", "target", ObjectKeyString(target)) + if updated, err := opslifecycle.Begin(ctx, r.xsetLabelAnnoMgr, r.Client, r.scaleInLifecycleAdapter, target); err != nil { + return fmt.Errorf("fail to begin TargetOpsLifecycle for deleting Target %s/%s: %w", target.GetNamespace(), target.GetName(), err) + } else if updated { + r.Recorder.Eventf(target, corev1.EventTypeNormal, "BeginDeleteLifecycle", "succeed to begin TargetOpsLifecycle for deletion") + // add an expectation for this target update, before next reconciling + if err := r.cacheExpectations.ExpectUpdation(clientutil.ObjectKeyString(target), r.targetGVK, target.GetNamespace(), target.GetName(), target.GetResourceVersion()); err != nil { + return err } } + + return nil + }) + if err != nil { + return err + } + + // Step 2: Check AllowOps and delete targets that are allowed + _, err = controllerutils.SlowStartBatch(len(needDeleteTargets), controllerutils.SlowStartInitialBatchSize, false, func(i int, _ error) error { + target := needDeleteTargets[i] + + // Skip if already being deleted + if target.GetDeletionTimestamp() != nil { + return nil + } + + // Check if operation is allowed (no delay for deletion, pass 0) + _, allowed := opslifecycle.AllowOps(r.xsetLabelAnnoMgr, r.scaleInLifecycleAdapter, 0, target) + if !allowed { + logger.V(1).Info("target not yet allowed to delete, waiting for lifecycle", "target", ObjectKeyString(target)) + return nil + } + + // Delete the target + logger.Info("deleting target for XSet deletion", "target", ObjectKeyString(target)) + if err := targetControl.DeleteTarget(ctx, target); err != nil { + return fmt.Errorf("failed to delete target %s/%s: %w", target.GetNamespace(), target.GetName(), err) + } + + r.Recorder.Eventf(target, corev1.EventTypeNormal, "TargetDeleted", "succeed to delete target for XSet deletion") + if err := r.cacheExpectations.ExpectDeletion(clientutil.ObjectKeyString(target), r.targetGVK, target.GetNamespace(), target.GetName()); err != nil { + return err + } + + // Clean up PVCs if needed (same logic as scale-in) + if _, enabled := subresources.GetSubresourcePvcAdapter(r.xsetController); enabled { + _, replaceOrigin := r.xsetLabelAnnoMgr.Get(target, api.XReplacePairOriginName) + _, replaceNew := r.xsetLabelAnnoMgr.Get(target, api.XReplacePairNewId) + if replaceOrigin || replaceNew || !r.pvcControl.RetainPvcWhenXSetDeleted(r.xsetController.NewXSetObject()) { + // Note: For deletion, we don't have syncContext.ExistingPvcs, so pass nil + // The pvcControl should handle this case + if err := r.pvcControl.DeleteTargetPvcs(ctx, r.xsetController.NewXSetObject(), target, nil); err != nil { + logger.Error(err, "failed to delete target PVCs", "target", ObjectKeyString(target)) + } + } + } + return nil }) return err From 4c9abea6f1fc21913d97e9d58ace6db9ea23f330 Mon Sep 17 00:00:00 2001 From: "yuyinglu.yyl" Date: Wed, 25 Mar 2026 11:49:22 +0800 Subject: [PATCH 2/2] fix(synccontrols): remove redundant PVC deletion from BatchDeleteTargetsByLabel PVC cleanup is already handled by ensureReclaimPvcs in xset_controller.go before BatchDeleteTargetsByLabel is called. The previous PVC deletion code was broken because DeleteTargetPvcs requires the pvcs parameter which was nil. Co-Authored-By: Claude Opus 4.6 --- synccontrols/sync_control.go | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/synccontrols/sync_control.go b/synccontrols/sync_control.go index 98cf586..e532a8d 100644 --- a/synccontrols/sync_control.go +++ b/synccontrols/sync_control.go @@ -1059,6 +1059,7 @@ func targetDuringReplace(labelMgr api.XSetLabelAnnotationManager, target client. // BatchDeleteTargetsByLabel triggers target deletion following the same lifecycle pattern as scale-in. // It triggers TargetOpsLifecycle, waits for permission, then directly deletes the targets. +// Note: PVC cleanup is handled separately by ensureReclaimPvcs in xset_controller.go. func (r *RealSyncControl) BatchDeleteTargetsByLabel(ctx context.Context, targetControl xcontrol.TargetControl, needDeleteTargets []client.Object) error { logger := logr.FromContext(ctx) @@ -1121,19 +1122,6 @@ func (r *RealSyncControl) BatchDeleteTargetsByLabel(ctx context.Context, targetC return err } - // Clean up PVCs if needed (same logic as scale-in) - if _, enabled := subresources.GetSubresourcePvcAdapter(r.xsetController); enabled { - _, replaceOrigin := r.xsetLabelAnnoMgr.Get(target, api.XReplacePairOriginName) - _, replaceNew := r.xsetLabelAnnoMgr.Get(target, api.XReplacePairNewId) - if replaceOrigin || replaceNew || !r.pvcControl.RetainPvcWhenXSetDeleted(r.xsetController.NewXSetObject()) { - // Note: For deletion, we don't have syncContext.ExistingPvcs, so pass nil - // The pvcControl should handle this case - if err := r.pvcControl.DeleteTargetPvcs(ctx, r.xsetController.NewXSetObject(), target, nil); err != nil { - logger.Error(err, "failed to delete target PVCs", "target", ObjectKeyString(target)) - } - } - } - return nil }) return err