Skip to content

fix: stop passing --kernel-build-id when the image derives it #2791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rugggger wants to merge 1 commit into from
Closed

fix: stop passing --kernel-build-id when the image derives it #2791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions charts/weka-operator/resources/weka_runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,8 @@ class FeaturesFlags:
ssd_proxy_includes_dpdk_memory: Union[bool, int] = 9
# flags 10, 11 are not used by the operator
weka_manages_non_ionode_affinity: Union[bool, int] = 12
# flags 13, 14 are not used by the operator
auto_build_ids: Union[bool, int] = 15

@claude claude Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The operator-side mirror of this bitmap, domain.FeatureFlags in internal/pkg/domain/feature_flags.go, doesn't get the new flag. Not a bug — json.Unmarshal ignores the extra auto_build_ids key that write_feature_flags_json() now emits — but the two lists are maintained in lockstep today (they even carry the same "flags N are not used by the operator" comments), and the operator already resolves flags per-image via services.GetFeatureFlags. Adding the field keeps parity and leaves the door open for the operator to make this decision itself, which would side-step the loader having to find a release spec at all.


def __init__(self, b64_flags: Optional[str]) -> None:
active: Set[int] = set(parse_feature_bitmap(b64_flags or ""))
Expand Down Expand Up @@ -1513,10 +1515,22 @@ class ReleaseSpec:


async def get_release_spec() -> ReleaseSpec:
release_dir = "/opt/weka/dist/release"
files = os.listdir(release_dir)
assert len(files) == 1, Exception(f"Expected one release spec file, found: {files}")
spec_file_path = os.path.join(release_dir, files[0])
# same candidates as get_weka_version: in pods that stage the target version the
# local release dir is absent, and the shared copy is the one describing the
# version actually being installed
release_dirs = ["/opt/weka/dist/release", "/shared-weka-version/opt-weka/dist/release"]
spec_file_path = None
for release_dir in release_dirs:
if not os.path.isdir(release_dir):
continue
files = os.listdir(release_dir)
if not files:
continue
assert len(files) == 1, Exception(f"Expected one release spec file, found: {files}")
spec_file_path = os.path.join(release_dir, files[0])
break
if spec_file_path is None:
raise Exception(f"No release spec found in any of: {release_dirs}")

with open(spec_file_path, 'r') as f:
data = json.load(f)
Expand Down Expand Up @@ -1616,7 +1630,10 @@ def should_skip_igb_uio():
elif is_google_cos():
kernelBuildIdArg = f"--kernel-build-id {OS_BUILD_ID}"
elif is_ubuntu_24() and weka_dist_service():
kernelBuildIdArg = f"--kernel-build-id {UBUNTU24_BUILD_ID}"
# weka derives the build id itself when AutoBuildIds is set
feature_flags = await get_feature_flags()
if not feature_flags.auto_build_ids:
kernelBuildIdArg = f"--kernel-build-id {UBUNTU24_BUILD_ID}"
Comment on lines +1633 to +1636

@claude claude Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The lazy placement resolves my earlier concern — get_feature_flags() no longer runs on the DRIVERS_BUILD_ID override or is_google_cos() branches, and it can no longer raise here: get_weka_version() on line 1625 already succeeded using the same candidate list with the same skip conditions, so get_release_spec() is guaranteed to find a spec, and to find it in the same directory the version came from. That's a stronger invariant than the "prefer shared when TARGET_IMAGE_NAME differs" shape I suggested — version and flags can't diverge by construction, and it holds regardless of whether this runs before or after version_get_cmd populates the local dir.

The one thing still open is the asymmetry with the override, which this PR's own rationale now reaches: on an AutoBuildIds ubuntu24 image with driversBuildId set, the builder (line 4393) ignores DRIVERS_BUILD_ID entirely and lets weka derive the id, while this branch passes the user's value to weka driver download/install — the two sides decide differently, which is the disagreement this change exists to prevent. Pre-existing, and arguably correct if the override is meant as an escape hatch that wins over everything, but worth deciding explicitly rather than leaving it as a side effect of where the flag check landed. If the override should keep winning, the builder is the side that needs to honour it.


# When TARGET_IMAGE_NAME differs from IMAGE_NAME, weka files are copied
# from cluster image to /shared-weka-version/ via init container
Expand Down Expand Up @@ -4370,9 +4387,11 @@ async def main():
raise Exception(f"Failed to get weka version {version}: {stderr}")
logging.info(f"Successfully got weka version {version}")

build_feature_flags = await get_feature_flags()

@claude claude Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Placement here is right — it's after the weka version get ... --from file://shared-weka-version/opt-weka on line 4372, so /opt/weka/dist/release should be populated with the target version's spec by the time get_release_spec() reads it (unlike the loader call site, see the other comment).

One residual risk: this assumes --driver-only lays down the release spec. If it doesn't on some image, get_release_spec() raises before write_results(...) ever runs, so the operator sees a crash-looping builder pod rather than a build error it can report. Given drivers-builder has no retry/report wrapper like drivers-loader does, it's worth being defensive:

try:
    build_feature_flags = await get_feature_flags()
except Exception as e:
    logging.warning(f"Could not read feature flags, assuming no auto build ids: {e}")
    build_feature_flags = FeaturesFlags(None)

The description says this was verified on Ubuntu 24.04 and RHEL 9.7, so this is about the untested-image case, not the happy path.

kernel_build_id = ""
kernel_arg = ""
if is_ubuntu_24():
if is_ubuntu_24() and not build_feature_flags.auto_build_ids:
# weka derives the build id itself when AutoBuildIds is set
kernel_build_id = UBUNTU24_BUILD_ID
kernel_arg = f"--kernel-build-id {kernel_build_id}"
stdout, stderr, ec = await run_command(f"weka driver pack --without-agent --version {version} {kernel_arg}")
Expand Down
2 changes: 2 additions & 0 deletions internal/pkg/domain/feature_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,6 @@ type FeatureFlags struct {
SsdProxyIncludesDpdkMemory bool `json:"ssd_proxy_includes_dpdk_memory"` // 9
// flags 10, 11 are not used by the operator
WekaManagesNonIonodeAffinity bool `json:"weka_manages_non_ionode_affinity"` // 12
// flags 13, 14 are not used by the operator
AutoBuildIds bool `json:"auto_build_ids"` // 15
}
Loading