Skip to content

terminationGracePeriodSeconds has had no effect since gitlab-runner v17.0.0 #62

Description

@alekc

KubernetesConfig.terminationGracePeriodSeconds (api/v1beta2/gitlab_types.go:69) has had no effect since gitlab-runner v17.0.0, but the CRD still advertises it as settable and admission still accepts it.

It used to be real

At v16.11.0 the key existed upstream and was load-bearing. GetPodTerminationGracePeriodSeconds (common/config.go:1558-1570) checked it first, so it took precedence over the newer pod_termination_grace_period_seconds, and the executor consumed that getter at kubernetes.go:1929 to set the build pod's grace period. Upstream's own comment there carried its removal ticket: "Will be removed with .../issues/28165."

Where it went

Upstream Present
v16.11.0 yes, and it overrides pod_termination_grace_period_seconds
v17.0.0 removed
v17.1.0 through v19.2.2 absent

Dropped at the v17 major boundary. At v19.2.2 no legacy path remains: the executor reads the field directly (kubernetes.go:2478, TerminationGracePeriodSeconds: s.Config.Kubernetes.PodTerminationGracePeriodSeconds) and the old getter is gone. A fossil confirms the direction: upstream's current pod_termination_grace_period_seconds description still ends "Ignored if KUBERNETES_TERMINATIONGRACEPERIODSECONDS is specified", referring to a field that no longer exists.

We have carried it since a89c37e ("feat: first draft", 2020-10-14), when it was current. Nothing operator-side ever read it; it is passthrough config.

Why it is worse than a harmless leftover

The runner decodes with toml.DecodeFile and never inspects MetaData.Undecoded(), so an unknown key is dropped with no error, no warning and no log line. A user who sets this on a Runner gets it accepted by admission, sees it in the rendered config.toml, and gets no grace-period behaviour at all. It is also the only camelCase toml key in the file, which is a reasonable smell test for exactly this kind of drift.

The mitigation that already exists: the field description copied upstream's deprecation text, so it names the two replacements, and we expose both (pod_termination_grace_period_seconds and cleanup_grace_period_seconds). The migration path is there; only the advertisement is wrong.

Suggested path

  1. Non-breaking, ship now: mark it deprecated in the CRD so kubectl explain and any UI stop presenting it as functional. Lead the description with "no effect since gitlab-runner v17.0.0, use pod_termination_grace_period_seconds".
  2. Optional: have the reconciler emit an event or status condition when it is set, so the silence becomes visible to someone who already has it in a manifest.
  3. Later, in a release that documents the break: remove the field.

Found while completing the v19.2.2 mirror in #58. Note this class of drift is invisible to the release watcher, which only compares upstream-minus-repo and never flags a key we carry that upstream has deleted. Tracked in #63.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions