Skip to content

[helm] Support several Fluss clusters in one namespace - #4424

Open
affo wants to merge 21 commits into
apache:mainfrom
affo:la-helm-release-scoped-names
Open

[helm] Support several Fluss clusters in one namespace#4424
affo wants to merge 21 commits into
apache:mainfrom
affo:la-helm-release-scoped-names

Conversation

@affo

@affo affo commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Closes #4532.

Every resource the chart creates has a fixed name — coordinator-server,
tablet-server, fluss-conf-file, and the matching headless Services — and
every cluster uses the ZooKeeper root path /fluss. Two releases therefore
cannot share a namespace, which blocks anyone whose permissions do not extend
to creating one.

uniqueResourceNames prefixes each resource with fluss.fullname and moves
the root path to /fluss/<release>. The cross-references follow:
spec.serviceName, the fluss-conf volume, and the ADVERTISED_LISTENERS
FQDN.

It defaults to false, because renaming a StatefulSet replaces it and orphans
its PersistentVolumeClaims, so an existing release opts in deliberately — as
the Airflow chart does with useStandardNaming. The docs cover running several
clusters in one namespace, and migrating an existing release by reserving its
volumes for the new claims.

Test Plan

  • New helm/tests/naming_test.yaml: fixed names by default, prefixed names and
    cross-references when opted in, the ZooKeeper root in both modes, and the
    name-length validation.
  • helm unittest ./helm: 59 suites, 161 tests pass. helm lint: clean.
  • With the option off, helm template renders byte-for-byte identically to
    main, including with nameOverride and fullnameOverride set.
  • Two releases rendered into one namespace share no resource name.

🤖 Generated with Claude Code

affo and others added 2 commits September 18, 2026 15:12
@affo @claude
[helm] Add opt-in release-scoped resource names
6a76dc2 
Every resource the chart creates had a fixed name (coordinator-server,
tablet-server, fluss-conf-file and the matching headless Services), and
every cluster used the ZooKeeper root path /fluss. Two releases could
therefore not share a namespace, which is a problem wherever namespace
creation is restricted and a team needs more than one Fluss cluster.

Route every name through a helper and add releaseScopedResourceNames,
which prefixes each resource with fluss.fullname and moves the
ZooKeeper root path to /fluss/<release>.

The option is off by default. Renaming a StatefulSet makes Helm replace
it and orphans its PersistentVolumeClaims, so an existing release must
opt in deliberately. With the option off the rendered output is
byte-for-byte identical to before.

Also validate that generated names stay within the 63 character DNS
label limit when the option is on, and document both the multi-cluster
setup and the migration path for an existing release.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[helm] Rename the option and correct the migration guide
b498a44 
Rename releaseScopedResourceNames to uniqueResourceNames: the old name
described the implementation rather than what an operator gets, and did
not cover the ZooKeeper root path. Rename the two component helpers from
fluss.<component>.fullname to fluss.<component>.name for consistency
with the serviceName, metricsServiceName and configMapName helpers.

Correct three errors in the migration guide:

- Helm creates the new resources before deleting the ones that left the
  release manifest, so the two generations of pods overlap rather than
  the old being torn down first.
- That overlap makes tablet server ids collide, because the id comes
  from the pod ordinal. Registration is an ephemeral znode, so the
  second server retries for 60 seconds before aborting, which is the
  same as the default ZooKeeper session timeout.
- Replication factor does not limit the data loss: every replica is
  replaced at once with an empty volume, so only what has been tiered
  to remote storage survives.

Lead with the side-by-side migration, which keeps the old cluster
serving and does not depend on tiering, and document replacing in place
as the fallback.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo affo changed the title [helm] Add opt-in release-scoped resource names [helm] Support several Fluss clusters in one namespace Sep 29, 2026
affo and others added 17 commits September 29, 2026 15:48
@affo @claude
[helm] Leave the metrics Services alone
c4ab25a 
The metrics Services were already named after the release, so they could
not collide between two releases in one namespace. Routing them through
a helper renamed them for anyone enabling uniqueResourceNames without
buying any isolation, so leave them as they were.

With the metrics Services out of the picture the longest generated name
is a coordinator pod rather than a coordinator metrics Service, so the
release name prefix now has 40 characters to work with instead of 33.
Add test cases on both sides of that boundary.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[helm] Trim the uniqueResourceNames comment
9d8efc9 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[helm] Trim the component name helper docblock
44fe347 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[helm] Move naming into _names.tpl and fix the migration guide
d91db34 
Collect the name helpers in their own _names.tpl, alongside fluss.name,
fluss.fullname and fluss.chart, so every definition that produces a name
sits in one file. Replace the per-helper if/else with a single
fluss.resourceName that applies the naming scheme, leaving the component
helpers as a flat list of the names themselves. Move
fluss.names.validateError there too, which is where the chart README
says a feature's validation belongs.

Replace the migration advice for persistent storage. Copying the data
out with a Flink job was never necessary: a StatefulSet looks each claim
up by name and creates it only when it is missing, so claims created
under the new names and bound to the existing volumes are adopted as
they are. Document that rebinding instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[helm] Drop the fixed parameter and collect the metrics names
1e4b792 
fluss.resourceName took an optional "fixed" argument that only the
ConfigMap ever passed, which made the common case carry the cost of the
exception. Give the helper one job and let the ConfigMap state its own
case: it is the only resource whose bare name carries the chart name.

Define the metrics Service names in _names.tpl as well. They are already
unique per release so the naming scheme leaves them alone, but building
them inline in the templates meant not every resource name lived in one
place.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Clarify who creates the claims during the rename
4ead9af 
The migration guide read as though the chart had to supply volumeName.
It does not: Helm creates the StatefulSet, and the StatefulSet
controller creates the claims from volumeClaimTemplates only when they
are missing. Claims applied by hand under the new names, already bound
to the old volumes, are what the new pods mount, so no chart value is
involved. Say that outright.

Demote the ephemeral storage case to a closing note. Nobody running
emptyDir needs a migration path, so it does not deserve a section
alongside the volume rebinding.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Reserve the volumes instead of hand-writing claims
4235e0b 
Creating the claims by hand worked but left the operator owning objects
the StatefulSet controller would otherwise create and label. Kubernetes
supports the reverse binding: a PersistentVolume whose claimRef names a
claim is reserved for it, including a claim that does not exist yet, and
the controller leaves such a volume Available until that claim appears.

So patch the volumes instead and let the new StatefulSets create their
own claims. Note that claimRef has to be replaced wholesale, since a
surviving uid from the previous claim stops the volume counting as
reserved, and that the ordinals must stay aligned because a tablet
server takes its id from its pod ordinal.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Answer the open questions on the rename procedure
4efa5bc 
- Highlight the point of the whole procedure: each new claim has to bind
  to the volume its predecessor used.
- Warn that the cluster is down for the manual steps and not only for
  the upgrade, and that a GitOps controller will recreate a StatefulSet
  deleted in step 2 unless it is suspended.
- Say that nothing recreates a deleted StatefulSet, since Helm acts on a
  release only when a command is run against it.
- Say that reserving a volume for a claim that does not exist yet is
  supported and does not fail, and why a stale uid breaks it.
- Spell out why the arrangement needs no upkeep: immutable claim
  templates, a controller that only creates missing claims, claims that
  belong to neither Helm nor the retention policy's delete paths.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Make the ordinal rule actionable
e084ed6 
The rule sat in its own paragraph stating a constraint without saying
what to do about it. Put it where it applies instead: step 1 records the
claim-to-volume mapping, with sample output, and step 4 says to work
from that mapping and keep the ordinal, with what goes wrong if two are
crossed over.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[helm] Correct the generated-name claims in the rename procedure
81bdc01 
An independent review of the migration guide found two errors.

The claim names were given as a formula, data-<release>-fluss-<component>,
which only holds when the release name does not already contain the
chart name and no fullnameOverride is set. fluss.fullname collapses to
the release name alone in the first case, so the guide was wrong for a
release called "fluss" -- the very name its own install examples use --
and reserving a claim nothing ever creates is the worst outcome
available here: the new StatefulSet quietly creates its own claim and
the data stays stranded on an orphaned volume. Have the operator read
the StatefulSet names off a render instead.

The release name limit was given as 40 characters, but the validator
measures the prefix, which adds -fluss, so a release name without
"fluss" in it is capped at 34. The chart's own error message made the
same conflation; it now talks about the prefix throughout.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Wait for the pods, and admit the retry can run out
7baa499 
Two more findings from the review.

Deleting the claims straight after the StatefulSets stalls: the
pvc-protection finalizer holds a claim in Terminating while any pod
still references it, and a StatefulSet that later finds a claim in that
state refuses to create the pod and reports "pvc ... is being deleted",
so the upgrade fails. Wait for the pods, and verify every volume reads
Available against its new claim before upgrading.

The ZooKeeper registration paragraph read as though the retry always
wins. It does not: the loop rethrows once the 60 second budget is spent
and the pod crash-loops. The budget also has no margin over the default
session timeout, which is 60 seconds as well, so an ungraceful exit can
hold the znode for the whole window. The coordinator goes through the
same loop, so this was never tablet-server-only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Six more corrections from the review
a7e4b1f 
- Pinning the old ZooKeeper root is permanent and forfeits the
  ZooKeeper half of the isolation. The root is a Curator namespace, so a
  later release on the default /fluss/<release> nests inside the pinned
  tree, and a release named after one of Fluss's own root children
  collides with its data. Recommend sibling roots instead.
- Binding checks capacity even for a reserved volume, so changing
  storage.size in the same upgrade leaves the claim Pending against an
  Available volume with no stated cause.
- Immutable volumeClaimTemplates also means storage.size and
  storageClass can never be changed by upgrade afterwards, so those
  values want settling before migrating.
- Replacing claimRef is one way to drop the stale uid, not the only one;
  a merge patch that nulls uid works too.
- The Kubernetes docs on reserving a volume do not cover reserving for a
  claim that does not exist yet, so stop citing them for it.
- The length validation only covers names derived from fluss.fullname,
  and the metrics Services were never fixed-name.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Scope the claim listing to the release, and finish the review
7731c39 
The mapping in step 1 selected on app.kubernetes.io/name, which is two
kinds of wrong. The label carries nameOverride when one is set, so the
selector can return nothing on a cluster that does have volumes, and an
empty listing there reads as "nothing to preserve" right before the
step that stops the pods. It is also not release-scoped, so in the
namespace this feature exists to enable it returns every release's
claims, and a neighbour's row carried into step 4 hands its volume to
this release's claim. Select on instance, which is always the release
name. The same fix applies to the PVC deletion in Cleanup, which this
feature turns into a way to delete another release's data.

Also: the coordinator claim exists only when coordinator.storage.enabled
is set, and the two storage flags are independent; step 5 has to repeat
the release's existing -f and --set flags, and must not use
--reuse-values, which would drop the pin; note the way back out; and say
that the procedure was verified from source rather than against every
storage backend, WaitForFirstConsumer in particular.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Attribute the rehearsal caveat to storage classes
de1aea5 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] State the reservation behaviour plainly
5169fec 
The procedure was run end to end against the chart on Kubernetes 1.37
with a WaitForFirstConsumer storage class: a sentinel written to the
tablet volume before the rename was read back from it afterwards, with
the new claim bound to the original volume. Drop the caveat about
documentation status and describe what happens.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Rewrap
0921731 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo @claude
[docs] Cut the migration guide back
9a0dc19 
Trim the explanation to what an operator acts on: why the selector is
scoped, why the wait matters, and what the claimRef patch has to get
right, without the mechanism behind each one.

Drop the note about volumeClaimTemplates immutability blocking later
storage.size changes. It reads as though the migration takes something
away, when the field has always been immutable on any existing
StatefulSet -- helm upgrade could never change it. The only thing worth
saying is already said: the new claims have to fit the volumes being
reserved, so do not change the size during this upgrade.

Drop the note on the coordinator claim being conditional; the listing in
step 1 already shows what exists.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@affo
affo marked this pull request as ready for review September 30, 2026 13:41
@affo

affo commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

@swuferhong could you have a look?

@affo @claude
[helm] Make the scoped ZooKeeper root a sibling of /fluss
19f9e43 
The scoped root was /fluss/<release>, a child of the path every
un-migrated release already uses. Fluss passes the root to Curator as a
namespace, so a new release taking the default would have placed its
whole tree inside the existing cluster's, among that cluster's own
children -- and a release named after one of them, config or tables say,
would have landed on its data. Installing a second cluster beside an
existing one is the case this option is for, so that was the default
doing the wrong thing.

Use /fluss-<release> instead. The two roots are then siblings, a release
still on /fluss is unaffected, and the migration guide no longer has to
warn about nesting.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
morazow
morazow approved these changes Oct 2, 2026
View reviewed changes

@morazow morazow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, added one question 🤝

Comment thread helm/templates/_names.tpl Outdated
{{- $longestSuffix := 23 -}}
{{- $maxPrefix := sub 63 $longestSuffix -}}
{{- if gt (len $prefix) (int $maxPrefix) -}}
{{- printf "resource name prefix %q is %d characters, but generated names must stay within 63, so the prefix must be at most %d. Shorten the release name or set fullnameOverride." $prefix (len $prefix) (int $maxPrefix) -}}

@morazow morazow Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor nit:

The revision hash (max 10 characters) will be also attached to the sts names. Does this account for it also?

Reference: kubernetes/kubernetes#64023

@affo affo Oct 22, 2026

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.

Good catch — it did not.

Pods carry a controller-revision-hash label holding <statefulset>-<hash>. The hash is 10 characters and a label value caps at 63, so a StatefulSet name cannot exceed 52, not the 63 the check was measuring against.

Confirmed on a cluster:

sts name 52 -> pod created
sts name 53 -> FailedCreate: metadata.labels: Invalid value:
               "aaa…aaa-696677dcbf": must be no more than 63 bytes

The chart was rendering and installing such releases happily — a 28 character release name gave a 53 character coordinator StatefulSet that created no pods at all, which is exactly what this check is meant to prevent.

Fixed in 783a066: the limit is measured against 52, taking the maximum prefix from 40 to 33 and a release name without fluss in it from 34 to 27. Boundary tests moved to 27/28 accordingly.

@affo @claude
[helm] Account for the revision hash in the name limit
783a066 
Reviewer question on the length check: the StatefulSet controller
appends a revision hash to names derived from the StatefulSet, so does
the limit allow for it?

It did not. Every pod carries a controller-revision-hash label holding
"<statefulset>-<hash>", the hash is 10 characters, and a label value
cannot exceed 63 -- so a StatefulSet name cannot exceed 52, well short
of the 63 the check assumed. Confirmed on a cluster: a 52 character
StatefulSet creates its pod, a 53 character one fails with

  metadata.labels: Invalid value: "<name>-696677dcbf":
  must be no more than 63 bytes

and the chart was rendering and installing such a release happily. A 28
character release name produced a 53 character coordinator StatefulSet
that created no pods at all, which is exactly what this check exists to
prevent.

Measure against 52 rather than 63, taking the maximum prefix from 40 to
33 and a release name without "fluss" in it from 34 to 27.

Reference: kubernetes/kubernetes#64023

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@morazow morazow morazow approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[helm] Support several Fluss clusters in one namespace

2 participants

@affo @morazow