Skip to content

fix(kafka): preserve stable log.dirs ordering across storageConfigs changes - #350

Open
dobrerazvan wants to merge 1 commit into
mainfrom
fix/log-dirs-stable-ordering
Open

dobrerazvan wants to merge 1 commit into
mainfrom
fix/log-dirs-stable-ordering

Conversation

@dobrerazvan

@dobrerazvan dobrerazvan commented Oct 2, 2026 •

Copy link
Copy Markdown

Summary

Fixes getEffectiveLogDirsMountPaths so that reordering entries in spec.brokers[].brokerConfig.storageConfigs (or the shared brokerConfigGroups equivalent) no longer reshuffles the broker's effective log.dirs.

Why this matters

In KRaft mode, when metadata.log.dir is not explicitly set, Kafka defaults it to log.dirs[0]. Koperator never sets metadata.log.dir explicitly, so whichever path ends up first in log.dirs silently becomes the metadata log directory.

Previously, the merge function started from mountPathsNew (the list freshly generated from the current storageConfigs), so any reordering of storageConfigs — even with no actual disks added or removed — propagated straight into log.dirs. This could silently move the KRaft metadata log to a different disk as a side effect of an unrelated spec edit (e.g., someone removing and then re-adding a disk entry, or just tidying up the YAML).

Behavior change

Before After
Existing paths, still declared Reordered to match storageConfigs declaration order Keep their original relative position from the previous log.dirs
Path with pending disk removal/rebalance Kept, but position followed storageConfigs order Kept in its original position
Genuinely new path (not previously in log.dirs) Took the position dictated by storageConfigs Appended at the end, in storageConfigs relative order

Example 1 — pure reordering, no disk changes

  • Old log.dirs: [/a/kafka, /b/kafka, /c/kafka]
  • storageConfigs reordered to declare: [/c, /a, /b]

Before: log.dirs becomes [/c/kafka, /a/kafka, /b/kafka] → /c is now log.dirs[0], silently becoming the KRaft metadata dir.
After: log.dirs stays [/a/kafka, /b/kafka, /c/kafka] → /a remains log.dirs[0].

Example 2 — disk removal confirmed, then re-added before an existing disk

  • Old log.dirs: [/b/kafka] (disk /a was already fully removed)
  • storageConfigs now declares: [/a, /b] (re-adding /a ahead of /b)

Before: log.dirs becomes [/a/kafka, /b/kafka] → /a becomes log.dirs[0].
After: log.dirs becomes [/b/kafka, /a/kafka] → /b keeps its existing slot, /a is appended as the newly (re-)added disk.

