[AISOS-358] Implementation for AISOS-358#10507
Open
eshulman2 wants to merge 7 commits intoopenshift:mainfrom
Open
[AISOS-358] Implementation for AISOS-358#10507eshulman2 wants to merge 7 commits intoopenshift:mainfrom
eshulman2 wants to merge 7 commits intoopenshift:mainfrom
Conversation
added 6 commits
April 16, 2026 12:42
Detailed description: - Added BootstrapFlavor string field to Platform struct in pkg/types/openstack/platform.go - Field uses json:"bootstrapFlavor,omitempty" tag for backward compatibility - Field is optional; when not set, bootstrap uses the control plane flavor (existing behavior) - No deepcopy changes needed: plain string fields are covered by the existing *out = *in assignment - All existing unit tests pass Closes: AISOS-360
…nition Auto-committed by Forge container fallback.
…rm validation
Detailed description:
- cloudinfo.go: Fetch bootstrapFlavor from OpenStack Compute API when specified,
following the same pattern used for DefaultMachinePlatform and ControlPlane flavors.
A 404 from OpenStack means the flavor is absent and is recorded as such (not fetched
into ci.Flavors), enabling the validator to report it as not found.
- platform.go: Add validateBootstrapFlavor() that returns a field.NotFound error when
the specified bootstrapFlavor is absent from ci.Flavors. The function is a no-op when
BootstrapFlavor is empty, preserving backward compatibility.
- platform_test.go: Add TestBootstrapFlavor covering three scenarios:
1. Flavor not specified -> no error (backward compatible)
2. Valid flavor present in CloudInfo -> no error
3. Invalid flavor absent from CloudInfo -> field.NotFound error
Closes: AISOS-361
…Flavor Detailed description: - Modified generateMachineSpec() in pkg/asset/machines/openstack/openstackmachines.go to select platform.BootstrapFlavor for the bootstrap machine when the field is set. Falls back to mpool.FlavorName when BootstrapFlavor is empty, preserving backward compatibility. Master machines continue to use FlavorName unconditionally. - Added TestBootstrapFlavorSelection in openstackmachines_test.go with three table-driven sub-tests covering: bootstrap+flavor set, bootstrap+flavor empty (fallback), and master+flavor set (master unaffected). Closes: AISOS-362
Detailed description: - pkg/asset/machines/openstack/machines.go: Updated generateProviderSpec() to check platform.BootstrapFlavor when role == bootstrapRole. When BootstrapFlavor is set, it is used as the flavor; otherwise falls back to mpool.FlavorName (control plane flavor). Master and worker machines are unaffected. - pkg/asset/machines/openstack/machines_test.go: Added TestMAPIBootstrapFlavorSelection with three table-driven sub-tests covering: bootstrap uses BootstrapFlavor when specified, bootstrap falls back to control plane flavor when empty, master always uses FlavorName. This mirrors the equivalent change already made in the CAPI code path (openstackmachines.go) and ensures consistency across both CAPI and MAPI machine generation for the bootstrapFlavor feature. Closes: AISOS-363
Detailed description: - Added bootstrapFlavor to the Cluster-scoped properties list with a description of its purpose, optional nature, and fallback behavior - Added a new 'Bootstrap flavor for NFV/NUMA workloads' example section explaining why the field is needed (NFV flavors with hw:numa_nodes or CPU pinning may cause scheduling issues for the temporary bootstrap node) and providing a complete install-config.yaml YAML example - Updated the Table of Contents to include the new example subsection - Updated the Machine pools bootstrap note to reference bootstrapFlavor as a way to independently override the bootstrap instance flavor - Fallback chain is clearly documented: bootstrapFlavor falls back to the controlPlane machine pool type, which in turn falls back to defaultMachinePlatform.type Closes: AISOS-365
WalkthroughAdds an optional OpenStack Changes
sequenceDiagram
participant IC as InstallConfig
participant CI as CloudInfo
participant OS as OpenStack API
participant VAL as Validator
participant MG as MachineGenerator
rect rgba(0,128,255,0.5)
IC->>CI: provide OpenStack.BootstrapFlavor (optional)
end
rect rgba(0,200,100,0.5)
CI->>OS: get flavor info (if BootstrapFlavor set and not cached)
OS-->>CI: flavor metadata / 404
end
rect rgba(255,165,0,0.5)
VAL->>CI: validate platform.BootstrapFlavor exists in CI.Flavors
CI-->>VAL: success or NotFound
end
rect rgba(128,0,128,0.5)
MG->>CI: request flavor for machine role (bootstrap/master)
CI-->>MG: flavor name (BootstrapFlavor or pool flavor)
MG->>OS: create machine with selected flavor
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/openstack/customization.md`:
- Around line 156-160: The second blockquote has an extra blank line breaking
the blockquote sequence and triggering MD028; fix the markdown by making the
blockquote lines contiguous (ensure each line begins with '>' with no blank line
between the two notes), so the two notes about bootstrapFlavor form a single
proper blockquote sequence in the content that includes both the "The bootstrap
instance..." and "If `bootstrapFlavor` is not set..." lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ff4d9ab8-3912-4028-b492-f9ea35e18539
📒 Files selected for processing (10)
.gitignoredocs/user/openstack/customization.mdpkg/asset/installconfig/openstack/validation/cloudinfo.gopkg/asset/installconfig/openstack/validation/platform.gopkg/asset/installconfig/openstack/validation/platform_test.gopkg/asset/machines/openstack/machines.gopkg/asset/machines/openstack/machines_test.gopkg/asset/machines/openstack/openstackmachines.gopkg/asset/machines/openstack/openstackmachines_test.gopkg/types/openstack/platform.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Regenerate data/data/install.openshift.io_installconfigs.yaml to include the bootstrapFlavor field added to the OpenStack Platform type. This fixes the ci/prow/verify-codegen CI failure caused by codegen drift. Closes: AISOS-358-ci-fix
Member
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/user/openstack/customization.md (1)
156-160:⚠️ Potential issue | 🟡 MinorFix the blockquote blank line for MD028.
Line 158 is a plain blank line between adjacent blockquote notes, which markdownlint flags as
MD028.🧹 Proposed markdown fix
> **Note** > The bootstrap instance is temporary and is removed once the cluster is fully operational. The `bootstrapFlavor` has no effect on control plane or worker machine flavors. - +> > **Note** > If `bootstrapFlavor` is not set, the bootstrap instance uses the flavor from the `controlPlane` machine pool. If that is also unset, it falls back to `defaultMachinePlatform.type`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/openstack/customization.md` around lines 156 - 160, Remove the blank line separating the two adjacent blockquote notes so they remain as two consecutive blockquotes (fixing MD028); locate the two "**Note**" blockquotes that mention `bootstrapFlavor` and `defaultMachinePlatform.type` and ensure there is no empty line between them so each stays its own blockquote paragraph.
🧹 Nitpick comments (2)
pkg/asset/machines/openstack/machines.go (1)
254-267: Flavor selection logic looks correct and mirrors the CAPI path.Bootstrap-only override with fallback to
mpool.FlavorNamematchesgenerateMachineSpecinopenstackmachines.go, preserving backward compatibility whenBootstrapFlavoris unset.Nit: the comment on line 256 says “Master machines always use FlavorName” but this fallback also applies to worker role here (this function is used for all roles via
Machines(...)). Consider tightening the comment to avoid implying role-specific behavior that isn't enforced at this site.Proposed comment tweak
- // Determine the flavor to use for this machine. - // Bootstrap machines use BootstrapFlavor when specified; otherwise fall back to the - // control plane flavor from the machine pool. Master machines always use FlavorName. + // Determine the flavor to use for this machine. + // Bootstrap machines use platform.BootstrapFlavor when specified; all other roles + // (and bootstrap when BootstrapFlavor is unset) use the machine pool's FlavorName.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/openstack/machines.go` around lines 254 - 267, The comment describing flavor selection is misleading because it implies role-specific behavior ("Master machines always use FlavorName") even though this function (used by Machines(...) for all roles) applies the same fallback to mpool.FlavorName; update the comment above the flavor selection logic to accurately state that bootstrapRole uses platform.BootstrapFlavor when set, otherwise all roles (including master/worker) fall back to mpool.FlavorName; reference the variables flavorName, mpool.FlavorName, platform.BootstrapFlavor, role and bootstrapRole so the intent is clear.pkg/asset/installconfig/openstack/validation/cloudinfo.go (1)
196-208: Repeated flavor-fetch block — consider extracting a helper.This is the fourth near-identical copy of the "if flavorName != '' && not seen → getFlavor → swallow 404 → store" pattern (lines 153-165, 167-179, 181-195, now 196-208). The new block is correct and consistent with the others, so no functional concern, but the duplication is now pronounced enough that a small helper would improve maintainability and ensure future changes (e.g., logging, retries, different error handling) stay uniform.
Proposed refactor
// fetchFlavorIfNeeded fetches the given flavor into ci.Flavors unless already present. // A 404 response is silently ignored; validation elsewhere reports the missing flavor. func (ci *CloudInfo) fetchFlavorIfNeeded(ctx context.Context, flavorName string) error { if flavorName == "" { return nil } if _, seen := ci.Flavors[flavorName]; seen { return nil } flavor, err := ci.getFlavor(ctx, flavorName) if gophercloud.ResponseCodeIs(err, http.StatusNotFound) { return nil } if err != nil { return err } ci.Flavors[flavorName] = flavor return nil }Then each call site collapses to e.g.
if err := ci.fetchFlavorIfNeeded(ctx, ic.OpenStack.BootstrapFlavor); err != nil { return err }.Also minor: line 196 drops the blank separator line that existed between the previous flavor blocks (195
}is immediately followed by the new block's comment on 197). Cosmetic only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/openstack/validation/cloudinfo.go` around lines 196 - 208, Extract the repeated "check flavor name, skip if blank or already present, call ci.getFlavor, ignore 404, store in ci.Flavors or return error" logic into a helper on CloudInfo (e.g. func (ci *CloudInfo) fetchFlavorIfNeeded(ctx context.Context, flavorName string) error) that returns nil for empty name, for already-seen names, and when getFlavor returns a 404, otherwise returns any error and sets ci.Flavors[flavorName]=flavor; then replace each duplicated block (the four places that reference ic.OpenStack.BootstrapFlavor, ic.OpenStack.MasterFlavorName, etc.) with calls like if err := ci.fetchFlavorIfNeeded(ctx, ic.OpenStack.BootstrapFlavor); err != nil { return err } to keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/user/openstack/customization.md`:
- Around line 156-160: Remove the blank line separating the two adjacent
blockquote notes so they remain as two consecutive blockquotes (fixing MD028);
locate the two "**Note**" blockquotes that mention `bootstrapFlavor` and
`defaultMachinePlatform.type` and ensure there is no empty line between them so
each stays its own blockquote paragraph.
---
Nitpick comments:
In `@pkg/asset/installconfig/openstack/validation/cloudinfo.go`:
- Around line 196-208: Extract the repeated "check flavor name, skip if blank or
already present, call ci.getFlavor, ignore 404, store in ci.Flavors or return
error" logic into a helper on CloudInfo (e.g. func (ci *CloudInfo)
fetchFlavorIfNeeded(ctx context.Context, flavorName string) error) that returns
nil for empty name, for already-seen names, and when getFlavor returns a 404,
otherwise returns any error and sets ci.Flavors[flavorName]=flavor; then replace
each duplicated block (the four places that reference
ic.OpenStack.BootstrapFlavor, ic.OpenStack.MasterFlavorName, etc.) with calls
like if err := ci.fetchFlavorIfNeeded(ctx, ic.OpenStack.BootstrapFlavor); err !=
nil { return err } to keep behavior identical.
In `@pkg/asset/machines/openstack/machines.go`:
- Around line 254-267: The comment describing flavor selection is misleading
because it implies role-specific behavior ("Master machines always use
FlavorName") even though this function (used by Machines(...) for all roles)
applies the same fallback to mpool.FlavorName; update the comment above the
flavor selection logic to accurately state that bootstrapRole uses
platform.BootstrapFlavor when set, otherwise all roles (including master/worker)
fall back to mpool.FlavorName; reference the variables flavorName,
mpool.FlavorName, platform.BootstrapFlavor, role and bootstrapRole so the intent
is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c91cf3bf-fc17-4cf5-9a7a-d8476c803b10
📒 Files selected for processing (11)
.gitignoredata/data/install.openshift.io_installconfigs.yamldocs/user/openstack/customization.mdpkg/asset/installconfig/openstack/validation/cloudinfo.gopkg/asset/installconfig/openstack/validation/platform.gopkg/asset/installconfig/openstack/validation/platform_test.gopkg/asset/machines/openstack/machines.gopkg/asset/machines/openstack/machines_test.gopkg/asset/machines/openstack/openstackmachines.gopkg/asset/machines/openstack/openstackmachines_test.gopkg/types/openstack/platform.go
Contributor
|
@eshulman2: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Member
|
We've not agreed to this feature as a team. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a new
bootstrapFlavorconfiguration option for OpenStack deployments, enabling users to specify a separate flavor for the bootstrap machine independent of the control plane flavor. This addresses NFV/NUMA-constrained deployments where control plane flavors with CPU pinning or NUMA node requirements may cause scheduling issues for the temporary bootstrap node, which doesn't require these specialized resources.Changes
Type Definition
BootstrapFlavorfield toPlatformstruct inpkg/types/openstack/platform.gowithjson:"bootstrapFlavor,omitempty"tag for backward compatibilityValidation
pkg/asset/openstack/validation/cloudinfo.go: Extended flavor fetching to include bootstrapFlavor from OpenStack Compute APIpkg/asset/openstack/validation/platform.go: AddedvalidateBootstrapFlavor()function that returnsfield.NotFounderror when specified flavor doesn't existpkg/asset/openstack/validation/platform_test.go: AddedTestBootstrapFlavorcovering not specified, valid, and invalid flavor scenariosCAPI Machine Generation
pkg/asset/machines/openstack/openstackmachines.go: ModifiedgenerateMachineSpec()to useBootstrapFlavorfor bootstrap machines when specified, falling back to control plane flavorpkg/asset/machines/openstack/openstackmachines_test.go: AddedTestBootstrapFlavorSelectionwith table-driven testsMAPI Machine Generation
pkg/asset/machines/openstack/machines.go: UpdatedgenerateProviderSpec()with same bootstrap flavor selection logic for consistency across both code pathspkg/asset/machines/openstack/machines_test.go: AddedTestMAPIBootstrapFlavorSelectionwith parallel test coverageDocumentation
docs/user/openstack/customization.md: AddedbootstrapFlavorto cluster-scoped properties, documented NFV/NUMA use case with example configuration, updated machine pools section to reference the new fieldImplementation Notes
BootstrapFlavorfield is a plainstringtype, so no deepcopy generation changes were required—existing*out = *inhandling covers scalar fieldsfield.NotFoundplatform.BootstrapFlavorfor bootstrap role, fall back tompool.FlavorNamewhen emptybootstrapFlavorpreserves existing behavior of using control plane flavorTesting
go build,go vet, andgofmtall pass cleanlyRelated Tickets
Generated by Forge SDLC Orchestrator
Summary by CodeRabbit
New Features
Documentation
Tests
Chores