Skip to content

Commit d50385f

Browse files
androndoandrondo
committed
fix(backups): tailor the Velero rename refusal to VMDisk and in-flight jobs
The refusal pointed every kind at "the application's engine-native strategy", but VMDisk, the only non-VMInstance kind the default BackupClass routes to Velero, has none. Offer that remedy only to kinds that can have one. The check also runs on every reconcile, so a rename RestoreJob that was already Running when the controller picked up the check is failed after its Velero Restore has completed. That job now says the application may already sit in the target namespace under its source name (a retry that drops the rename would otherwise trip failIfTargetExists on that copy without explanation), and its resource-modifier ConfigMap is released right away instead of at the job's deletion. Assisted-By: LLM Signed-off-by: Andrey Kolkov <[email protected]>
1 parent 11bac54 commit d50385f

2 files changed

Lines changed: 146 additions & 4 deletions

File tree

‎internal/backupcontroller/velerostrategy_controller.go‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -609,12 +609,22 @@ func (r *RestoreJobReconciler) reconcileVeleroRestore(ctx context.Context, resto
609609
// Validate: rename is only implemented for VMInstance. Failing here beats a
610610
// Succeeded RestoreJob whose application silently keeps the source name.
611611
if target.IsRenamed && !veleroRenameSupported(backup.Spec.ApplicationRef.Kind) {
612-
return r.markRestoreJobFailed(ctx, restoreJob, fmt.Sprintf(
612+
msg := fmt.Sprintf(
613613
"restoring %s %q under a different name (%q) is not supported by the Velero strategy: "+
614614
"Velero DataUpload always restores volumes under their original names and only a VMInstance "+
615-
"can adopt them. Omit targetApplicationRef.name to keep the source name, "+
616-
"or use the application's engine-native strategy to restore into a new application",
617-
backup.Spec.ApplicationRef.Kind, backup.Spec.ApplicationRef.Name, target.AppName))
615+
"can adopt them. Omit targetApplicationRef.name to keep the source name",
616+
backup.Spec.ApplicationRef.Kind, backup.Spec.ApplicationRef.Name, target.AppName)
617+
if backup.Spec.ApplicationRef.Kind != vmDiskAppKind {
618+
msg += ", or use the application's engine-native strategy to restore into a new application"
619+
}
620+
if restoreJob.Status.StartedAt != nil {
621+
// The job predates this check: its Velero Restore may already have
622+
// restored the application under the source name.
623+
r.cleanupResourceModifierConfigMaps(ctx, restoreJob)
624+
msg += fmt.Sprintf(". This RestoreJob was already running, so %q may have been restored into namespace %q under its source name",
625+
backup.Spec.ApplicationRef.Name, target.Namespace)
626+
}
627+
return r.markRestoreJobFailed(ctx, restoreJob, msg)
618628
}
619629

620630
// Step 1: On first reconcile, set startedAt and phase = Running

‎internal/backupcontroller/velerostrategy_controller_test.go‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"testing"
88

99
corev1 "k8s.io/api/core/v1"
10+
"k8s.io/apimachinery/pkg/api/errors"
1011
"k8s.io/apimachinery/pkg/api/meta"
1112
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1213
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -1163,6 +1164,7 @@ func TestReconcileVeleroRestore_RenameGuard(t *testing.T) {
11631164
targetNS string
11641165
wantPhase backupsv1alpha1.RestoreJobPhase
11651166
wantMessage string
1167+
wantMissing string // a remedy the message must not offer for this kind
11661168
}{
11671169
{
11681170
name: "postgres cross-namespace rename is rejected",
@@ -1172,13 +1174,22 @@ func TestReconcileVeleroRestore_RenameGuard(t *testing.T) {
11721174
wantPhase: backupsv1alpha1.RestoreJobPhaseFailed,
11731175
wantMessage: `restoring Postgres "src" under a different name ("dst") is not supported by the Velero strategy`,
11741176
},
1177+
{
1178+
name: "postgres rejection points at the engine-native strategy",
1179+
appKind: "Postgres",
1180+
targetName: "dst",
1181+
targetNS: targetNS,
1182+
wantPhase: backupsv1alpha1.RestoreJobPhaseFailed,
1183+
wantMessage: "engine-native strategy",
1184+
},
11751185
{
11761186
name: "vmdisk cross-namespace rename is rejected",
11771187
appKind: vmDiskAppKind,
11781188
targetName: "dst",
11791189
targetNS: targetNS,
11801190
wantPhase: backupsv1alpha1.RestoreJobPhaseFailed,
11811191
wantMessage: `restoring VMDisk "src" under a different name ("dst") is not supported by the Velero strategy`,
1192+
wantMissing: "engine-native strategy",
11821193
},
11831194
{
11841195
name: "vmdisk rename is rejected by the backup kind, not the target kind",
@@ -1282,6 +1293,127 @@ func TestReconcileVeleroRestore_RenameGuard(t *testing.T) {
12821293
if !strings.Contains(ready.Message, tt.wantMessage) {
12831294
t.Errorf("Ready message = %q, want it to contain %q", ready.Message, tt.wantMessage)
12841295
}
1296+
if tt.wantMissing != "" && strings.Contains(ready.Message, tt.wantMissing) {
1297+
t.Errorf("Ready message = %q, must not offer %q", ready.Message, tt.wantMissing)
1298+
}
1299+
})
1300+
}
1301+
}
1302+
1303+
// TestReconcileVeleroRestore_RenameGuard_InFlight covers a rename RestoreJob
1304+
// that was already Running when the guard was introduced: its Velero Restore
1305+
// has completed under the source name, so the job must fail with a message
1306+
// saying so and release its resource-modifier ConfigMap instead of waiting
1307+
// for the job's deletion. A VMInstance in the same state still succeeds.
1308+
func TestReconcileVeleroRestore_RenameGuard_InFlight(t *testing.T) {
1309+
const (
1310+
sourceNS = "tenant-src"
1311+
targetNS = "tenant-dst"
1312+
)
1313+
tests := []struct {
1314+
name string
1315+
appKind string
1316+
wantPhase backupsv1alpha1.RestoreJobPhase
1317+
wantMessage string
1318+
}{
1319+
{
1320+
name: "vmdisk in-flight rename fails and names the source-name copy",
1321+
appKind: vmDiskAppKind,
1322+
wantPhase: backupsv1alpha1.RestoreJobPhaseFailed,
1323+
wantMessage: `This RestoreJob was already running, so "src" may have been restored into namespace "tenant-dst" under its source name`,
1324+
},
1325+
{
1326+
name: "vminstance in-flight rename still succeeds",
1327+
appKind: vmInstanceKind,
1328+
wantPhase: backupsv1alpha1.RestoreJobPhaseSucceeded,
1329+
},
1330+
}
1331+
for _, tt := range tests {
1332+
t.Run(tt.name, func(t *testing.T) {
1333+
started := metav1.Now()
1334+
backup := &backupsv1alpha1.Backup{
1335+
ObjectMeta: metav1.ObjectMeta{Name: "src-sb", Namespace: sourceNS},
1336+
Spec: backupsv1alpha1.BackupSpec{
1337+
ApplicationRef: corev1.TypedLocalObjectReference{
1338+
APIGroup: stringPtr("apps.cozystack.io"),
1339+
Kind: tt.appKind,
1340+
Name: "src",
1341+
},
1342+
StrategyRef: corev1.TypedLocalObjectReference{Kind: "Velero", Name: "cozy-default-velero"},
1343+
DriverMetadata: map[string]string{veleroBackupNameMetadataKey: "vb"},
1344+
},
1345+
}
1346+
raw, err := json.Marshal(RestoreOptions{CommonRestoreOptions: CommonRestoreOptions{TargetNamespace: targetNS}})
1347+
if err != nil {
1348+
t.Fatalf("marshal options: %v", err)
1349+
}
1350+
restoreJob := &backupsv1alpha1.RestoreJob{
1351+
ObjectMeta: metav1.ObjectMeta{Name: "src-restore", Namespace: sourceNS},
1352+
Spec: backupsv1alpha1.RestoreJobSpec{
1353+
BackupRef: corev1.LocalObjectReference{Name: "src-sb"},
1354+
TargetApplicationRef: &corev1.TypedLocalObjectReference{
1355+
APIGroup: stringPtr("apps.cozystack.io"),
1356+
Kind: tt.appKind,
1357+
Name: "dst",
1358+
},
1359+
Options: &runtime.RawExtension{Raw: raw},
1360+
},
1361+
Status: backupsv1alpha1.RestoreJobStatus{
1362+
Phase: backupsv1alpha1.RestoreJobPhaseRunning,
1363+
StartedAt: &started,
1364+
},
1365+
}
1366+
owningLabels := map[string]string{
1367+
backupsv1alpha1.OwningJobNameLabel: restoreJob.Name,
1368+
backupsv1alpha1.OwningJobNamespaceLabel: restoreJob.Namespace,
1369+
}
1370+
veleroRestore := &velerov1.Restore{
1371+
ObjectMeta: metav1.ObjectMeta{Name: "vr", Namespace: veleroNamespace, Labels: owningLabels},
1372+
Status: velerov1.RestoreStatus{Phase: velerov1.RestorePhaseCompleted},
1373+
}
1374+
modifiers := &corev1.ConfigMap{
1375+
ObjectMeta: metav1.ObjectMeta{Name: "restore-modifiers-tenant-src-src-restore", Namespace: veleroNamespace, Labels: owningLabels},
1376+
}
1377+
strategy := &strategyv1alpha1.Velero{ObjectMeta: metav1.ObjectMeta{Name: "cozy-default-velero"}}
1378+
targetNamespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: targetNS}}
1379+
1380+
reconciler := newTestRestoreJobReconcilerWithDynamic(t, nil)
1381+
testScheme := runtime.NewScheme()
1382+
_ = scheme.AddToScheme(testScheme)
1383+
_ = backupsv1alpha1.AddToScheme(testScheme)
1384+
_ = velerov1.AddToScheme(testScheme)
1385+
_ = strategyv1alpha1.AddToScheme(testScheme)
1386+
reconciler.Client = clientfake.NewClientBuilder().
1387+
WithScheme(testScheme).
1388+
WithObjects(restoreJob, backup, veleroRestore, modifiers, strategy, targetNamespace).
1389+
WithStatusSubresource(&backupsv1alpha1.RestoreJob{}).
1390+
Build()
1391+
ctx := context.Background()
1392+
1393+
if _, err := reconciler.reconcileVeleroRestore(ctx, restoreJob, backup); err != nil {
1394+
t.Fatalf("reconcileVeleroRestore() error = %v", err)
1395+
}
1396+
1397+
got := &backupsv1alpha1.RestoreJob{}
1398+
if err := reconciler.Get(ctx, client.ObjectKeyFromObject(restoreJob), got); err != nil {
1399+
t.Fatalf("get RestoreJob: %v", err)
1400+
}
1401+
if got.Status.Phase != tt.wantPhase {
1402+
t.Fatalf("phase = %q, want %q", got.Status.Phase, tt.wantPhase)
1403+
}
1404+
if err := reconciler.Get(ctx, client.ObjectKeyFromObject(modifiers), &corev1.ConfigMap{}); !errors.IsNotFound(err) {
1405+
t.Errorf("resource-modifier ConfigMap should be deleted, get err = %v", err)
1406+
}
1407+
if tt.wantMessage == "" {
1408+
return
1409+
}
1410+
ready := meta.FindStatusCondition(got.Status.Conditions, "Ready")
1411+
if ready == nil {
1412+
t.Fatalf("Ready condition missing, conditions: %+v", got.Status.Conditions)
1413+
}
1414+
if !strings.Contains(ready.Message, tt.wantMessage) {
1415+
t.Errorf("Ready message = %q, want it to contain %q", ready.Message, tt.wantMessage)
1416+
}
12851417
})
12861418
}
12871419
}

0 commit comments

Comments
 (0)