Repository navigation
OSAC-1270: Design - Base OS Management for Bare Metal Instances - #197
Conversation
…Instances Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
|
@adriengentil: This pull request references OSAC-1270 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
Next review available in: 36 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesThe design replaces inline bare-metal image data with immutable BMaaS DiskImage integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This design change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Client
participant BMaaSAPI
participant CatalogItem
participant DiskImage
participant Reconciler
Client->>BMaaSAPI: Create BareMetalInstance with disk_image
BMaaSAPI->>CatalogItem: Apply CatalogItem defaults
BMaaSAPI->>DiskImage: Validate reference and lifecycle
BMaaSAPI-->>Client: Return instance and warnings
Reconciler->>DiskImage: Resolve disk_image
DiskImage-->>Reconciler: Return source_ref
Reconciler->>Reconciler: Set imageURL template parameter
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
AI Design Review: EP-197Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-structured design that follows OSAC patterns with deep implementation detail; held back from a perfect score by goals framed as implementation tasks rather than user-visible outcomes. Feedback: Reframe the Goals section as user-visible outcomes rather than implementation tasks — e.g., 'Tenants select base OS images from a governed DiskImage catalog when provisioning bare-metal instances' rather than 'Replace BareMetalInstanceSpec.image with disk_image'. Expand the Summary to 3-5 sentences covering what's added, why it's valuable, and key capabilities. Consider resolving the JSONB key case ambiguity (snake_case vs camelCase in the trigger SQL) before merge rather than leaving it as a documented risk — this is a correctness issue that integration tests should catch early. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
avishayt
left a comment
avishayt
left a comment
There was a problem hiding this comment.
Two minor items, neither blocking — approve after addressing:
1. UX Alignment: @temp-api file exists
The section states "No @temp-api file exists at osac-ux/libs/ui-components/src/api/v1/baremetal-instance.ts" — but this file does exist. It defines useCreateBareMetalInstance with a hardcoded spec shape that will need diskImage added. The generated @osac/types will pick up the proto field automatically via pnpm gen-types, but the hardcoded mutation body in the hooks file needs a manual update. Please correct the section and add a brief mapping note (proto disk_image → TS diskImage).
2. Documentation dimension
This is a breaking public API change (removing BareMetalInstanceSpec.image, adding disk_image). Please add a line addressing whether API docs / CLI help text updates are in scope or deferred.
- UX Alignment: correct claim that @temp-api file doesn't exist; baremetal-instance.ts exists and useCreateBareMetalInstance needs diskImage added to its hardcoded spec type. Add field mapping table (proto disk_image → TS diskImage). - Documentation: add Documentation subsection noting REST API reference, CLI help text (--image/--image-source-type → --disk-image), and migration notes are in scope; full docs deferred to GA. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
|
Both items addressed in 629e7cd:
|
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: An exemplary design document that provides deep, implementation-ready technical detail while maintaining tight scope boundaries, following all OSAC architectural patterns, and including a comprehensive multi-level test plan. Feedback: The design is strong across all dimensions. Two minor improvements: (1) GA graduation criteria could be more measurable — 'production-hardened with validated deployment feedback' is vague compared to the concrete Dev Preview and Tech Preview conditions; consider specifying metrics thresholds or minimum deployment duration. (2) The reconciler fetches DiskImage source_ref at reconciliation time, meaning a DiskImage's source_ref update would silently change the imageURL on next reconciliation of existing BareMetalInstances — document whether this is intentional behavior or whether source_ref is also immutable on DiskImage. Critical (0)None. Important (0)None. Suggestions (3)
Review costModel: claude-opus-4-6 |
|
@avishayt the fix mentioned in my comment above was not part of this PR 🤦♂️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@enhancements/OSAC-1270-base-os-management-bmaas/design.md`:
- Around line 311-317: Add a catalog-item write trigger alongside the existing
deletion-reference check that extracts referenced DiskImage IDs from
catalog-item inserts and updates, then acquires FOR SHARE locks on each
referenced DiskImage before the write completes. Cover both insert and update
paths, and add concurrency tests proving catalog-item creation cannot race with
DiskImage deletion; preserve existing deletion validation and error behavior.
- Around line 513-516: The downgrade procedure must restore OSAC-2540’s
check_disk_image_not_in_use function and deletion trigger before removing
BMaaS-specific database additions. Update the migration rollback steps to
preserve deletion protection for compute resources, removing only the BMaaS
changes rather than leaving the original trigger and function absent.
- Around line 511-518: Add an upgrade migration path for persisted legacy inline
images in the reconciliation and instance-creation flows: preserve reading
BareMetalInstanceSpec.image and BareMetalInstanceTemplateSpecDefaults.image,
convert or backfill them to governed DiskImages and disk_image references, or
add a preflight check that blocks upgrades when such data exists. Update the
upgrade strategy and tests to cover pending instances and existing templates.
- Line 537: Update the fenced code block containing the osac baremetal-instances
command to use the shell language identifier, changing the opening fence to
```shell while preserving the command and closing fence.
- Around line 115-125: Define a repeated warnings field on the private
BareMetalInstancesCreateResponse and specify the mapping from private to public
responses so PrivateBareMetalInstancesServer.Create() warnings are preserved.
Also document warning propagation for CatalogItem Create and Update, including
how those warnings reach the API caller without being dropped.
- Around line 520-524: Correct the Version Skew Strategy documentation to state
that reserved field 7 prevents schema reuse but old clients may still send image
as an unknown wire field; document the server’s unknown-field handling policy,
and specify that InvalidArgument is returned only when disk_image is absent. Add
a compatibility test covering an old client sending image to a new server.
- Around line 279-317: Verify the persisted JSONB key casing used by
compute_instances, compute_instance_templates, bare_metal_instances, and both
catalog-item resources before finalizing the deletion trigger; align each JSONB
lookup with the actual stored schema rather than assuming camelCase or
snake_case. Add integration tests that create an active reference for each
resource and assert that deleting the referenced DiskImage raises the expected
protection error.
🪄 Autofix
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: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c552d7d1-8099-451d-864e-8e6ba3b0bf59
📒 Files selected for processing (1)
enhancements/OSAC-1270-base-os-management-bmaas/design.md
- Add warnings field to private BareMetalInstancesCreateResponse and to catalog-item create/update responses (public and private); document private-to-public propagation - Document FOR SHARE locking in validateFieldDefinitionsDiskImage for catalog-item write-side TOCTOU protection; explain why DB trigger is impractical for opaque field_definitions JSONB - Add integration tests for compute resource deletion protection (regression), JSONB key casing verification requirement, and write-side TOCTOU tests for BareMetalInstance and CatalogItem - Fix upgrade strategy: document that pending instances with spec.image but no disk_image fail reconciliation after upgrade; add preflight requirement - Fix downgrade strategy: down migration must recreate OSAC-2540's original trigger (not remove it entirely) before dropping BMaaS additions - Fix version skew: reserved field 7 causes unknown-field discard, not InvalidArgument; server returns InvalidArgument only when disk_image is absent; add compatibility test requirement - Add shell language identifier to fenced code block (MD040) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: A very strong design document with exceptional implementation depth, clear scope boundaries, comprehensive test coverage, and full alignment with OSAC architectural patterns — one of the better-calibrated designs for a narrow integration feature. Feedback: Minor improvements: the Summary section is only 2 sentences where the template recommends 3-5 — consider expanding to explicitly state the key capabilities (governed image catalog for BMaaS, deletion protection, CatalogItem defaults). Goals are implementation-focused ('Replace BareMetalInstanceSpec.image with disk_image') rather than user-visible outcomes ('Enable tenants to select pre-approved OS images from a curated catalog'); rewording would better align with the rubric. The JSONB key case mismatch risk is well-identified — consider adding an explicit pre-merge verification step to the integration test plan (e.g., 'inspect persisted row to confirm actual JSONB key before running trigger tests'). Critical (0)None. Important (0)None. Suggestions (4)
Review costModel: claude-opus-4-6 |
Replace implementation-framed goals with user-value-led statements: tenants selecting from a catalog, admins governing lifecycle, actionable feedback on deprecated/obsolete images, deletion protection, and catalog-defaulted images. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
|
Good call. Rewrote the Goals section to lead with user-visible outcomes (tenants selecting from a governed catalog, admins governing lifecycle, actionable feedback on deprecated/obsolete images, deletion protection, catalog-defaulted provisioning) rather than implementation tasks. Applied in the latest commit. |
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: A thorough, implementation-ready design that follows all OSAC patterns with deep technical specificity — proto schemas, SQL triggers, Go code, concrete error codes, and a comprehensive test plan covering unit, integration, and E2E levels with specific scenarios. Feedback: The design is strong across all dimensions. Two minor improvements: (1) Resolve the JSONB key casing uncertainty (snake_case disk_image vs camelCase diskImage) definitively in the design rather than deferring to implementation — inspect existing bare_metal_instances rows or the DAO serialization code now and pin the correct key in the trigger SQL. (2) The Observability section could note whether existing dashboards or alerts would surface DiskImage resolution failures during reconciliation, or whether operators should add a specific alert rule for the 'failed to fetch disk image' error pattern described in Support Procedures. Critical (0)None. Important (1)
Suggestions (2)
Review costModel: claude-opus-4-6 |
OSAC-2540's design is the source of truth: compute resources use camelCase (diskImage/specDefaults), bare-metal resources use snake_case (disk_image). Remove 'implementors must verify' hedges; update Risk entry and integration test note accordingly. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: This is a high-quality design document that demonstrates deep familiarity with OSAC patterns, provides implementation-ready detail (proto schemas, SQL triggers, Go code), clearly bounds its scope relative to OSAC-2540, and includes a thorough multi-level test plan — scoring full marks across all four criteria. Feedback: The design is ready for merge as-is. Two minor polish opportunities: (1) the Summary could expand from one sentence to 3-5 sentences to better match the template guidance — explicitly listing key capabilities (governed catalog selection, lifecycle warnings, deletion protection, CatalogItem defaults) would help readers who skim. (2) The Installation dimension could get an explicit one-line 'No installation changes required — no new CRDs, operators, or Helm values are introduced' to preempt reviewer questions, even though the answer is obvious from context. Critical (0)None. Important (0)None. Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: This is an exceptionally well-crafted design document that follows all OSAC patterns, provides deep implementation detail (full proto schemas, SQL DDL, Go code), covers all lifecycle operations with specific error codes, and includes a comprehensive test plan at all levels — earning full marks across all four criteria. Feedback: The summary section should be expanded from one sentence to 3-5 sentences per template guidance — cover what's added, why it's valuable, and key capabilities separately. Graduation criteria for Tech Preview and GA stages would benefit from more measurable conditions (e.g., 'multi-tenant DiskImage isolation passes E2E with N concurrent tenants' rather than 'validated in multi-tenant environment'). Minor: the CLI changes (replacing --image/--image-source-type flags with --disk-image) are mentioned in Documentation but not detailed in Implementation Details — consider adding a brief CLI implementation section or noting it follows existing CLI patterns. Critical (0)None. Important (0)None. Suggestions (4)
Review costModel: claude-opus-4-6 |
- Add CLI implementation section: --image/--image-source-type → --disk-image flag replacement, following the kubectl-style pattern and referencing the compute instance precedent from OSAC-2540 - Clarify UI scope in UX Alignment: form changes deferred until OSAC-2540's DiskImage picker component is available; @osac/types update (pnpm gen-types) can land with the backend Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
|
@coderabbitai review |
|
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: A thorough and implementation-ready design that follows OSAC patterns consistently, provides exceptional technical detail (proto schemas, SQL triggers, Go code, sequence diagrams), and includes a strong test plan with specific scenarios at all levels — one of the strongest designs reviewed against this rubric. Feedback: Minor improvements: expand the Summary to 3-5 sentences covering key capabilities (currently 1 sentence + PRD link); make GA graduation criteria more measurable (e.g., 'zero DiskImage-related incidents over N days in staging' rather than 'production-hardened'); and add a brief note in Observability about specific log messages or metrics operators should watch for when DiskImage resolution fails in the reconciler (currently says 'no new observability changes' but new failure modes warrant at least naming the existing signals that surface them). Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…umbers Remove server handler logic, reconciler Go code, CLI flag details, and SQL migrations. Keep proto schema changes only, without field numbers. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
|
@adriengentil: This pull request references OSAC-1270 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: A well-crafted design that cleanly extends the OSAC-2540 DiskImage pattern to BMaaS with narrow, well-understood integration points, comprehensive test coverage, and strong adherence to established OSAC architectural patterns. Feedback: The proto snippet (Implementation Details) reserves only the name 'image' but not the field number — add 'reserved 7;' alongside 'reserved "image";' and include the field number assignment for disk_image (field 10) in the snippet, since the Version Skew section references these numbers but the proto code doesn't show them. The JSONB key casing discrepancy flagged in the PR description (OSAC-2540 indexes use camelCase 'diskImage' while this design's test plan asserts 'all snake_case' enforced by UseProtoNames: true) should be resolved in the design text itself — either confirm the casing after checking the actual DAO serialization, or note it as an implementation-time verification step rather than asserting snake_case as fact. Consider adding a brief note in Risks about what happens if OSAC-2540 changes its trigger function signature after OSAC-1270 merges — since the coupling is at the DB function level, a future OSAC-2540 refactor could silently break BMaaS deletion protection. Critical (0)None. Important (2)
Suggestions (2)
Review costModel: claude-opus-4-6 |
reserved "image" alone doesn't prevent number reuse — add reserved 7/1 to BareMetalInstanceSpec and BareMetalInstanceTemplateSpecDefaults. Remove stale field 10 reference from Version Skew section. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design that correctly maps to the existing codebase, follows established OSAC patterns, and provides a comprehensive test plan — minor gaps around imageSourceType handling and JSONB key casing resolution are the only notable weaknesses. Feedback: Two items need explicit resolution before implementation: (1) The design omits what happens to the Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
mutateBMI currently injects both imageURL and imageSourceType from spec.image. Explicitly state that imageSourceType is removed since DiskImage abstracts the source type and AAP templates don't consume it. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: A high-quality, well-structured design that cleanly extends OSAC-2540's DiskImage governance to the BMaaS provisioning path with thorough test coverage, sound architectural decisions, and clear risk identification. Feedback: The JSONB key casing discrepancy between this design and OSAC-2540 is the most important implementation risk — OSAC-2540's migration SQL uses camelCase keys (diskImage, specDefaults) in JSONB path expressions, while this design's integration tests assert snake_case (disk_image, spec_defaults) based on UseProtoNames: true. Before writing migration SQL, verify the actual persisted keys in the database; a mismatch silently breaks deletion protection with no error at write time. Additionally, the claim that AAP templates do not consume imageSourceType for bare-metal provisioning should be verified with evidence (e.g., grep the AAP role templates) — if any role uses it, dropping it breaks provisioning. Consider including the actual trigger function SQL in the design to make the migration reviewable before implementation. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
useCreateBareMetalInstance uses MessageInitShape<typeof BareMetalInstanceSchema> (generated), not a hardcoded spec. pnpm gen-types handles the hook; only the form component needs updating. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: A well-crafted design document that extends an existing pattern (OSAC-2540 DiskImage) to a new resource type (BMaaS) with narrow integration scope, thorough failure handling, comprehensive test coverage, and honest trade-off analysis across four substantive alternatives. Feedback: The JSONB key casing risk (camelCase vs snake_case due to UseProtoNames: true) is called out in the PR body as a review concern and covered by integration tests, but should also appear in the Risks and Mitigations section — silent deletion-protection failure is serious enough to warrant a documented risk with the mitigation being pre-implementation verification and the explicit regression test. The support procedure for 'DiskImage deletion returns FailedPrecondition' should mention CatalogItem references alongside BareMetalInstance references, since the deletion trigger checks both tables but the resolution steps only show how to find BareMetalInstances. Critical (0)None. Important (1)
Suggestions (2)
Review costModel: claude-opus-4-6 |
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
Do we want to do any checking or labeling to show that some images can be used only with VMaaS or BMaaS? Do we know this beforehand?
|
|
||
| ## Proposal | ||
|
|
||
| `BareMetalInstanceSpec.disk_image` replaces the inline `image` field as a reference to a DiskImage by ID. At creation time, the server resolves the DiskImage reference (from the user or from the CatalogItem's `field_definitions`), validates it against the DiskImage lifecycle and visibility rules, and persists the BareMetalInstance. The reconciler then fetches the DiskImage's `source_ref` and injects it as `params["imageURL"]` — the same JSON template parameter the AAP provisioning roles already consume. `imageSourceType`, previously injected alongside `imageURL` from the inline `spec.image.source_type`, is dropped: DiskImage abstracts the source type, and AAP provisioning templates do not consume `imageSourceType` for bare-metal provisioning. This keeps the operator CRD and all downstream provisioning code unchanged. |
There was a problem hiding this comment.
How do we want to handle the default mechanism? Will the disk image be required when creating a bare metal instance?
I would prefer to always require an image, but this would mean changes to aap and CI jobs.
There was a problem hiding this comment.
In this scope, when I talk about "default", it's set by the catalog item, not by the template/ansible which we should remove. I'll try to make it clearer.
There was a problem hiding this comment.
Fixed. disk_image is now always required — creation fails with InvalidArgument if neither the user nor the CatalogItem supplies it. There is no system-level default. The only default mechanism is CatalogItem field_definitions (what I was referring to as 'default' earlier). The design now explicitly documents that the AAP provisioning template's current hardcoded default imageURL must be removed as part of this feature, so the template relies entirely on the value injected by the reconciler.
|
|
||
| User->>API: Create BareMetalInstance (disk_image=<id> or omitted) | ||
| API->>DB: Get CatalogItem → applyFieldDefinitions | ||
| Note over API: disk_image default applied if not provided |
There was a problem hiding this comment.
Same as previous comment, do we really want this?
There was a problem hiding this comment.
See reply to the first comment — updated to explicitly require disk_image with no system fallback.
The mid/long term is to have the same diskimage for VMaaS and BMaaS. @vladikr added OCI artifact support in kubevirt, so VMaaS can consume the same disk format as BMaaS. On the long term, we want to support bootc images directly for both. I also know there's discussion to allow users to import their own OS Images, that would be a good entry point to accommodate different expected formats for a single image. We can add a label or an annotation to make the image compatible only for one or the other service, but it's not the direction we want to take on the long term. I think that should be part of another feature as this design is about DiskImage integration in BMaaS. I think image compatibility (or capabilities), or checks should be part of DiskImage itself and decisions be aligned with VMaaS. |
- disk_image is required: creation fails (InvalidArgument) if neither user nor CatalogItem provides it — no system-level fallback - Non-Goals: explicitly exclude system/Ansible-provided default - Goals: clarify CatalogItem field_definitions is the only default source - Summary and Implementation Details: document that osac-aap templates currently carry a hardcoded default imageURL that must be removed as part of this feature (AAP change lands in same PR as fulfillment-service) - Sequence diagram note: precise about CatalogItem default path and InvalidArgument when disk_image is still missing after defaults Addresses: carbonin's review comments at lines 52, 77, 96 Assisted-by: Claude Code <noreply@anthropic.com>
AI Design Review: EP-197Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design that extends the existing OSAC-2540 DiskImage pattern to BMaaS with minimal blast radius, comprehensive test coverage, and sound architectural decisions — the main actionable gap is that the JSONB key casing mismatch risk is flagged only in the PR description and should be surfaced in the design document's own Risks section. Feedback: The JSONB key casing concern (OSAC-2540's compute trigger uses camelCase keys while UseProtoNames: true produces snake_case) is only called out in the PR description's 'Requesting Review On' section — add it to the Risks and Mitigations section of the design document itself, since a mismatch silently breaks deletion protection without any error at write time. Consider adding a down-migration test to the Test Plan: the downgrade procedure must recreate OSAC-2540's original trigger function (compute-only), and verifying that the down migration correctly restores compute deletion protection would reduce risk on a particularly coupled migration step. The CLI transition story (replacing --image/--image-source-type flags with --disk-image) could be mentioned in the Version Skew Strategy to guide client-side migration tooling. Critical (0)None. Important (2)
Suggestions (2)
Review costModel: claude-opus-4-6 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, avishayt, carbonin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Design: Base OS Management for Bare Metal Instances
Jira: https://redhat.atlassian.net/browse/OSAC-1270
PRD:
prd.mdSummary
This design integrates the DiskImage resource (OSAC-2540) into the BMaaS provisioning path.
BareMetalInstanceSpec.image(inline OCI URL) is replaced by adisk_imagereference to a governed DiskImage. The fulfillment-service reconciler resolves the DiskImage'ssource_refand injects it asimageURLin the template parameters — no changes to the bare-metal-fulfillment-operator CRD or osac-aap provisioning templates. DiskImage deletion protection is extended tobare_metal_instancesandbare_metal_instance_catalog_itemsvia an updated database trigger.Requesting Review On
check_disk_image_not_in_usefunction from OSAC-2540 is dropped and recreated with BMI table checks added; reviewers should verify this is safe and consistent with how the instance_type trigger was handled in migration 56.diskImage,specDefaults), while the DAO'sUseProtoNames: truesetting produces snake_case. Implementors must verify the actual persisted key fordisk_imageinbare_metal_instancesbefore writing the migration SQL — a mismatch silently breaks deletion protection with no error at write time.imageSourceTypedropped —mutateBMIcurrently injects bothimageURLandimageSourceTypefromspec.image. This design dropsimageSourceTypeon the grounds that DiskImage abstracts the source type and AAP templates do not consume it. Reviewers should confirm the BMaaS provisioning templates truly do not needimageSourceType; if they do, it should be derived fromDiskImage.source_type.disk_imageonBareMetalInstanceTemplate— defaults are carried only onBareMetalInstanceCatalogItem.field_definitions. This is a locked decision from the PRD; reviewers should flag if this is too restrictive for operator workflows.guest_os_familynot forwarded to AAP — the DiskImage's OS family is not injected as a template parameter. Reviewers should confirm the BMaaS provisioning templates do not need it today.Documents
design.md— technical design documentHow to Review