Example 3 — mixed: pending removal + confirmed-removal re-add

  • Old log.dirs: [/b/kafka, /c/kafka], with /c mid graceful-disk-removal (GracefulDiskRemovalRequired)
  • storageConfigs now declares: [/a, /b] (adds /a, drops /c from the spec, /c's removal is still pending)

Before: log.dirs becomes [/a/kafka, /b/kafka] (and /c handling depended on where it ended up relative to the new order).
After: log.dirs becomes [/b/kafka, /c/kafka, /a/kafka] → /b and /c keep their original relative order (with /c retained because its removal hasn't completed yet), and /a is appended at the end as the new disk.

Changes

  • pkg/resources/kafka/configmap.go: getEffectiveLogDirsMountPaths now builds the effective list by walking mountPathsOld first (preserving order, including paths kept during a pending removal/rebalance), then appends only genuinely new paths from mountPathsNew at the end. Added a doc comment explaining the KRaft metadata.log.dir rationale.
  • pkg/resources/kafka/configmap_test.go: added 5 new table-driven cases to TestGetEffectiveLogDirsMountPaths:
    • reordering existing paths in storageConfigs does not reorder log.dirs
    • newly added paths are appended at the end in their declared relative order
    • a path with pending disk removal, re-declared in a swapped position, keeps its original position
    • a path re-added after its removal was already confirmed is appended at the end, ignoring its declared position
    • mixed scenario combining both pending-retention and confirmed-readd-at-tail behaviors

All 14 sub-tests in TestGetEffectiveLogDirsMountPaths (9 existing + 5 new) pass.

This PR alone does not guarantee __cluster_metadata-0 stays on log.dirs[0]

This fix only stabilizes log.dirs ordering when a previous broker ConfigMap exists to merge against (mountPathsOld non-empty). It does not verify or correct where the physical __cluster_metadata-0 partition actually lives on disk, and there are still code paths where log.dirs[0] can end up pointing at a mount path that was never the metadata directory:

  • No previous ConfigMap found — getEffectiveLogDirsMountPaths short-circuits to mountPathsNew as-is:
    if len(mountPathsOld) == 0 {
        return append([]string{}, mountPathsNew...)
    }
    This triggers on first-ever reconcile, a deleted/recreated broker ConfigMap, or any NotFound/API error while fetching it — regardless of whether the attached PVCs already contain real __cluster_metadata-0 data from a prior broker life (e.g. disaster recovery / PV restore). log.dirs[0] then becomes whatever is first in storageConfigs, with no check against existing on-disk metadata state.
  • Pre-existing drift from before this fix is deployed — this PR freezes whatever order is already recorded in the ConfigMap; it does not detect or repair clusters that already drifted under the old (pre-fix) behavior.
  • Confirmed disk removal — when the disk at log.dirs[0] is removed and shouldKeepRemovedLogDirInConfig reports the removal as no longer pending (no GracefulActionState entry, or no broker status at all), that path is dropped and whatever is next becomes log.dirs[0], with no verification that __cluster_metadata-0 was actually moved off it beforehand.
  • Manual/out-of-band changes — direct edits to the broker ConfigMap or CR, Helm chart migrations, or PV relabeling bypass the merge logic entirely.

Recommendation: keep (or add) an init script/container that scans log.dirs for the directory actually holding __cluster_metadata-0 and mvs it under log.dirs[0] before Kafka starts, as a defense-in-depth safety net. This PR reduces how often that script needs to do anything — the common "user reordered storageConfigs" case becomes a no-op for it — but it does not replace the need for it in the scenarios above.

Testing

go test ./pkg/resources/kafka/... -run TestGetEffectiveLogDirsMountPaths -v

…hanges

Previously, getEffectiveLogDirsMountPaths() seeded the effective list
directly from mountPathsNew (the freshly generated order from the
current storageConfigs spec), so simply reordering entries in
storageConfigs reshuffled log.dirs to match. In KRaft mode an unset
metadata.log.dir defaults to log.dirs[0], so this could silently move
the metadata log to a different disk whenever storageConfigs was
reordered (e.g. while removing/re-adding a disk), with no change
intended to the actual broker storage layout.

Now the effective list is built by walking mountPathsOld first,
keeping each existing path in its original position (including paths
kept temporarily during a pending disk removal/rebalance), and only
appending genuinely new paths from mountPathsNew at the end, in their
declared relative order.

Example: old log.dirs = [a, b, c]; storageConfigs reordered to
declare [c, a, b] -> before: log.dirs became [c, a, b]; after:
log.dirs stays [a, b, c].

Example: disk 'b' removal confirmed, then re-added before 'a' in
storageConfigs (old=[b], new=[a, b]) -> before: log.dirs became
[a, b]; after: log.dirs becomes [b, a] (b keeps its slot, a is
appended as new).

Adds 5 table-driven test cases to TestGetEffectiveLogDirsMountPaths
covering reordering, new-path append order, pending-removal position
retention under reordering, confirmed-removal-then-readd append, and
a mixed scenario.
@dobrerazvan

Copy link
Copy Markdown
Author

@azun @eduardagarici Please review.

This branch has not been deployed

No deployments
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.

1 participant