Skip to content

feat(go-pmtud): remove init container and preStop hook - #12636

Merged
rkschamer merged 7 commits into
masterfrom
d053727/go-pmtud-from-scratch
Aug 26, 2026
Merged

feat(go-pmtud): remove init container and preStop hook #12636
rkschamer merged 7 commits into
masterfrom
d053727/go-pmtud-from-scratch

Conversation

@rkschamer

@rkschamer rkschamer commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

sapcc/go-pmtud#79 moves the preStart and preStop scripts into Go code of go-pmtud to enable FROM SCRATCH runtime image.

This PR removes the script from the respective helm chart


---

**Verdict: APPROVED** — Specification fully met, code quality sound, no issues identified.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check this in?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this can be removed of course! Thanks for highlighting

@defo89

defo89 commented Aug 25, 2026

Copy link
Copy Markdown
Member

nit: I'd prefer a safer rollout approach here. Instead of removing the init container and preStop hook outright, could we:

  1. Add a values toggle (e.g. .Values.pmtud.inBinaryLifecycle: false) that conditionally skips the init container / preStop hook
  2. Enable it only in QA values first to let it soak
  3. Once confirmed stable, flip the default to true and remove the legacy code path in a follow-up

The iptables rules are crucial for PMTUD — if the new in-binary lifecycle has a bug in setup/teardown, we'd break path MTU discovery cluster-wide with no easy rollback other than reverting the chart bump.

@rkschamer

rkschamer commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Yes...This makes sense. I'll address this.

Q: With the changes to the go-pmtud image (https://github.com/sapcc/go-pmtud/pull/79/changes#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R17) where will be no shell available once sapcc/go-pmtud#79 is merged. So, even we have the flag in the helm chart the preStart/Stop scripts would only be work with an image version before sapcc/go-pmtud#79.

Is this okay or should we also delay the FROM scratch runtime image...However, this brings me to another concern: having in the nftable rules set by the scripts and by the go binary might cause issues.

Is there a way to prevent the promotion to production based on the chart version? If not, we might need to add the toggle also for go-pmtud.

Add pmtud.inBinaryLifecycle (default: false) to allow gradual migration.
When false, the init container and preStop hook manage iptables rules.
When true, the binary handles rule lifecycle (no init container / preStop).
@rkschamer

rkschamer commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

...
Is there a way to prevent the promotion to production based on the chart version? If not, we might need to add the toggle also for go-pmtud.

I've double checked with Claude and it could answer all of my questions. Based on the insights Claude provided, here is my proposal to make the rollout to QA first.

Current Problem

Once the scratch image (go-pmtud#79) runs, there's no shell in the
container. The preStop hook exec ["/scripts/iptables-stop.sh"] fails on
scratch — it needs the legacy image. Plus the binary's fw.Setup()
runs unconditionally, so having both the init container AND binary manage
NFLOG rules creates conflict.

Change Sequence

  1. DO NOT merge go-pmtud PR yet
  2. gh workflow run "Container Registry GHCR" --ref <pr-branch> → produces
    sha-<sha> image without merging master
  3. Merge helm-charts PR (toggle, default false — prod unchanged)
  4. Merge cc-kube-secrets QA override:
    # values/helm/metal/qa-de-1/qa-de-1/go-pmtud.yaml
    images:
      pmtud:
        tag: sha-<from-step-2>
    pmtud:
      inBinaryLifecycle: true

@rkschamer
rkschamer removed the request for review from a team August 26, 2026 14:18
@rkschamer
rkschamer merged commit b2bac81 into master Aug 26, 2026
4 checks passed
@rkschamer
rkschamer deleted the d053727/go-pmtud-from-scratch branch August 26, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants