Add architecture hints to ephemeral volumes #1435

lukasfrank · 2025-12-19T14:28:43Z

Proposed Changes

Add architecture hints to ephemeral volumes

Fixes #1434

Summary by CodeRabbit

New Features
- Ephemeral volumes now inherit machine architecture (when available) so OS images created for ephemeral volumes include architecture hints.
Tests
- Added test coverage validating architecture-aware ephemeral volume creation and OS image architecture propagation.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

lukasfrank · 2025-12-19T14:29:08Z

@coderabbitai review

coderabbitai · 2025-12-19T14:46:47Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-12-19T14:46:52Z

Walkthrough

Controller and tests updated to infer a machine's architecture from its MachineClass label and propagate that architecture into newly created ephemeral Volume OSImage.Architecture. A new constant defines the MachineArchitectureLabel key; tests and test helpers adjusted to exercise the behavior.

Changes

Cohort / File(s)	Summary
Architecture Label Definition `api/common/v1alpha1/common_types.go`	Added exported constant `MachineArchitectureLabel = "common.ironcore.dev/architecture"` with comment.
Controller Architecture Handling `internal/controllers/compute/machine_ephemeralvolume_controller.go`	Added `getMachineArchitecture(ctx, machine)` to read architecture from MachineClass labels and `addArchitectureIfNeeded(log, volume, architecture)` to set `OSImage.Architecture` when appropriate. Reconcile flow adjusted to fetch architecture and apply it to ephemeral volumes before creation; imports updated.
Controller Tests `internal/controllers/compute/machine_ephemeralvolume_controller_test.go`	Added imports (`corev1alpha1`, `resource`, `ptr`), introduced test case asserting ephemeral volume `spec.osImage.architecture == "amd64"`, and added VolumeClass setup usage.
Test Helpers / Suite `internal/controllers/compute/suite_test.go`	Added `SetupVolumeClass()` helper; extended `SetupMachineClass()` to include `ObjectMeta.Labels` with `commonv1alpha1.MachineArchitectureLabel: "amd64"`; added `commonv1alpha1` import.

Sequence Diagram

sequenceDiagram
    participant Reconciler as MachineEphemeralVolume Reconciler
    participant Machine as Machine (object)
    participant MachineClass as MachineClass (labels)
    participant Volume as Volume (creation)

    Reconciler->>Machine: Read Machine being reconciled
    activate Machine
    Reconciler->>MachineClass: Fetch referenced MachineClass
    activate MachineClass
    MachineClass-->>Reconciler: Return labels (may include architecture)
    deactivate MachineClass

    note right of Reconciler: Extract architecture via getMachineArchitecture()

    loop for each ephemeral volume to create
        Reconciler->>Volume: addArchitectureIfNeeded(volume, architecture)
        alt architecture present and OSImage exists and not set
            Volume-->>Reconciler: OSImage.Architecture set
        else already present or no architecture
            Volume-->>Reconciler: no change
        end
        Reconciler->>Volume: create Volume resource
        Volume-->>Reconciler: creation result
    end
    deactivate Machine

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas to focus review on:
- getMachineArchitecture error handling and label parsing.
- addArchitectureIfNeeded conditions (when it sets vs skips) and interaction with existing OSImage handling.
- Tests: correctness of VolumeClass setup and the new test assertions verifying architecture propagation.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: adding architecture hints to ephemeral volumes, which matches the primary objective.
Description check	✅ Passed	The description includes the required sections from the template with a clear proposed change and issue reference, though details are minimal.
Linked Issues check	✅ Passed	The PR implements the core objective from #1434: extending MachineEphemeralVolume controller to infer OS architecture from the Machine resource and propagate it to ephemeral volumes.
Out of Scope Changes check	✅ Passed	All changes are scoped to the stated objectives: adding architecture label constant, implementing architecture retrieval and propagation logic, and adding corresponding tests.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feat/set-architecture-hints

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3408391 and d6195e7.

📒 Files selected for processing (4)

api/common/v1alpha1/common_types.go (1 hunks)
internal/controllers/compute/machine_ephemeralvolume_controller.go (4 hunks)
internal/controllers/compute/machine_ephemeralvolume_controller_test.go (2 hunks)
internal/controllers/compute/suite_test.go (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

internal/controllers/compute/machine_ephemeralvolume_controller.go
internal/controllers/compute/machine_ephemeralvolume_controller_test.go
api/common/v1alpha1/common_types.go
internal/controllers/compute/suite_test.go

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6fb9ba and 3408391.

📒 Files selected for processing (4)

api/common/v1alpha1/common_types.go (1 hunks)
internal/controllers/compute/machine_ephemeralvolume_controller.go (4 hunks)
internal/controllers/compute/machine_ephemeralvolume_controller_test.go (2 hunks)
internal/controllers/compute/suite_test.go (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

internal/controllers/compute/suite_test.go (6)

api/common/v1alpha1/common_types.go (1)

MachineArchitectureLabel (36-36)

internal/controllers/storage/suite_test.go (1)

SetupVolumeClass (189-201)

internal/apis/storage/volumeclass_types.go (1)

VolumeClass (17-26)

api/storage/v1alpha1/volumeclass_types.go (1)

VolumeClass (21-29)

api/core/v1alpha1/resource.go (3)

ResourceList (74-74)

ResourceIOPS (27-27)

ResourceTPS (25-25)

internal/apis/core/resource.go (3)

ResourceList (49-49)

ResourceIOPS (24-24)

ResourceTPS (22-22)

internal/controllers/compute/machine_ephemeralvolume_controller.go (1)

api/common/v1alpha1/common_types.go (1)

MachineArchitectureLabel (36-36)

internal/controllers/compute/machine_ephemeralvolume_controller_test.go (2)

utils/testing/testing.go (1)

SetupNamespace (148-156)

internal/controllers/compute/suite_test.go (2)

SetupMachineClass (177-192)

SetupVolumeClass (194-206)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: test

🔇 Additional comments (10)

internal/controllers/compute/suite_test.go (3)

13-14: LGTM!

The import addition is necessary for using MachineArchitectureLabel in the test setup.

182-184: LGTM!

Adding the architecture label to the test MachineClass properly supports the new architecture-aware test scenarios.

194-206: LGTM!

The SetupVolumeClass helper follows the existing pattern and correctly configures a VolumeClass with appropriate capabilities for testing.

internal/controllers/compute/machine_ephemeralvolume_controller_test.go (3)

8-8: LGTM!

The new imports are necessary for the architecture propagation test.

Also applies to: 16-16, 18-18

26-26: LGTM!

Setting up the volume class for use in the architecture test.

97-147: LGTM!

The test comprehensively validates that the controller properly propagates the architecture hint from the MachineClass to the ephemeral volume's OSImage data source. The test structure and assertions are well-designed.

internal/controllers/compute/machine_ephemeralvolume_controller.go (4)

13-13: LGTM!

The new imports are necessary for the architecture propagation functionality.

Also applies to: 22-22, 25-25

135-147: LGTM!

The getMachineArchitecture method correctly fetches the MachineClass and returns the architecture label value. Returning nil when the label is not present is appropriate.

149-161: LGTM!

The addArchitectureIfNeeded method correctly checks for OSImage presence and avoids overwriting an existing architecture value before setting the hint.

174-178: LGTM!

Retrieving the machine architecture early in the reconcile loop is efficient and appropriate.

api/common/v1alpha1/common_types.go

internal/controllers/compute/machine_ephemeralvolume_controller.go

github-actions bot added the size/L label Dec 19, 2025

coderabbitai bot reviewed Dec 19, 2025

View reviewed changes

api/common/v1alpha1/common_types.go Outdated Show resolved Hide resolved

internal/controllers/compute/machine_ephemeralvolume_controller.go Outdated Show resolved Hide resolved

lukasfrank force-pushed the feat/set-architecture-hints branch from 3408391 to fbf76c0 Compare December 19, 2025 14:57

Add architecture hints to ephemeral volumes

d6195e7

lukasfrank force-pushed the feat/set-architecture-hints branch from fbf76c0 to d6195e7 Compare December 19, 2025 15:02

lukasfrank marked this pull request as ready for review December 19, 2025 15:40

lukasfrank requested a review from a team as a code owner December 19, 2025 15:40

Merge branch 'main' into feat/set-architecture-hints

b13343a

lukasfrank requested a review from afritzler January 7, 2026 09:28

afritzler approved these changes Jan 7, 2026

View reviewed changes

lukasfrank merged commit 2957c0e into main Jan 7, 2026
10 of 11 checks passed

lukasfrank deleted the feat/set-architecture-hints branch January 7, 2026 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add architecture hints to ephemeral volumes #1435

Add architecture hints to ephemeral volumes #1435

Uh oh!

lukasfrank commented Dec 19, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

lukasfrank commented Dec 19, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add architecture hints to ephemeral volumes #1435

Add architecture hints to ephemeral volumes #1435

Uh oh!

Conversation

lukasfrank commented Dec 19, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Summary by CodeRabbit

Uh oh!

lukasfrank commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lukasfrank commented Dec 19, 2025 •

edited by coderabbitai bot

Loading

lukasfrank commented Dec 19, 2025 •

edited

Loading

coderabbitai bot commented Dec 19, 2025 •

edited

Loading