Skip to content

Commit a6c14f6

Browse files
androndoandrondo
committed
fix(workloadmonitor): migrate ownerless legacy bucket Workloads and prefer legacy sizes on takeover
A BucketClaim deleted with --cascade=orphan and recreated leaves its bucket-bucket-<name> Workload without owners. The migration recognised a legacy Workload only by the claim's UID among its owners, so that object stayed next to the new one with a frozen size and the bucket was metered twice. Treat an ownerless legacy Workload that carries the monitor's label as the claim's: delete it with the same preconditions, but take no sizes from it, since they may predate the claim. The size carry-over kept whatever the Workload at the new name had recorded. When that Workload was not yet owned by the claim, an ownerless leftover, its sizes were not written for this bucket, yet they won over the migrating legacy Workload's. Prefer the recorded sizes only when the claim already owned the Workload. Assisted-by: LLM Signed-off-by: Andrey Kolkov <[email protected]>
1 parent 36ce21d commit a6c14f6

2 files changed

Lines changed: 63 additions & 15 deletions

File tree

‎internal/controller/workloadmonitor_controller.go‎

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -597,14 +597,18 @@ func bucketWorkloadName(claimName string) string {
597597
return "bucket-" + claimName
598598
}
599599

600-
// ownedBySomeoneElse reports whether w has owners and bc is not one of them.
601-
func ownedBySomeoneElse(w *cozyv1alpha1.Workload, bc *cosiv1alpha1.BucketClaim) bool {
600+
func ownedBy(w *cozyv1alpha1.Workload, bc *cosiv1alpha1.BucketClaim) bool {
602601
for _, ref := range w.OwnerReferences {
603602
if ref.UID == bc.UID {
604-
return false
603+
return true
605604
}
606605
}
607-
return len(w.OwnerReferences) > 0
606+
return false
607+
}
608+
609+
// ownedBySomeoneElse reports whether w has owners and bc is not one of them.
610+
func ownedBySomeoneElse(w *cozyv1alpha1.Workload, bc *cosiv1alpha1.BucketClaim) bool {
611+
return len(w.OwnerReferences) > 0 && !ownedBy(w, bc)
608612
}
609613

610614
// heldBySomeoneElse reports whether the Workload called name exists and has
@@ -618,9 +622,10 @@ func (r *WorkloadMonitorReconciler) heldBySomeoneElse(ctx context.Context, bc *c
618622
}
619623

620624
// legacyBucketWorkload returns the Workload a previous version created for bc
621-
// under the unconditionally prefixed name, if it is still there and bc is
622-
// among its owners.
623-
func (r *WorkloadMonitorReconciler) legacyBucketWorkload(ctx context.Context, bc *cosiv1alpha1.BucketClaim) (*cozyv1alpha1.Workload, error) {
625+
// under the unconditionally prefixed name, if it is still there and either bc
626+
// is among its owners or it has lost every owner (a claim deleted with
627+
// --cascade=orphan and recreated) while still carrying monitor's label.
628+
func (r *WorkloadMonitorReconciler) legacyBucketWorkload(ctx context.Context, monitor *cozyv1alpha1.WorkloadMonitor, bc *cosiv1alpha1.BucketClaim) (*cozyv1alpha1.Workload, error) {
624629
name := "bucket-" + bc.Name
625630
if name == bucketWorkloadName(bc.Name) {
626631
return nil, nil
@@ -629,10 +634,8 @@ func (r *WorkloadMonitorReconciler) legacyBucketWorkload(ctx context.Context, bc
629634
if err := r.Get(ctx, client.ObjectKey{Namespace: bc.Namespace, Name: name}, legacy); err != nil {
630635
return nil, client.IgnoreNotFound(err)
631636
}
632-
for _, ref := range legacy.OwnerReferences {
633-
if ref.UID == bc.UID {
634-
return legacy, nil
635-
}
637+
if ownedBy(legacy, bc) || (len(legacy.OwnerReferences) == 0 && legacy.Labels[workloadMonitorLabel] == monitor.Name) {
638+
return legacy, nil
636639
}
637640
return nil, nil
638641
}
@@ -692,17 +695,21 @@ func (r *WorkloadMonitorReconciler) reconcileBucketClaimForMonitor(
692695

693696
var legacy *cozyv1alpha1.Workload
694697
if migrate {
695-
if legacy, err = r.legacyBucketWorkload(ctx, &bc); err != nil {
698+
if legacy, err = r.legacyBucketWorkload(ctx, monitor, &bc); err != nil {
696699
return err
697700
}
698701
}
699702
// A legacy Workload with other owners is another bucket's renamed one that
700703
// a rolled-back controller also attached bc to: neither its sizes nor the
701-
// object are bc's to take.
704+
// object are bc's to take. An ownerless one is removed, but its sizes may
705+
// be older than the claim it is recognised for.
702706
soleLegacy := legacy != nil && len(legacy.OwnerReferences) == 1
707+
orphanLegacy := legacy != nil && len(legacy.OwnerReferences) == 0
703708

704709
monitorLabels := r.getMonitorLabels(monitor)
705710
_, err = ctrl.CreateOrUpdate(ctx, r.Client, workload, func() error {
711+
// sizes on a Workload bc did not own yet were not written for it
712+
ownedBefore := ownedBy(workload, &bc)
706713
updateOwnerReferences(workload.GetObjectMeta(), &bc)
707714

708715
if workload.Labels == nil {
@@ -731,7 +738,7 @@ func (r *WorkloadMonitorReconciler) reconcileBucketClaimForMonitor(
731738
// rename leaves no gap in the bucket's metered size.
732739
resources := make(map[string]resource.Quantity)
733740
for _, key := range []string{resourceS3StorageBytes, resourceS3PhysicalStorageBytes} {
734-
if q, ok := workload.Status.Resources[key]; ok {
741+
if q, ok := workload.Status.Resources[key]; ok && (ownedBefore || !soleLegacy) {
735742
resources[key] = q
736743
} else if soleLegacy {
737744
if q, ok := legacy.Status.Resources[key]; ok {
@@ -769,7 +776,7 @@ func (r *WorkloadMonitorReconciler) reconcileBucketClaimForMonitor(
769776
// preconditions delete only the object that was checked, not one that
770777
// took over the name since the lookup; a conflict is retried next pass.
771778
switch {
772-
case soleLegacy:
779+
case soleLegacy || orphanLegacy:
773780
err = r.Delete(ctx, legacy, client.Preconditions{UID: &legacy.UID, ResourceVersion: &legacy.ResourceVersion})
774781
case legacy != nil:
775782
orig := legacy.DeepCopy()

‎internal/controller/workloadmonitor_controller_test.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2934,3 +2934,44 @@ func TestReconcileBucketClaim_BothNamesHeldIsSkipped(t *testing.T) {
29342934
}
29352935
}
29362936
}
2937+
2938+
func orphanedLegacyWorkload(monitor string) *cozyv1alpha1.Workload {
2939+
w := legacyBucketWorkloadOwnedBy("gone-uid")
2940+
w.OwnerReferences = nil
2941+
w.Labels = map[string]string{workloadMonitorLabel: monitor}
2942+
return w
2943+
}
2944+
2945+
// A claim deleted with --cascade=orphan and recreated leaves its legacy
2946+
// Workload without owners; it must not be metered next to the new one.
2947+
func TestReconcileBucketApp_RemovesOrphanedLegacyWorkload(t *testing.T) {
2948+
c := reconcileBucketApp(t, orphanedLegacyWorkload("bucket-cozy-backups"))
2949+
2950+
err := c.Get(context.TODO(), types.NamespacedName{Name: "bucket-bucket-cozy-backups", Namespace: "tenant-demo"}, &cozyv1alpha1.Workload{})
2951+
if !apierrors.IsNotFound(err) {
2952+
t.Errorf("expected the orphaned legacy Workload to be deleted, got error %v", err)
2953+
}
2954+
if _, w := workloadSize(t, c, "bucket-cozy-backups"); len(w.Status.Resources) != 0 {
2955+
t.Errorf("sizes carried over from an orphaned legacy Workload: %v", w.Status.Resources)
2956+
}
2957+
}
2958+
2959+
func TestReconcileBucketApp_KeepsOrphanOfAnotherMonitor(t *testing.T) {
2960+
c := reconcileBucketApp(t, orphanedLegacyWorkload("some-other-monitor"))
2961+
2962+
if err := c.Get(context.TODO(), types.NamespacedName{Name: "bucket-bucket-cozy-backups", Namespace: "tenant-demo"}, &cozyv1alpha1.Workload{}); err != nil {
2963+
t.Errorf("expected another monitor's orphan to stay, got error %v", err)
2964+
}
2965+
}
2966+
2967+
// Sizes recorded on a Workload the claim did not own yet were not written for
2968+
// it, so the legacy Workload's sizes take precedence.
2969+
func TestReconcileBucketApp_LegacySizeWinsOverOwnerlessNewWorkload(t *testing.T) {
2970+
stale := bucketWorkloadWithSize("bucket-cozy-backups", "bucket-cozy-backups", "claim-uid", "10Gi")
2971+
stale.OwnerReferences = nil
2972+
c := reconcileBucketApp(t, legacyBucketWorkloadOwnedBy("claim-uid"), stale)
2973+
2974+
if got, _ := workloadSize(t, c, "bucket-cozy-backups"); got.Cmp(resource.MustParse("689Gi")) != 0 {
2975+
t.Errorf("size = %s, want the legacy 689Gi over the stale 10Gi", got.String())
2976+
}
2977+
}

0 commit comments

Comments
 (0)