Merge pull request #8937 from dokku/8912-cron-containers-are-never-retired-past-their-active-deadline

Retire cron containers past their active deadline

plugins/cron/cron.go

@@ -42,6 +42,23 @@ var (
 
 const MaintenancePropertyPrefix = "maintenance."
 
+// DefaultTTLSeconds is how long a cron task may run before it is reaped. The
+// docker-local scheduler stamps this onto the container as the
+// com.dokku.active-deadline-seconds label, while the k3s scheduler renders it
+// as the CronJob's activeDeadlineSeconds.
+const DefaultTTLSeconds int64 = 86400
+
+// validateTTLSeconds returns an error if the requested task lifetime is not a
+// positive number of seconds. A zero or negative deadline would either expire
+// the task the instant it starts or never expire it at all.
+func validateTTLSeconds(ttlSeconds int64) error {
+	if ttlSeconds <= 0 {
+		return fmt.Errorf("--ttl-seconds must be a positive integer")
+	}
+
+	return nil
+}
+
 // CronTask is a struct that represents a cron task
 type CronTask struct {
 	// ID is a unique identifier for the cron task

plugins/cron/cron_test.go

@@ -123,3 +123,32 @@ func TestDokkuRunCommandAppTaskIgnoresLogFile(t *testing.T) {
 		t.Errorf("DokkuRunCommand() interpolated a redirect into an app task line: %q", got)
 	}
 }
+
+// TestValidateTTLSeconds pins that a cron task deadline must be a positive
+// number of seconds. A zero or negative value would either expire the task the
+// instant it starts or leave it running forever, and the docker-local retire
+// pass only reaps containers whose deadline has actually elapsed.
+func TestValidateTTLSeconds(t *testing.T) {
+	cases := []struct {
+		name       string
+		ttlSeconds int64
+		wantErr    bool
+	}{
+		{name: "default", ttlSeconds: DefaultTTLSeconds},
+		{name: "positive override", ttlSeconds: 1},
+		{name: "zero", ttlSeconds: 0, wantErr: true},
+		{name: "negative", ttlSeconds: -1, wantErr: true},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateTTLSeconds(tc.ttlSeconds)
+			if tc.wantErr && err == nil {
+				t.Errorf("validateTTLSeconds(%d) = nil, want an error", tc.ttlSeconds)
+			}
+			if !tc.wantErr && err != nil {
+				t.Errorf("validateTTLSeconds(%d) = %v, want nil", tc.ttlSeconds, err)
+			}
+		})
+	}
+}

plugins/cron/src/commands/commands.go

@@ -21,7 +21,7 @@ Additional commands:`
     cron:list <app> [--format json|stdout], List scheduled cron tasks for an app
     cron:report [<app>] [<flag>], Display report about an app
     cron:resume <app> <cron_id>, Resume a cron task
-    cron:run <app> <cron_id> [--detach], Run a cron task on the fly
+    cron:run <app> <cron_id> [--detach] [--ttl-seconds SECONDS], Run a cron task on the fly
     cron:set [--global|<app>] <key> <value>, Set or clear a cron property for an app
     cron:suspend <app> <cron_id>, Suspend a cron task`
 )

plugins/cron/src/subcommands/subcommands.go

@@ -49,10 +49,11 @@ func main() {
 	case "run":
 		args := flag.NewFlagSet("cron:run", flag.ExitOnError)
 		detached := args.Bool("detach", false, "--detach: run the container in a detached mode")
+		ttlSeconds := args.Int64("ttl-seconds", cron.DefaultTTLSeconds, "--ttl-seconds: number of seconds the task may run before it is reaped")
 		args.Parse(os.Args[2:])
 		appName := args.Arg(0)
 		cronID := args.Arg(1)
-		err = cron.CommandRun(appName, cronID, *detached)
+		err = cron.CommandRun(appName, cronID, *detached, *ttlSeconds)
 	case "set":
 		args := flag.NewFlagSet("cron:set", flag.ExitOnError)
 		global := args.Bool("global", false, "--global: set a global property")

plugins/cron/subcommands.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"strconv"
 	"strings"
 
 	"github.com/dokku/dokku/plugins/common"
@@ -97,11 +98,15 @@ func CommandResume(appName string, cronID string) error {
 }
 
 // CommandRun executes a cron task on the fly
-func CommandRun(appName string, cronID string, detached bool) error {
+func CommandRun(appName string, cronID string, detached bool, ttlSeconds int64) error {
 	if err := common.VerifyAppName(appName); err != nil {
 		return err
 	}
 
+	if err := validateTTLSeconds(ttlSeconds); err != nil {
+		return err
+	}
+
 	tasks, err := FetchCronTasks(FetchCronTasksInput{AppName: appName})
 	if err != nil {
 		return err
@@ -139,7 +144,7 @@ func CommandRun(appName string, cronID string, detached bool) error {
 	os.Setenv("DOKKU_CONCURRENCY_POLICY", concurrencyPolicy)
 	os.Setenv("DOKKU_CRON_ID", cronID)
 	os.Setenv("DOKKU_RM_CONTAINER", "1")
-	os.Setenv("DOKKU_RUN_TTL_SECONDS", "86400")
+	os.Setenv("DOKKU_RUN_TTL_SECONDS", strconv.FormatInt(ttlSeconds, 10))
 	scheduler := common.GetAppScheduler(appName)
 	args := append([]string{scheduler, appName, "0", "--"}, fields...)
 	_, err = common.CallPlugnTrigger(common.PlugnTriggerInput{

plugins/ps/subcommands.go

@@ -145,7 +145,7 @@ func CommandRetire(appName string) error {
 		return fmt.Errorf("Error retiring containers: %w", err)
 	}
 
-	common.LogInfo1("Retiring expired run containers")
+	common.LogInfo1("Retiring expired run and cron containers")
 	_, err = common.CallPlugnTrigger(common.PlugnTriggerInput{
 		Trigger:     "scheduler-run-retire",
 		StreamStdio: true,

plugins/scheduler-docker-local/scheduler-run-retire

@@ -7,7 +7,7 @@ source "$PLUGIN_AVAILABLE_PATH/scheduler-docker-local/internal-functions"
 
 fn-scheduler-docker-local-run-retire-container() {
   declare desc="stop a container"
-  declare CONTAINER_ID="$1"
+  declare CONTAINER_ID="$1" APP="$2"
 
   local DOKKU_DOCKER_STOP_TIMEOUT="$(plugn trigger ps-get-property "$APP" stop-timeout-seconds || true)"
 
@@ -18,34 +18,45 @@ fn-scheduler-docker-local-run-retire-container() {
   "$DOCKER_BIN" container kill "$CONTAINER_ID" &>/dev/null || true
   "$DOCKER_BIN" container rm "$CONTAINER_ID" &>/dev/null || true
 
-  if "$DOCKER_BIN" container inspect "$CONTAINER_ID" &>/dev/null; then
-    dokku_log_warn "Unable to retire container ${CONTAINER_ID}"
-    return 1
+  local CONTAINER_STATUS
+  if ! CONTAINER_STATUS="$("$DOCKER_BIN" container inspect "$CONTAINER_ID" --format '{{ .State.Status }}' 2>/dev/null)"; then
+    return
   fi
+
+  # a container started with --rm is removed by the daemon once it exits, and
+  # stays inspectable in the removing state until that completes
+  if [[ "$CONTAINER_STATUS" == "removing" ]]; then
+    return
+  fi
+
+  dokku_log_warn "Unable to retire container ${CONTAINER_ID}"
+  return 1
 }
 
 fn-scheduler-docker-local-run-retire() {
-  declare desc="stop all run containers that have exceeded their active deadline"
-  declare APP="$1"
+  declare desc="stop all containers of a given type that have exceeded their active deadline"
+  declare CONTAINER_TYPE="$1" APP="$2"
   local containers=""
 
-  # find all run containers for the specified app
-  container_type_filter="label=com.dokku.container-type=run"
+  # docker ands repeated label filters, so each container type requires its own pass
+  declare -a filters=(--filter "label=com.dokku.container-type=$CONTAINER_TYPE")
   if [[ -n "$APP" ]]; then
-    app_filter="label=com.dokku.app-name=$APP"
-    containers="$("$DOCKER_BIN" container ls --filter "$container_type_filter" --filter "$app_filter" --format '{{.ID}} {{.CreatedAt}}')"
-  else
-    containers="$("$DOCKER_BIN" container ls --filter "$container_type_filter" --format '{{.ID}} {{.CreatedAt}}')"
+    filters+=(--filter "label=com.dokku.app-name=$APP")
   fi
 
+  containers="$("$DOCKER_BIN" container ls "${filters[@]}" --format '{{.ID}} {{.CreatedAt}}')"
+
   if [[ -z "$containers" ]]; then
     return
   fi
 
   # iterate over all containers, ignoring the timezone in the last two columns
   echo "$containers" | awk '{NF-=2} 1' | while read -r container_id container_date container_time; do
-    # get the cutoff time in seconds from the container's `com.dokku.active-deadline-seconds` label
-    active_deadline_seconds="$("$DOCKER_BIN" container inspect "$container_id" --format '{{ index .Config.Labels "com.dokku.active-deadline-seconds" }}')"
+    # get the cutoff time in seconds from the container's `com.dokku.active-deadline-seconds` label,
+    # along with the owning app so the app's stop timeout is respected during the global pass
+    container_labels="$("$DOCKER_BIN" container inspect "$container_id" --format '{{ index .Config.Labels "com.dokku.active-deadline-seconds" }}|{{ index .Config.Labels "com.dokku.app-name" }}')"
+    active_deadline_seconds="${container_labels%%|*}"
+    container_app="${container_labels##*|}"
     if [[ -z "$active_deadline_seconds" ]]; then
       continue
     fi
@@ -59,28 +70,36 @@ fn-scheduler-docker-local-run-retire() {
     # if the container start time is before the cutoff time, stop the container
     if [[ "$start_time" -lt "$cutoff_time" ]]; then
       dokku_log_verbose_quiet "Retiring container ${container_id}"
-      fn-scheduler-docker-local-run-retire-container "$container_id"
+      fn-scheduler-docker-local-run-retire-container "$container_id" "$container_app"
     fi
   done
 }
 
 trigger-scheduler-docker-local-scheduler-run-retire() {
-  declare desc="retires all run containers for an app that have exceeded their active deadline"
+  declare desc="retires all run and cron containers for an app that have exceeded their active deadline"
   declare trigger="scheduler-run-retire"
   declare DOKKU_SCHEDULER="$1" APP="$2"
+  declare -a CONTAINER_TYPES=(run cron)
+  local exit_code=0
 
   if [[ -z "$DOKKU_SCHEDULER" ]]; then
-    dokku_log_info1_quiet "Retiring all run containers"
-    fn-scheduler-docker-local-run-retire
-    return "$?"
+    dokku_log_info1_quiet "Retiring all run and cron containers"
+    for container_type in "${CONTAINER_TYPES[@]}"; do
+      fn-scheduler-docker-local-run-retire "$container_type" || exit_code=1
+    done
+    return "$exit_code"
   fi
 
   if [[ "$DOKKU_SCHEDULER" != "docker-local" ]]; then
     return
   fi
 
-  dokku_log_info1_quiet "Retiring run containers for app ${APP}"
-  fn-scheduler-docker-local-run-retire "$APP"
+  dokku_log_info1_quiet "Retiring run and cron containers for app ${APP}"
+  for container_type in "${CONTAINER_TYPES[@]}"; do
+    fn-scheduler-docker-local-run-retire "$container_type" "$APP" || exit_code=1
+  done
+
+  return "$exit_code"
 }
 
 trigger-scheduler-docker-local-scheduler-run-retire "$@"

plugins/scheduler-k3s/chart.go

@@ -577,12 +577,13 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti
 			Args:        words,
 			Annotations: annotations,
 			Cron: ProcessCron{
-				ID:                cronTask.ID,
-				Hash:              cronIDLabelValue(cronTask.ID),
-				Schedule:          cronTask.Schedule,
-				Suffix:            suffix,
-				Suspend:           cronTask.Maintenance,
-				ConcurrencyPolicy: ProcessCronConcurrencyPolicy(concurrencyPolicy),
+				ID:                    cronTask.ID,
+				Hash:                  cronIDLabelValue(cronTask.ID),
+				Schedule:              cronTask.Schedule,
+				Suffix:                suffix,
+				Suspend:               cronTask.Maintenance,
+				ConcurrencyPolicy:     ProcessCronConcurrencyPolicy(concurrencyPolicy),
+				ActiveDeadlineSeconds: cron.DefaultTTLSeconds,
 			},
 			Labels:      labels,
 			ProcessType: ProcessType_Cron,

plugins/scheduler-k3s/cron_job_template_test.go

@@ -3,6 +3,7 @@ package scheduler_k3s
 import (
 	"errors"
 	"io"
+	"maps"
 	"os"
 	"path/filepath"
 	"strings"
@@ -148,6 +149,61 @@ func TestCronJobTemplateRendersLongCronID(t *testing.T) {
 	})
 }
 
+// TestCronJobTemplateRendersActiveDeadlineSeconds asserts that the job's
+// activeDeadlineSeconds comes from the chart values rather than a literal
+// baked into the template, so the deadline cannot drift from the value the
+// docker-local scheduler stamps onto its cron containers. A release rendered
+// from values that predate the field must still fall back to 24 hours.
+func TestCronJobTemplateRendersActiveDeadlineSeconds(t *testing.T) {
+	baseValues := map[string]interface{}{
+		"id":                 "abcde",
+		"hash":               "0123456789abcdef0123456789abcdef01234567",
+		"schedule":           "5 5 5 5 5",
+		"suffix":             "abcde",
+		"suspend":            false,
+		"concurrency_policy": "Allow",
+	}
+
+	t.Run("from values", func(t *testing.T) {
+		values := maps.Clone(baseValues)
+		values["active_deadline_seconds"] = 600
+		if got := renderedActiveDeadlineSeconds(t, renderCronJobTemplate(t, values)); got != 600 {
+			t.Errorf("activeDeadlineSeconds = %d, want 600", got)
+		}
+	})
+
+	t.Run("defaults when absent", func(t *testing.T) {
+		if got := renderedActiveDeadlineSeconds(t, renderCronJobTemplate(t, maps.Clone(baseValues))); got != 86400 {
+			t.Errorf("activeDeadlineSeconds = %d, want 86400", got)
+		}
+	})
+}
+
+// renderedActiveDeadlineSeconds pulls jobTemplate.spec.activeDeadlineSeconds
+// out of a rendered cron-job manifest.
+func renderedActiveDeadlineSeconds(t *testing.T, manifest string) int {
+	t.Helper()
+
+	var doc map[string]interface{}
+	if err := yaml.NewDecoder(strings.NewReader(manifest)).Decode(&doc); err != nil {
+		t.Fatalf("yaml decode failed (would also fail in Kubernetes API): %v\nrendered:\n%s", err, manifest)
+	}
+
+	spec, _ := doc["spec"].(map[string]interface{})
+	jobTemplate, _ := spec["jobTemplate"].(map[string]interface{})
+	jobSpec, ok := jobTemplate["spec"].(map[string]interface{})
+	if !ok {
+		t.Fatalf("jobTemplate.spec missing from rendered manifest:\n%s", manifest)
+	}
+
+	deadline, ok := jobSpec["activeDeadlineSeconds"].(int)
+	if !ok {
+		t.Fatalf("activeDeadlineSeconds = %v (type %T); want an integer", jobSpec["activeDeadlineSeconds"], jobSpec["activeDeadlineSeconds"])
+	}
+
+	return deadline
+}
+
 // renderCronJobTemplate renders templates/chart/cron-job.yaml with the
 // supplied cron-process values and returns the YAML manifest as a string.
 // The chart is materialised in a tempdir alongside the shared _helpers.tpl

plugins/scheduler-k3s/template.go

@@ -306,12 +306,13 @@ const (
 )
 
 type ProcessCron struct {
-	ID                string                       `yaml:"id"`
-	Hash              string                       `yaml:"hash"`
-	Schedule          string                       `yaml:"schedule"`
-	Suffix            string                       `yaml:"suffix"`
-	Suspend           bool                         `yaml:"suspend"`
-	ConcurrencyPolicy ProcessCronConcurrencyPolicy `yaml:"concurrency_policy"`
+	ID                    string                       `yaml:"id"`
+	Hash                  string                       `yaml:"hash"`
+	Schedule              string                       `yaml:"schedule"`
+	Suffix                string                       `yaml:"suffix"`
+	Suspend               bool                         `yaml:"suspend"`
+	ConcurrencyPolicy     ProcessCronConcurrencyPolicy `yaml:"concurrency_policy"`
+	ActiveDeadlineSeconds int64                        `yaml:"active_deadline_seconds"`
 }
 
 type ProcessCronConcurrencyPolicy string

plugins/scheduler-k3s/templates/chart/cron-job.yaml

@@ -52,7 +52,7 @@ spec:
       backoffLimit: 0
       podReplacementPolicy: Failed
       ttlSecondsAfterFinished: 60
-      activeDeadlineSeconds: 86400
+      activeDeadlineSeconds: {{ $config.cron.active_deadline_seconds | default 86400 }}
       template:
         metadata:
           annotations: