From be419e159f0e4d93079b77cc611ee87c7d6cff79 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Tue, 10 Dec 2024 11:17:26 +0100 Subject: [PATCH] Make sure the backup cleanup is always triggered This commit makes sure, that backups go into a completed state, even if there was nothing to backup at all. Signed-off-by: Simon Beck --- e2e/lib/k8up.bash | 26 +++++++++++++++++------ e2e/test-12-annotated-failure.bats | 2 +- e2e/test-13-cleanup-empty-jobs.bats | 28 +++++++++++++++++++++++++ operator/backupcontroller/controller.go | 2 ++ operator/backupcontroller/executor.go | 7 +++++++ operator/reconciler/reconciler.go | 2 +- 6 files changed, 59 insertions(+), 8 deletions(-) create mode 100644 e2e/test-13-cleanup-empty-jobs.bats diff --git a/e2e/lib/k8up.bash b/e2e/lib/k8up.bash index 774593c46..1b1b67a0d 100755 --- a/e2e/lib/k8up.bash +++ b/e2e/lib/k8up.bash @@ -149,6 +149,9 @@ given_a_subject() { kubectl apply -f definitions/pv/pvc.yaml yq e '.spec.template.spec.containers[0].securityContext.runAsUser='$(id -u)' | .spec.template.spec.containers[0].env[0].value=strenv(BACKUP_FILE_CONTENT) | .spec.template.spec.containers[0].env[1].value=strenv(BACKUP_FILE_NAME)' definitions/subject/deployment.yaml | kubectl apply -f - + # Let's wait for the deployment to actually be ready + kubectl -n k8up-e2e-subject wait --timeout 1m --for=condition=available deployment subject-deployment + echo "✅ The subject is ready" } @@ -167,6 +170,9 @@ given_an_annotated_subject() { kubectl apply -f definitions/pv/pvc.yaml yq e '.spec.template.spec.containers[1].securityContext.runAsUser='$(id -u)' | .spec.template.spec.containers[1].env[0].value=strenv(BACKUP_FILE_CONTENT) | .spec.template.spec.containers[1].env[1].value=strenv(BACKUP_FILE_NAME)' definitions/annotated-subject/deployment.yaml | kubectl apply -f - + # Let's wait for the deployment to actually be ready + kubectl -n k8up-e2e-subject wait --timeout 1m --for=condition=available deployment annotated-subject-deployment + echo "✅ The annotated subject is ready" } @@ -176,6 +182,9 @@ given_a_broken_annotated_subject() { kubectl apply -f definitions/pv/pvc.yaml yq e '.spec.template.spec.containers[1].securityContext.runAsUser='$(id -u)' ' definitions/annotated-subject/deployment-error.yaml | kubectl apply -f - + # Let's wait for the deployment to actually be ready + kubectl -n k8up-e2e-subject wait --timeout 1m --for=condition=available deployment annotated-subject-deployment + echo "✅ The annotated subject is ready" } @@ -187,6 +196,9 @@ given_an_annotated_subject_pod() { yq e '.spec.containers[1].securityContext.runAsUser='$(id -u)' | .spec.containers[1].env[0].value=strenv(BACKUP_FILE_CONTENT) | .spec.containers[1].env[1].value=strenv(BACKUP_FILE_NAME)' definitions/annotated-subject/pod.yaml | kubectl apply -f - + # Let's wait for the pod to actually be ready + kubectl -n k8up-e2e-subject wait --timeout 1m --for=condition=ready pod subject-pod + echo "✅ The annotated subject pod is ready" } @@ -198,6 +210,9 @@ given_a_rwo_pvc_subject_in_worker_node() { yq e 'with(select(document_index == 1) .spec.template.spec; .containers[0].securityContext.runAsUser='$(id -u)' | .containers[0].env[0].value=strenv(BACKUP_FILE_CONTENT) | .containers[0].env[1].value=strenv(BACKUP_FILE_NAME))' definitions/pvc-rwo-subject/worker.yaml | kubectl apply -f - + # Let's wait for the deployment to actually be ready + kubectl -n k8up-e2e-subject wait --timeout 1m --for=condition=available deployment pvc-rwo-subject-worker + echo "✅ The pvc rwo worker subject is ready" } @@ -209,6 +224,9 @@ given_a_rwo_pvc_subject_in_controlplane_node() { yq e 'with(select(document_index == 1) .spec.template.spec; .containers[0].securityContext.runAsUser='$(id -u)' | .containers[0].env[0].value=strenv(BACKUP_FILE_CONTENT) | .containers[0].env[1].value=strenv(BACKUP_FILE_NAME))' definitions/pvc-rwo-subject/controlplane.yaml | kubectl apply -f - + # Let's wait for the deployment to actually be ready + kubectl -n k8up-e2e-subject wait --timeout 1m --for=condition=available deployment pvc-rwo-subject-controlplane + echo "✅ The pvc rwo controlplane subject is ready" } @@ -324,14 +342,10 @@ given_an_existing_mtls_backup() { wait_until backup/k8up-backup-mtls completed verify "'.status.conditions[?(@.type==\"Completed\")].reason' is 'Succeeded' for Backup named 'k8up-backup-mtls'" - for i in {1..3}; do - run restic dump latest "/data/subject-pvc/${backup_file_name}" - if [ ! -z "${output}" ]; then - break - fi - done + run restic dump latest "/data/subject-pvc/${backup_file_name}" # shellcheck disable=SC2154 + echo "${backup_file_content} = ${output}" [ "${backup_file_content}" = "${output}" ] echo "✅ An existing backup is ready" diff --git a/e2e/test-12-annotated-failure.bats b/e2e/test-12-annotated-failure.bats index f4b26683b..0b72e8375 100644 --- a/e2e/test-12-annotated-failure.bats +++ b/e2e/test-12-annotated-failure.bats @@ -11,7 +11,7 @@ DETIK_CLIENT_NAMESPACE="k8up-e2e-subject" # shellcheck disable=SC2034 DEBUG_DETIK="true" -@test "Annotated app, When creating a backup, Then expect Error" { +@test "Given annotated app, When creating a backup, Then expect Error" { expected_content="expected content: $(timestamp)" expected_filename="expected_filename.txt" diff --git a/e2e/test-13-cleanup-empty-jobs.bats b/e2e/test-13-cleanup-empty-jobs.bats new file mode 100644 index 000000000..a7c01b092 --- /dev/null +++ b/e2e/test-13-cleanup-empty-jobs.bats @@ -0,0 +1,28 @@ +#!/usr/bin/env bats + +load "lib/utils" +load "lib/detik" +load "lib/k8up" + +# shellcheck disable=SC2034 +DETIK_CLIENT_NAME="kubectl" +# shellcheck disable=SC2034 +DETIK_CLIENT_NAMESPACE="k8up-e2e-subject" +# shellcheck disable=SC2034 +DEBUG_DETIK="true" + +@test "Given empty namespace, When creating multiple backups, Then expect cleanup" { + given_a_running_operator + given_a_clean_ns + given_s3_storage + + kubectl apply -f definitions/secrets + yq e '.spec.podSecurityContext.runAsUser='$(id -u)' | .metadata.name="first-backup"' definitions/backup/backup.yaml | kubectl apply -f - + + yq e '.spec.podSecurityContext.runAsUser='$(id -u)' | .metadata.name="second-backup"' definitions/backup/backup.yaml | kubectl apply -f - + + kubectl -n "$DETIK_CLIENT_NAMESPACE" annotate backup/second-backup reconcile=now + + wait_for_until_jsonpath backup/second-backup 5m 'jsonpath={.status.conditions[?(@.type=="Scrubbed")].message}="Deleted 1 resources"' + +} diff --git a/operator/backupcontroller/controller.go b/operator/backupcontroller/controller.go index a192aa0e7..bf4a39c3e 100644 --- a/operator/backupcontroller/controller.go +++ b/operator/backupcontroller/controller.go @@ -47,6 +47,7 @@ func (r *BackupReconciler) Provision(ctx context.Context, obj *k8upv1.Backup) (r log.V(1).Info("backup just started, waiting") return controllerruntime.Result{RequeueAfter: 5 * time.Second}, nil } + if obj.Status.HasFinished() || isPrebackupFailed(obj) { cleanupCond := meta.FindStatusCondition(obj.Status.Conditions, k8upv1.ConditionScrubbed.String()) if cleanupCond == nil || cleanupCond.Reason != k8upv1.ReasonSucceeded.String() { @@ -110,6 +111,7 @@ func (r *BackupReconciler) ReconcileJobStatus(ctx context.Context, obj *k8upv1.B } else if numStarted > 0 { objStatus.SetStarted(message) } + obj.SetStatus(objStatus) log.V(1).Info("updating status") diff --git a/operator/backupcontroller/executor.go b/operator/backupcontroller/executor.go index dfbfeb99d..ff488b6f0 100644 --- a/operator/backupcontroller/executor.go +++ b/operator/backupcontroller/executor.go @@ -289,6 +289,13 @@ func (b *BackupExecutor) startBackup(ctx context.Context) error { } } + if len(backupJobs) == 0 { + status := b.Obj.GetStatus() + status.SetSucceeded("nothing to backup") + b.Obj.SetStatus(status) + return b.Client.Status().Update(ctx, b.Obj) + } + return nil } diff --git a/operator/reconciler/reconciler.go b/operator/reconciler/reconciler.go index 31f99e721..8f98932d4 100644 --- a/operator/reconciler/reconciler.go +++ b/operator/reconciler/reconciler.go @@ -56,7 +56,7 @@ func (ctrl *controller[T, L]) Reconcile(ctx context.Context, request controllerr } else { res, provisionErr = ctrl.reconciler.Provision(ctx, obj) } - if apierrors.IsConflict(err) { // ignore "the object has been modified; please apply your changes to the latest version and try again" error, but requeue + if apierrors.IsConflict(provisionErr) { // ignore "the object has been modified; please apply your changes to the latest version and try again" error, but requeue log := controllerruntime.LoggerFrom(ctx) log.Info("Object has been modified, retrying...", "error", provisionErr.Error()) res.Requeue = true