Skip to content

Commit 2013e62

Browse files
androndoandrondo
committed
fix(backups): raise ArtifactNotMeasured only for a Backup this pass created
The Event was recorded before the Backup was created, so a Create that then failed left the BackupJob Failed next to an Event saying the backup succeeded, and a Create that collided with a Backup another pass had made reported on that pass's Backup. The Event is now recorded once Create succeeds. It also covers the one unmeasured path that had none: a failed existence check, which skips the probe. A test now also pins that a CA Secret key the s3-ca volume's items do not project is never trusted. Assisted-by: LLM Signed-off-by: Andrey Kolkov <[email protected]>
1 parent 6f3d26c commit 2013e62

2 files changed

Lines changed: 60 additions & 2 deletions

File tree

‎internal/backupcontroller/redis_artifact_size_test.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,16 @@ func configMapS3CA(spec *corev1.PodSpec) *corev1.PodSpec {
186186
return spec
187187
}
188188

189+
// nextEvent returns the next recorded event, or "" when there is none.
190+
func nextEvent(rec *record.FakeRecorder) string {
191+
select {
192+
case ev := <-rec.Events:
193+
return ev
194+
default:
195+
return ""
196+
}
197+
}
198+
189199
func TestRedisS3Trust(t *testing.T) {
190200
caSecret := func(data map[string]string) *corev1.Secret {
191201
s := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "s3-endpoint-ca"}, Data: map[string][]byte{}}
@@ -267,6 +277,14 @@ func TestRedisS3Trust(t *testing.T) {
267277
objs: []client.Object{caSecret(map[string]string{"other": "x"})},
268278
},
269279
"an s3-ca volume of another source": {spec: func(skip string) *corev1.PodSpec { return configMapS3CA(redisPodSpec("", skip)) }},
280+
"a CA Secret whose keys the volume does not project": {
281+
spec: func(skip string) *corev1.PodSpec {
282+
s := redisPodSpec("s3-endpoint-ca", skip)
283+
s.Volumes[0].Secret.Items = []corev1.KeyToPath{{Key: "bundle.pem", Path: "ca.crt"}}
284+
return s
285+
},
286+
objs: []client.Object{caSecret(map[string]string{"tls.crt": "UNPROJECTED"})},
287+
},
270288
}
271289
for name, tc := range refused {
272290
for _, skip := range []string{"", "true"} {
@@ -445,13 +463,46 @@ func TestCreateRedisBackupArtifact_RetryCheck(t *testing.T) {
445463
l := &fakeS3Lister{statSize: 777}
446464
r := redisReconciler(t, l, j, projectedCredsSecret(nil))
447465
r.APIReader = failingReads(t, newMariaDBStrategyTestClient(t))
466+
rec := record.NewFakeRecorder(4)
467+
r.Recorder = rec
448468
got, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey, nil)
449469
if err != nil || got == nil {
450470
t.Fatalf("createRedisBackupArtifact = %v, %v; a transient read must not fail the backup", got, err)
451471
}
452472
if l.calls != 0 {
453473
t.Errorf("S3 probed %d times after the existence check failed", l.calls)
454474
}
475+
if ev := nextEvent(rec); !strings.HasPrefix(ev, "Normal ArtifactNotMeasured ") || !strings.Contains(ev, "could not be checked") {
476+
t.Errorf("event = %q, want ArtifactNotMeasured saying the existence check failed", ev)
477+
}
478+
})
479+
480+
t.Run("no event when Create fails", func(t *testing.T) {
481+
r := redisReconciler(t, &fakeS3Lister{statErr: errors.New("NoSuchKey")}, j, projectedCredsSecret(nil))
482+
r.Client = forbidBackupCreate(t, r.Client)
483+
rec := record.NewFakeRecorder(4)
484+
r.Recorder = rec
485+
if _, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey, nil); err == nil {
486+
t.Fatal("createRedisBackupArtifact succeeded although Create was rejected")
487+
}
488+
if ev := nextEvent(rec); ev != "" {
489+
t.Errorf("event %q although the BackupJob fails", ev)
490+
}
491+
})
492+
493+
t.Run("no event when Create collides with a Backup another pass made", func(t *testing.T) {
494+
backup := &backupsv1alpha1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-1"}}
495+
store := newMariaDBStrategyTestClient(t, j, backup, projectedCredsSecret(nil))
496+
r := redisReconciler(t, &fakeS3Lister{statErr: errors.New("NoSuchKey")})
497+
r.Client, r.APIReader = staleCache(t, store), notFoundOnce(t, store)
498+
rec := record.NewFakeRecorder(4)
499+
r.Recorder = rec
500+
if _, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey, nil); err != nil {
501+
t.Fatalf("createRedisBackupArtifact: %v", err)
502+
}
503+
if ev := nextEvent(rec); ev != "" {
504+
t.Errorf("event %q about a Backup this pass did not create", ev)
505+
}
455506
})
456507

457508
t.Run("a Create collision is resolved from the API server past the cache", func(t *testing.T) {

‎internal/backupcontroller/redisstrategy_controller.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -498,13 +498,15 @@ func (r *BackupJobReconciler) createRedisBackupArtifact(
498498
Phase: backupsv1alpha1.BackupPhaseReady,
499499
},
500500
}
501+
unmeasured := "whether the Backup already exists could not be checked"
501502
if probe {
502503
artifact, err := r.redisArtifact(ctx, j.Namespace, objectKey, podSpec)
504+
unmeasured = ""
503505
if err != nil {
504506
getLogger(ctx).Info("leaving the Redis backup artifact unset", "backupjob", j.Name, "objectKey", objectKey, "error", err)
505507
var probeErr *artifactProbeError
506-
if r.Recorder != nil && errors.As(err, &probeErr) {
507-
r.Recorder.Eventf(j, corev1.EventTypeNormal, "ArtifactNotMeasured", "The backup succeeded, but its location and size were not recorded: %s", probeErr.reason)
508+
if errors.As(err, &probeErr) {
509+
unmeasured = probeErr.reason
508510
}
509511
}
510512
backup.Status.Artifact = artifact
@@ -522,6 +524,11 @@ func (r *BackupJobReconciler) createRedisBackupArtifact(
522524
}
523525
return existing, nil
524526
}
527+
// Only once the Backup exists: a terminal Create error fails the
528+
// BackupJob, and a collision returns a Backup another pass measured.
529+
if unmeasured != "" && r.Recorder != nil {
530+
r.Recorder.Eventf(j, corev1.EventTypeNormal, "ArtifactNotMeasured", "The backup succeeded, but its location and size were not recorded: %s", unmeasured)
531+
}
525532
return backup, nil
526533
}
527534

0 commit comments

Comments
 (0)