From 2924d9471770c97debfaa7e9668d5888f1ddb36c Mon Sep 17 00:00:00 2001 From: Talal Al-Tamimi Date: Mon, 7 Mar 2022 12:24:38 +0000 Subject: [PATCH 1/3] Abort consoles with more than one pod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we are setting a console's Job's `restartPolicy` to "Never", so if any container within the Pod fails then the Job won't spawn another Pod. We are also setting the Job's `backoffLimit` to 0, so if the Pod itself fails for any reason then the Job won't spawn another Pod either. These two settings together prevent a second Pod from being launched when a failure occurs. However, these settings don't cover situations where a console’s Pod is deleted. This can happen due to manual deletion, Pod eviction, or Pod preemption. There are no Job settings to prevent relaunching a Pod that has disappeared in one of these ways. Launching a subsequent console Pod beyond the first one is problematic, even if there is only one running Pod at any given time. A subsequent Pod causes the workloads controller to enter its state machine logic in a way that it wasn't designed to handle. It also causes the console to remain in a running state for far longer than users expect. With this change, the workloads controller stops a console and deletes its resources if it detects that more than one Pod belongs (or belonged) to that console. --- controllers/workloads/console/controller.go | 102 +++++++++++++++----- 1 file changed, 79 insertions(+), 23 deletions(-) diff --git a/controllers/workloads/console/controller.go b/controllers/workloads/console/controller.go index 46c17011..570f46a8 100644 --- a/controllers/workloads/console/controller.go +++ b/controllers/workloads/console/controller.go @@ -267,7 +267,13 @@ func (r *ConsoleReconciler) Reconcile(logger logr.Logger, ctx context.Context, r } } - job, err := r.getJob(ctx, req.NamespacedName) + var ( + job *batchv1.Job + pod *corev1.Pod + podList corev1.PodList + ) + + job, err = r.getJob(ctx, req.NamespacedName) if err != nil { job = nil } @@ -293,6 +299,36 @@ func (r *ConsoleReconciler) Reconcile(logger logr.Logger, ctx context.Context, r } } + if job != nil { + matchLabels := client.MatchingLabels(map[string]string{"job-name": job.ObjectMeta.Name}) + if err := r.List(ctx, &podList, client.InNamespace(csl.Namespace), matchLabels); err != nil { + return ctrl.Result{}, errors.Wrap(err, "failed to list pods for console job") + } + + // Check if more than one pod belongs to the console or if the current pod is not the same + // as the original pod (which also means that more than one pods was launched). A console's + // job object has `restartPolicy=Never` and `backoffLimit=0`. These two settings together + // prevent the job from launching a second pod when a failure occurs on the original pod. + // However, these settings don't cover situations where a console pod is deleted + // (e.g. manual deletion, eviction, preemption). We want consoles to never launch more than + // one pod. Launching a subsequent pod is problematic even if there is only one running pod + // at any given time. It causes the controller to enter its state machine logic in a way + // that it wasn't designed to handle. It also causes the console to remain in a running + // state for far longer than users expect. + if len(podList.Items) > 1 || (len(podList.Items) == 1 && csl.Status.PodName != "" && csl.Status.PodName != podList.Items[0].Name) { + logger.Info("More than one pod observed for console; deleting job and stopping console") + if err := r.abort(ctx, logger, csl, job, &podList); err != nil { + return ctrl.Result{}, errors.Wrap(err, "failed to abort console") + } + // No need to requeue after an abort because the deleted job will trigger us again. + return ctrl.Result{Requeue: false}, nil + } + + if len(podList.Items) == 1 { + pod = &podList.Items[0] + } + } + // Update the status fields in case they're out of sync, or the console spec // has been updated statusCtx := consoleStatusContext{ @@ -301,9 +337,10 @@ func (r *ConsoleReconciler) Reconcile(logger logr.Logger, ctx context.Context, r Authorisation: authorisation, AuthorisationRule: authRule, Job: job, + Pod: pod, } - csl, err = r.generateStatusAndAuditEvents(ctx, logger, req.NamespacedName, csl, statusCtx) + csl, err = r.generateStatusAndAuditEvents(ctx, logger, csl, statusCtx) if err != nil { return ctrl.Result{}, errors.Wrap(err, "failed to generate console status or audit events") } @@ -339,6 +376,10 @@ func (r *ConsoleReconciler) Reconcile(logger logr.Logger, ctx context.Context, r return ctrl.Result{}, err } } + // Retrigger reconciliation periodically to catch situations where a console pod is deleted + // and re-spawned by the console job. Note that this isn't strictly necessary as Kubernetes + // will periodically refresh caches and queue reconciliation events anyway. + res = requeueAfterInterval(logger, 30*time.Second) case csl.PostRunning(): // Requeue for when the console has reached its after finished TTL so it can be deleted res = requeueAfterInterval(logger, time.Until(*csl.GetGCTime())) @@ -517,27 +558,7 @@ type consoleStatusContext struct { Job *batchv1.Job } -func (r *ConsoleReconciler) generateStatusAndAuditEvents(ctx context.Context, logger logr.Logger, name types.NamespacedName, csl *workloadsv1alpha1.Console, statusCtx consoleStatusContext) (*workloadsv1alpha1.Console, error) { - var ( - pod *corev1.Pod - podList corev1.PodList - ) - - if statusCtx.Job != nil { - inNamespace := client.InNamespace(name.Namespace) - matchLabels := client.MatchingLabels(map[string]string{"job-name": statusCtx.Job.ObjectMeta.Name}) - if err := r.List(ctx, &podList, inNamespace, matchLabels); err != nil { - return nil, errors.Wrap(err, "failed to list pods for console job") - } - } - if len(podList.Items) > 0 { - pod = &podList.Items[0] - } else { - pod = nil - } - - statusCtx.Pod = pod - +func (r *ConsoleReconciler) generateStatusAndAuditEvents(ctx context.Context, logger logr.Logger, csl *workloadsv1alpha1.Console, statusCtx consoleStatusContext) (*workloadsv1alpha1.Console, error) { logger = getAuditLogger(logger, csl, statusCtx) newStatus := calculateStatus(csl, statusCtx) @@ -635,6 +656,41 @@ func calculateStatus(csl *workloadsv1alpha1.Console, statusCtx consoleStatusCont return newStatus } +func (r *ConsoleReconciler) abort(ctx context.Context, logger logr.Logger, csl *workloadsv1alpha1.Console, job *batchv1.Job, podList *corev1.PodList) error { + // Delete job + if err := r.Client.Delete(ctx, job); err != nil { + return errors.Wrap(err, "failed to delete job") + } + + // Delete pods + var podDeleteError error + for _, pod := range podList.Items { + if err := r.Client.Delete(ctx, &pod); err != nil { + podDeleteError = err + } + } + if podDeleteError != nil { + return errors.Wrap(podDeleteError, "failed to delete pod(s)") + } + + // Update console status + newStatus := csl.DeepCopy().Status + newStatus.Phase = workloadsv1alpha1.ConsoleStopped + updatedCsl := csl.DeepCopy() + updatedCsl.Status = newStatus + + if err := r.createOrUpdate(ctx, logger, csl, csl, Console, consoleDiff); err != nil { + return err + } + + // Publish termination event + if err := r.LifecycleRecorder.ConsoleTerminate(ctx, csl, false, nil); err != nil { + logging.WithNoRecord(logger).Error(err, "failed to record event", "event", "console.terminate") + } + + return nil +} + func calculatePhase(statusCtx consoleStatusContext) workloadsv1alpha1.ConsolePhase { if !statusCtx.IsAuthorised { return workloadsv1alpha1.ConsolePendingAuthorisation From dcf37eb3f9f37817f4e54fb78ec22c3bd027acf5 Mon Sep 17 00:00:00 2001 From: Talal Al-Tamimi Date: Sun, 13 Mar 2022 23:21:37 +0000 Subject: [PATCH 2/3] Source code comments --- controllers/workloads/console/controller.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/controllers/workloads/console/controller.go b/controllers/workloads/console/controller.go index 570f46a8..b7f6557f 100644 --- a/controllers/workloads/console/controller.go +++ b/controllers/workloads/console/controller.go @@ -662,7 +662,9 @@ func (r *ConsoleReconciler) abort(ctx context.Context, logger logr.Logger, csl * return errors.Wrap(err, "failed to delete job") } - // Delete pods + // Delete pods. In theory we shouldn't have to do this. All console pods are owned by + // the console job. A delete operation should cascade. However, in our testing we saw + // that the second pod launched by the job consistently lingers on after the job is gone. var podDeleteError error for _, pod := range podList.Items { if err := r.Client.Delete(ctx, &pod); err != nil { From a7adac98d935637cae9fe38c9a2528b92f32bd8f Mon Sep 17 00:00:00 2001 From: Talal Al-Tamimi Date: Sun, 13 Mar 2022 23:31:59 +0000 Subject: [PATCH 3/3] Bump version --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 19811903..f2807196 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.8.0 +3.8.1