OSAC-2047, OSAC-2250, OSAC-2254, OSAC-1496, OSAC-3997, OSAC-3998: BMaaS networking — reconcileNetworking, IP discovery, auto-cleanup, BM DNAT, AAP templates, Helm ConfigMaps - #235
Open
danmanor wants to merge 21 commits into
Conversation
|
@danmanor: This pull request references OSAC-1448 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 epic 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBareMetalInstance now provisions network attachments, discovers DHCP-assigned IP addresses, tracks networking jobs, and cleans up related resources. OSAC controllers now resolve implementation strategies through a shared dispatcher and support CUDN task delegation. ChangesBareMetalInstance networking
Dispatcher-based network manager resolution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery.go (1)
87-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap this error for context.
reconcileProvisioningwraps the same call asfailed to compute desired config version: %w. Here the raw error propagates, so a log line cannot distinguish IP discovery from provisioning.♻️ Suggested change
if err != nil { - return ctrl.Result{}, err + return ctrl.Result{}, fmt.Errorf("failed to compute desired config version for IP discovery: %w", err) }🤖 Prompt for 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. In `@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery.go` around lines 87 - 89, Wrap the error returned by the IP discovery call in the surrounding reconciliation flow before returning it, adding context that identifies the failure as IP discovery while preserving the original error with error wrapping. Keep the existing ctrl.Result{} return unchanged.bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery_test.go (1)
130-130: 📐 Maintainability & Code Quality | 🔵 TrivialNo test covers
applyIPDiscoveryResults.
AAPClientis nil in every case, so artifact retrieval,DHCPLeaseResultunmarshalling, lease-to-attachment matching bysubnetRef, and the parse-failure path are all unexercised. The PR lists the DHCP artifact format as unverified, which makes this the highest-value gap in the suite. Introducing a small interface forGetJobwould let the test inject a fake and assert thatStatus.NetworkAttachmentStatuses[i].IPAddressis populated.Do you want me to draft those tests?
🤖 Prompt for 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. In `@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery_test.go` at line 130, The test suite lacks coverage for applyIPDiscoveryResults because AAPClient is always nil. Introduce a small GetJob interface that the AAP client dependency can satisfy, inject a fake implementation in bare-metal instance IP discovery tests, and add cases covering artifact retrieval, DHCPLeaseResult unmarshalling, subnetRef lease-to-attachment matching with IPAddress population, and parse-failure handling.bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.go (1)
196-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPolling loops in both new test files discard errors. Each loop calls the reconcile function and
k8sClient.Status().Updatewith the results assigned to_. When a condition never reaches the expected value, the assertion after the loop fails with no indication of why. The shared fix is to assert on both return values inside the loop.
bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.go#L196-L206: assert the error fromreconcileNetworkingand fromStatus().Updateinside the loop; apply the same change to the failure-path loop at Lines 228-237, matching that path's expected return.bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery_test.go#L154-L162: assert the error fromreconcileIPDiscoveryand fromStatus().Updateinside the loop; apply the same change to the job-tracking loop at Lines 188-192.As per path instructions for
**/*.go: "Never ignore error returns".🤖 Prompt for 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. In `@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.go` around lines 196 - 206, The polling loops discard errors from reconcile and status-update operations. In bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.go:196-206, assert both return values from reconcileNetworking and k8sClient.Status().Update; apply the same assertions to the failure-path loop at lines 228-237, preserving that path’s expected reconcile result. In bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery_test.go:154-162, assert errors from reconcileIPDiscovery and Status().Update, and make the same change in the job-tracking loop at lines 188-192; do not ignore any error returns.Source: Path instructions
bare-metal-fulfillment-operator/cmd/main.go (1)
414-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a config struct for the reconciler dependencies.
NewBareMetalInstanceReconcilernow takes three parameters of the identical typeprovisioning.ProvisioningProviderin sequence, followed by fourtime.Durationparameters. A swapped argument compiles cleanly and fails only at runtime. A struct with named fields removes that class of error and keeps future additions source-compatible.♻️ Sketch
type BareMetalInstanceReconcilerConfig struct { ProvisioningProvider provisioning.ProvisioningProvider NetworkingProvider provisioning.ProvisioningProvider IPDiscoveryProvider provisioning.ProvisioningProvider AAPClient *aap.Client // intervals... }Also applies to: 484-497
🤖 Prompt for 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. In `@bare-metal-fulfillment-operator/cmd/main.go` around lines 414 - 421, Introduce a named configuration struct for the dependencies and interval settings used by NewBareMetalInstanceReconciler, with distinct fields for ProvisioningProvider, NetworkingProvider, IPDiscoveryProvider, and AAPClient. Update setupBareMetalInstanceController and the reconciler construction call to pass and consume this config by field name, preserving existing behavior while preventing same-typed arguments from being swapped.bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking.go (1)
69-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse keyed fields in the anonymous struct literal.
The literal at Line 74 to Line 76 relies on positional order. If a field is added or reordered in the type at Line 70 to Line 72, the hash input changes silently and every BMI recomputes its config version. Keyed fields remove that coupling.
♻️ Proposed refactor
}{ - bareMetalInstance.Spec.NetworkAttachments, - bareMetalInstance.Spec.ExternalHostID, - bareMetalInstance.Spec.HostClass, + NetworkAttachments: bareMetalInstance.Spec.NetworkAttachments, + ExternalHostID: bareMetalInstance.Spec.ExternalHostID, + HostClass: bareMetalInstance.Spec.HostClass, })🤖 Prompt for 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. In `@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking.go` around lines 69 - 77, Update the anonymous struct literal passed to provisioning.ComputeDesiredConfigVersion to use keyed fields for NetworkAttachments, ExternalHostID, and HostClass, preserving the existing values while removing reliance on field order.bare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup_test.go (2)
91-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe teardown can block when the test body fails.
The BMI is created with
BareMetalInstanceCleanupFinalizerat Line 87.AfterEachcallsDelete, which only marks the object for deletion. If the assertion at Line 97 or Line 98 fails, the finalizer remains and the object stays in thedefaultnamespace for the rest of the suite.Remove the finalizer in
AfterEachbefore deleting.♻️ Proposed change
AfterEach(func() { + fresh := &v1alpha1.BareMetalInstance{} + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(bmi), fresh); err == nil { + if controllerutil.RemoveFinalizer(fresh, BareMetalInstanceCleanupFinalizer) { + _ = k8sClient.Update(ctx, fresh) + } + } _ = k8sClient.Delete(ctx, bmi) })🤖 Prompt for 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. In `@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup_test.go` around lines 91 - 93, Update the AfterEach teardown for the BMI to remove BareMetalInstanceCleanupFinalizer before calling k8sClient.Delete, ensuring cleanup completes even when the test body assertions fail.
46-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the label-selective deletion contract.
Both tests exercise only the empty paths. The behavior that carries real risk is untested:
- An
ExternalIPAttachmentlabeledauto-provisioned=truewith the matching BMI UID is deleted, and the reconcile requeues withdone=false.- An
ExternalIPwith the same labels is deleted only after the attachment is gone.- A manually created
ExternalIPwithout theauto-provisionedlabel survives. The doc comment atbaremetalinstance_cleanup.goLine 38 to Line 40 promises this. Nothing verifies it. A regression here deletes tenant-owned resources.- An
ExternalIPcarrying a different BMI UID survives.
suite_test.gonow registers the osac-operator CRDs, so these objects can be created in envtest.🤖 Prompt for 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. In `@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup_test.go` around lines 46 - 107, Extend the reconcileAutoCleanup tests to cover label-selective deletion: create labeled auto-provisioned ExternalIPAttachment and matching ExternalIP resources, verify the attachment is deleted first and reconciliation returns done=false, then verify the ExternalIP is deleted after the attachment is gone. Also verify ExternalIP resources without the auto-provisioned label or with a different BMI UID remain undeleted, using the registered CRD types and the BMI UID labels.bare-metal-fulfillment-operator/internal/controller/suite_test.go (1)
74-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider failing fast when the osac-operator CRD directory is missing.
Line 77 adds a cross-module CRD path.
ErrorIfCRDPathMissing: falseat Line 79 hides a missing directory. If the path is wrong or the sibling module is not checked out, envtest starts withoutExternalIPandExternalIPAttachmentCRDs. The cleanup tests then fail insidereconcileAutoCleanupwith a "no matches for kind" list error instead of a clear setup failure.Either set
ErrorIfCRDPathMissing: true, or assert the directory exists before starting envtest.🤖 Prompt for 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. In `@bare-metal-fulfillment-operator/internal/controller/suite_test.go` around lines 74 - 79, Make envtest fail fast when the external osac-operator CRD directory is unavailable by changing the CRD setup around CRDDirectoryPaths and ErrorIfCRDPathMissing to enable missing-path errors, or explicitly validate that directory before envtest starts. Preserve the existing CRD paths while ensuring missing ExternalIP and ExternalIPAttachment definitions produce a clear setup failure.
🤖 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
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup.go`:
- Around line 56-61: Confirm the namespace used for auto-provisioned ExternalIP
resources by checking their creation paths and RBAC, then update all three list
calls in
bare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup.go
(lines 56-61, 90-94, and 142-147) within the cleanup and
addCleanupFinalizerIfNeeded flows to include client.InNamespace with that
namespace. Apply the same correct namespace scope to both ExternalIPList calls
and the ExternalIPAttachmentList call, preserving their existing label filters
and deletion behavior.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_controller.go`:
- Around line 499-511: Move the IP discovery block containing
reconcileIPDiscovery out of the TemplateID provisioning branch in the main
reconciliation flow. Gate it on provisioning readiness so instances with an
empty or shared.OsacNoopTemplate still invoke reconcileIPDiscovery when
networkAttachments are configured, while retaining the existing error,
progressing phase, and zero-result handling.
- Around line 61-62: Add kubebuilder RBAC markers alongside the existing
osac.openshift.io permissions for externalipattachments and externalips,
granting list, get, and delete verbs so reconcileAutoCleanup can list and remove
these resources during BMI deletion.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery.go`:
- Around line 212-223: Update the network attachment status synchronization loop
in the IP discovery function to refresh each existing status entry from its
corresponding spec attachment, especially the Primary field, instead of only
appending missing entries. Preserve existing status values as needed while
ensuring SubnetRef, Interface, and Primary match the spec, and reuse the
normalization approach from applyIPDiscoveryResults to eliminate duplicated
append-only logic.
- Around line 111-125: Update the OnSuccess callback around
applyIPDiscoveryResults so any parse or GetJob error sets
HostConditionIPDiscoveryComplete to False with a failure reason/message and
returns without reporting success, allowing reconciliation to retry. Keep the
existing empty-artifacts behavior unchanged so applyIPDiscoveryResults returning
nil still proceeds through initNetworkAttachmentStatuses and reports Succeeded.
- Around line 197-201: Validate lease.IPAddress with net.ParseIP before
assigning it in the leaseMap lookup within the IP discovery reconciliation flow;
only copy and log the address when parsing succeeds, and leave status.IPAddress
unchanged for empty or malformed values. Add the net import and use this
allow-list validation at the trust boundary.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.go`:
- Around line 258-266: Update the reconciliation loop around reconcileNetworking
so it does not overwrite in-memory status before the assertion: either persist
bmi.Status with k8sClient.Status().Update before each Get, matching the earlier
loops, or remove the Get and assert directly on the in-memory bmi. Preserve the
existing NetworkingJobs and JobID assertions.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking.go`:
- Around line 171-173: Verify the return contract of RunDeprovisioningLifecycle
and its callers before changing this logic. Remove the generic done = true
override for !done, result.IsZero(), and err == nil unless it is explicitly
required; if it handles a provider-specific no-work state, gate it on that
provider state instead, preserving finalizer removal until asynchronous
deprovisioning completes.
- Around line 166-184: Persist the mutated Status.NetworkingJobs immediately
after RunDeprovisioningLifecycle in the deprovisioning flow, using
r.Status().Update with the existing context and bareMetalInstance before
handling completion or finalizer removal. Ensure this status update occurs on
every lifecycle return path after the call, while preserving the existing error
and done handling in the surrounding reconciliation logic.
In `@osac-operator/internal/controller/externalipattachment_controller.go`:
- Around line 315-321: Resolve the target IP in a single mutually exclusive
selection before updating osacExternalIPTargetIPAnnotation: preserve the
ClusterOrder endpoint when both spec.ClusterOrder and spec.BaremetalInstance are
set, and use the BMI primary IP only when no ClusterOrder target exists. Update
needsUpdate and the annotation from that one resolved target, and verify the CRD
validation if available before deciding whether this is only defensive cleanup.
---
Nitpick comments:
In `@bare-metal-fulfillment-operator/cmd/main.go`:
- Around line 414-421: Introduce a named configuration struct for the
dependencies and interval settings used by NewBareMetalInstanceReconciler, with
distinct fields for ProvisioningProvider, NetworkingProvider,
IPDiscoveryProvider, and AAPClient. Update setupBareMetalInstanceController and
the reconciler construction call to pass and consume this config by field name,
preserving existing behavior while preventing same-typed arguments from being
swapped.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup_test.go`:
- Around line 91-93: Update the AfterEach teardown for the BMI to remove
BareMetalInstanceCleanupFinalizer before calling k8sClient.Delete, ensuring
cleanup completes even when the test body assertions fail.
- Around line 46-107: Extend the reconcileAutoCleanup tests to cover
label-selective deletion: create labeled auto-provisioned ExternalIPAttachment
and matching ExternalIP resources, verify the attachment is deleted first and
reconciliation returns done=false, then verify the ExternalIP is deleted after
the attachment is gone. Also verify ExternalIP resources without the
auto-provisioned label or with a different BMI UID remain undeleted, using the
registered CRD types and the BMI UID labels.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery_test.go`:
- Line 130: The test suite lacks coverage for applyIPDiscoveryResults because
AAPClient is always nil. Introduce a small GetJob interface that the AAP client
dependency can satisfy, inject a fake implementation in bare-metal instance IP
discovery tests, and add cases covering artifact retrieval, DHCPLeaseResult
unmarshalling, subnetRef lease-to-attachment matching with IPAddress population,
and parse-failure handling.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery.go`:
- Around line 87-89: Wrap the error returned by the IP discovery call in the
surrounding reconciliation flow before returning it, adding context that
identifies the failure as IP discovery while preserving the original error with
error wrapping. Keep the existing ctrl.Result{} return unchanged.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.go`:
- Around line 196-206: The polling loops discard errors from reconcile and
status-update operations. In
bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.go:196-206,
assert both return values from reconcileNetworking and
k8sClient.Status().Update; apply the same assertions to the failure-path loop at
lines 228-237, preserving that path’s expected reconcile result. In
bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery_test.go:154-162,
assert errors from reconcileIPDiscovery and Status().Update, and make the same
change in the job-tracking loop at lines 188-192; do not ignore any error
returns.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking.go`:
- Around line 69-77: Update the anonymous struct literal passed to
provisioning.ComputeDesiredConfigVersion to use keyed fields for
NetworkAttachments, ExternalHostID, and HostClass, preserving the existing
values while removing reliance on field order.
In `@bare-metal-fulfillment-operator/internal/controller/suite_test.go`:
- Around line 74-79: Make envtest fail fast when the external osac-operator CRD
directory is unavailable by changing the CRD setup around CRDDirectoryPaths and
ErrorIfCRDPathMissing to enable missing-path errors, or explicitly validate that
directory before envtest starts. Preserve the existing CRD paths while ensuring
missing ExternalIP and ExternalIPAttachment definitions produce a clear setup
failure.
🪄 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: e10be740-b793-4938-a94c-83aabaf705da
📒 Files selected for processing (18)
bare-metal-fulfillment-operator/api/v1alpha1/baremetalinstance_types.gobare-metal-fulfillment-operator/api/v1alpha1/zz_generated.deepcopy.gobare-metal-fulfillment-operator/charts/operator-crds/templates/osac.openshift.io_baremetalinstances.yamlbare-metal-fulfillment-operator/cmd/main.gobare-metal-fulfillment-operator/config/crd/bases/osac.openshift.io_baremetalinstances.yamlbare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup_test.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_controller.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_controller_test.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery_test.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_metal3_integration_test.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_names.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.gobare-metal-fulfillment-operator/internal/controller/suite_test.goosac-operator/internal/controller/externalipattachment_controller.goosac-operator/pkg/aap/client.go
Comment on lines
+171
to
+173
| if !done && result.IsZero() && err == nil { | ||
| done = true | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Justify or remove the done override.
This block forces done = true whenever the lifecycle returns false, a zero result, and no error. That triple is also the natural return shape for "job dispatched, nothing to requeue yet" in several lifecycle implementations. If that is the case here, the finalizer is removed at Line 181 while the AAP deprovision job still runs, and the network attachments are orphaned in the fabric.
Confirm the exact return contract before merging. If the override exists to work around a provider that reports no work, gate it on that provider state instead of on a generic zero-value check.
#!/bin/bash
# Description: Determine the (result, done, error) return contract of RunDeprovisioningLifecycle.
set -euo pipefail
rg -nP -A 60 'func RunDeprovisioningLifecycle' --type=go
# Compare with other callers to see whether any of them apply the same override.
rg -nP -B 3 -A 12 'RunDeprovisioningLifecycle\(' --type=go🤖 Prompt for 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.
In
`@bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking.go`
around lines 171 - 173, Verify the return contract of RunDeprovisioningLifecycle
and its callers before changing this logic. Remove the generic done = true
override for !done, result.IsZero(), and err == nil unless it is explicitly
required; if it handles a provider-specific no-work state, gate it on that
provider state instead, preserving finalizer removal until asynchronous
deprovisioning completes.
Comment on lines
+315
to
+321
| if bmi != nil { | ||
| targetIP := bmi.PrimaryIPAddress() | ||
| if targetIP != "" && attachment.Annotations[osacExternalIPTargetIPAnnotation] != targetIP { | ||
| attachment.Annotations[osacExternalIPTargetIPAnnotation] = targetIP | ||
| needsUpdate = true | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The BareMetalInstance branch overwrites the ClusterOrder target IP.
Line 317 writes osacExternalIPTargetIPAnnotation. Line 311 writes the same annotation key from the ClusterOrder endpoint. The bmi branch runs second, so it wins.
If an ExternalIPAttachment sets both spec.ClusterOrder and spec.BaremetalInstance, two failures follow:
- The DNAT target points at the BMI primary IP instead of the cluster endpoint.
- The two branches fight each other across reconciles. Pass N writes the BMI IP,
needsUpdateis true,handleUpdatecallsUpdateand requeues after one second at Line 245. Pass N+1 sees the BMI IP, thecobranch rewrites the cluster endpoint,needsUpdateis true again. The attachment never reacheshandleProvisioning, and the controller issues one write per second per attachment indefinitely.
Resolve one target explicitly instead of letting the last branch win.
🐛 Proposed direction
- if co != nil {
- targetIP := r.resolveClusterEndpoint(co, attachment)
- if targetIP != "" && attachment.Annotations[osacExternalIPTargetIPAnnotation] != targetIP {
- attachment.Annotations[osacExternalIPTargetIPAnnotation] = targetIP
- needsUpdate = true
- }
- }
- if bmi != nil {
- targetIP := bmi.PrimaryIPAddress()
- if targetIP != "" && attachment.Annotations[osacExternalIPTargetIPAnnotation] != targetIP {
- attachment.Annotations[osacExternalIPTargetIPAnnotation] = targetIP
- needsUpdate = true
- }
- }
+ // Exactly one target owns the target-IP annotation.
+ var targetIP string
+ switch {
+ case bmi != nil:
+ targetIP = bmi.PrimaryIPAddress()
+ case co != nil:
+ targetIP = r.resolveClusterEndpoint(co, attachment)
+ }
+ if targetIP != "" && attachment.Annotations[osacExternalIPTargetIPAnnotation] != targetIP {
+ attachment.Annotations[osacExternalIPTargetIPAnnotation] = targetIP
+ needsUpdate = true
+ }Confirm whether the CRD already forbids setting both fields. If it does, downgrade this to a defensive cleanup.
#!/bin/bash
# Description: Check whether the ExternalIPAttachment CRD enforces mutual exclusivity between target fields.
set -euo pipefail
fd -t f 'externalipattachment' --iglob '*.yaml' --exec rg -n -C 6 'x-kubernetes-validations|oneOf|baremetalInstance|clusterOrder|computeInstance' {}
rg -nP -B 8 -A 30 'type ExternalIPAttachmentSpec struct' --type=go🤖 Prompt for 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.
In `@osac-operator/internal/controller/externalipattachment_controller.go` around
lines 315 - 321, Resolve the target IP in a single mutually exclusive selection
before updating osacExternalIPTargetIPAnnotation: preserve the ClusterOrder
endpoint when both spec.ClusterOrder and spec.BaremetalInstance are set, and use
the BMI primary IP only when no ClusterOrder target exists. Update needsUpdate
and the annotation from that one resolved target, and verify the CRD validation
if available before deciding whether this is only defensive cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
osac-operator/internal/controller/securitygroup_controller_test.go (1)
751-767: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the error identifies the unregistered manager.
Expect(err).To(HaveOccurred())passes for any failure, including an unrelated List or Update error. That makes the test insensitive to a regression where dispatcher resolution stops running. Match the message.♻️ Proposed assertion tightening
_, err = reconciler.Reconcile(ctx, mcreconcile.Request{Request: ctrl.Request{NamespacedName: key}}) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("does-not-exist")))🤖 Prompt for 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. In `@osac-operator/internal/controller/securitygroup_controller_test.go` around lines 751 - 767, Strengthen the assertion in the unregistered-manager test by verifying that the reconcile error message identifies “does-not-exist” (or the established dispatcher wording for the missing manager), rather than only checking that an error occurred. Keep the existing reconcile setup and error expectation unchanged.
🤖 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 `@osac-operator/pkg/dispatcher/resolver.go`:
- Around line 98-100: Update Resolve in
osac-operator/pkg/dispatcher/resolver.go:98-100 to return the neither-manager
sentinel only when both FabricManager and K8sManager are empty, while preserving
successful k8s-only resolution; update
osac-operator/pkg/dispatcher/resolver_test.go:170-188 to separately test the
sentinel and k8s-only success. In
osac-operator/pkg/dispatcher/dispatch_test.go:158-167, use k8sName rather than
&k8sName at lines 279, 299, 322, and 341, and "" rather than nil at line 357
while retaining k8s-only fallback assertions. In
osac-operator/internal/controller/networkclass_capabilities_controller.go:183-188,
match the neither-manager sentinel so resolved k8s-only classes reach the
existing fabric-nil capability skip.
---
Nitpick comments:
In `@osac-operator/internal/controller/securitygroup_controller_test.go`:
- Around line 751-767: Strengthen the assertion in the unregistered-manager test
by verifying that the reconcile error message identifies “does-not-exist” (or
the established dispatcher wording for the missing manager), rather than only
checking that an error occurred. Keep the existing reconcile setup and error
expectation unchanged.
🪄 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: c8c63382-85a7-474b-9e1b-5b7f085148fc
📒 Files selected for processing (31)
bare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_controller.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking.gobare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.goosac-aap/collections/ansible_collections/osac/templates/roles/cudn_net/README.mdosac-aap/collections/ansible_collections/osac/templates/roles/cudn_net/meta/argument_specs.yamlosac-aap/collections/ansible_collections/osac/templates/roles/cudn_net/meta/osac.yamlosac-aap/collections/ansible_collections/osac/templates/roles/cudn_net/tasks/create_security_group.yamlosac-aap/collections/ansible_collections/osac/templates/roles/cudn_net/tasks/delete_security_group.yamlosac-operator/AGENTS.mdosac-operator/charts/operator/values.yamlosac-operator/cmd/main.goosac-operator/internal/controller/constants_common.goosac-operator/internal/controller/dispatcher_helpers.goosac-operator/internal/controller/networkclass_capabilities_controller.goosac-operator/internal/controller/securitygroup_controller.goosac-operator/internal/controller/securitygroup_controller_test.goosac-operator/internal/controller/subnet_controller.goosac-operator/internal/controller/subnet_controller_test.goosac-operator/internal/controller/virtualnetwork_controller.goosac-operator/internal/controller/virtualnetwork_controller_test.goosac-operator/internal/dispatcheradapter/dispatcheradapter_suite_test.goosac-operator/internal/dispatcheradapter/network_class_adapter.goosac-operator/internal/dispatcheradapter/network_class_adapter_test.goosac-operator/pkg/aap/client.goosac-operator/pkg/dispatcher/dispatch_test.goosac-operator/pkg/dispatcher/doc.goosac-operator/pkg/dispatcher/resolver.goosac-operator/pkg/dispatcher/resolver_cache_test.goosac-operator/pkg/dispatcher/resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking_test.go
- osac-operator/pkg/aap/client.go
- bare-metal-fulfillment-operator/internal/controller/baremetalinstance_ip_discovery.go
- bare-metal-fulfillment-operator/internal/controller/baremetalinstance_networking.go
- bare-metal-fulfillment-operator/internal/controller/baremetalinstance_cleanup.go
- bare-metal-fulfillment-operator/internal/controller/baremetalinstance_controller.go
Comment on lines
+98
to
+100
| if managers.FabricManager == "" { | ||
| return nil, fmt.Errorf("NetworkClass %q: %w", networkClassID, ErrFabricManagerNotSet) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the k8s-only NetworkClass contract.
Resolve rejects every NetworkClass without FabricManager, including a NetworkClass with a valid K8sManager. This disables the legacy k8s-only fallback described by the review cohort. It also makes the later resolved.FabricManager == nil path unreachable.
osac-operator/pkg/dispatcher/resolver.go#L98-L100: Return a neither-manager sentinel only when both manager names are empty. Resolve and return a k8s-only result whenK8sManageris set.osac-operator/pkg/dispatcher/resolver_test.go#L170-L188: Test the neither-manager sentinel separately. Restore coverage for successful k8s-only resolution.osac-operator/pkg/dispatcher/dispatch_test.go#L158-L167: Passk8sNameinstead of&k8sNameat Lines 279, 299, 322, and 341. Pass""instead ofnilat Line 357. Keep the k8s-only fallback assertions.osac-operator/internal/controller/networkclass_capabilities_controller.go#L183-L188: Match the neither-manager sentinel here. Let resolved k8s-only NetworkClasses reach the existing fabric-nil capability skip.
📍 Affects 4 files
osac-operator/pkg/dispatcher/resolver.go#L98-L100(this comment)osac-operator/pkg/dispatcher/resolver_test.go#L170-L188osac-operator/pkg/dispatcher/dispatch_test.go#L158-L167osac-operator/internal/controller/networkclass_capabilities_controller.go#L183-L188
🤖 Prompt for 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.
In `@osac-operator/pkg/dispatcher/resolver.go` around lines 98 - 100, Update
Resolve in osac-operator/pkg/dispatcher/resolver.go:98-100 to return the
neither-manager sentinel only when both FabricManager and K8sManager are empty,
while preserving successful k8s-only resolution; update
osac-operator/pkg/dispatcher/resolver_test.go:170-188 to separately test the
sentinel and k8s-only success. In
osac-operator/pkg/dispatcher/dispatch_test.go:158-167, use k8sName rather than
&k8sName at lines 279, 299, 322, and 341, and "" rather than nil at line 357
while retaining k8s-only fallback assertions. In
osac-operator/internal/controller/networkclass_capabilities_controller.go:183-188,
match the neither-manager sentinel so resolved k8s-only classes reach the
existing fabric-nil capability skip.
This was referenced Aug 11, 2026
Merged
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 4:46 PM UTC · Completed 5:03 PM UTC Commit: |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…o BMI CRD Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Add NetworkingProvider field to BareMetalInstanceReconciler and wire it in main.go using NewAAPProvider with explicit template names for create/delete network attachment. Add RBAC markers for Subnet and NetworkClass CRs. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Add reconcileNetworking and reconcileNetworkingDeletion methods to the BMI controller. The networking phase runs between inventory allocation and OS provisioning, dispatching create_network_attachment AAP jobs via the networking provider. All attachments must reach Ready before provisioning proceeds. On deletion, network attachments are deprovisioned before management cleanup. A dedicated finalizer (baremetalinstance-networking) ensures cleanup runs even on unexpected deletion. Includes Ginkgo tests covering skip paths (no attachments, nil provider), provisioning lifecycle (success, failure, in-progress), finalizer lifecycle, and deletion ordering. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Add reconcileIPDiscovery to the BMI controller. After OS provisioning completes, this phase dispatches query_dhcp_lease AAP jobs to discover DHCP-assigned IPs for each network attachment. Results are parsed from AAP job artifacts and written to status.networkAttachmentStatuses. Adds IPDiscoveryJobs and IPDiscoveryComplete condition to the CRD. Adds Artifacts field to aap.Job for reading job results. Extracts reconcileNetworkProvisionAndDiscovery to reduce cyclomatic complexity of reconcileManagement. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
…etion On BareMetalInstance deletion, the cleanup finalizer deletes auto- provisioned ExternalIPAttachment resources first, then ExternalIP resources. Resources are identified by osac.openshift.io/auto-provisioned and baremetalinstance-uuid labels. Manually created resources are not affected. Phased requeue prevents finalizer leaks. Registers osac-operator CRDs in envtest suite and osac-operator scheme in cmd/main.go so the BMF operator can list/delete ExternalIP resources. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
When the ExternalIPAttachment target is a BareMetalInstance, the controller now reads the primary IP address from the BMI's networkAttachmentStatuses and sets the target-ip annotation for DNAT provisioning. The controller requeues if the BMI's primary IP has not yet been discovered by the IP discovery phase. Adds PrimaryIPAddress() helper to the BareMetalInstance type, which returns the IP of the primary attachment (explicit or implicit for single-attachment instances). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
- Add addCleanupFinalizerIfNeeded to reconcileManagement so the cleanup finalizer is added when auto-provisioned ExternalIP resources exist - Change aap.Job.Artifacts from string to json.RawMessage to match the AAP REST API response format - Update IP discovery to use json.RawMessage for artifact parsing Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
- Scope cleanup list calls to BMI namespace - Move IP discovery outside provisioning branch so noop-template BMIs still discover DHCP leases - Fail IP discovery condition when artifact parsing fails instead of reporting success - Validate lease IP with net.ParseIP before writing to status - Refresh existing network attachment statuses from spec (fixes stale Primary flag) - Persist NetworkingJobs status after RunDeprovisioningLifecycle - Fix test loop to persist status before re-reading Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Use auto-created/auto-created-for labels (matching the fulfillment- service's ComputeInstance auto-provision pattern) instead of auto-provisioned/baremetalinstance-uuid. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Replace typed opv1alpha1.ExternalIPList/ExternalIPAttachmentList with unstructured.UnstructuredList in the cleanup code. Remove opv1alpha1.AddToScheme from cmd/main.go. The previous approach registered all osac-operator CRD types in the BMF operator's scheme, which caused controller-runtime's informer cache to fail when ExternalIP CRDs weren't installed — blocking all BMI reconciliation and causing BMIs to get stuck in Provisioning state. Unstructured lists bypass the cache and don't require the CRD to be registered in the scheme. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
- Add ExternalIP/ExternalIPAttachment RBAC markers to BMF controller and regenerate Helm chart ClusterRole - Create playbooks for create/delete network attachment and query DHCP lease operations - Register the 3 job templates in AAP config-as-code (controller.yml) using the networking-operations inventory and instance group Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Add a networkManagers values array that creates ConfigMaps with the osac.openshift.io/network-fabric-manager or network-k8s-manager labels. The osac-operator dispatcher discovers these to resolve which fabric/k8s manager handles each NetworkClass. Add netris fabric manager to bmaas-ci values. Includes schema validation for the new values. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Use implementation_strategy from CR annotation (matching the subnet/ virtualnetwork pattern) instead of hardcoding osac.templates.netris. Playbooks now work for any resource with spec.networkAttachments, not just BareMetalInstance. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Change /api/v2/hw/ports to /api/v2/ports in create_network_attachment and delete_network_attachment. The hw/ports endpoint omits ports that are part of a bond or have active links (e.g. NS bond ports), returning only a subset. The /api/v2/ports endpoint returns all ports. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
The role now accepts network_attachment (host_name, logical_interface_name, subnet_ref) matching the create/delete roles, and resolves the port MAC via Netris API internally. Previously it expected dhcp_lease_query with a pre-resolved port_mac that the playbook didn't provide. Uses /api/v2/ports (not /api/v2/hw/ports) for the port lookup, consistent with the port endpoint fix in the other roles. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Prevents HTTP 304 Not Modified responses when Ansible's uri module reuses conditional request headers from prior calls to the same endpoint. Applied to create, delete, and query_dhcp_lease roles. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
The V-Net gateway was created without DHCP, so bare metal hosts attached to the V-Net couldn't obtain IP addresses. Now the create_subnet role computes a DHCP range from the subnet CIDR (second usable IP to last usable IP, excluding the gateway) and passes dhcpEnabled/dhcpStartIp/dhcpEndIp to the vnet create role. Also adds vnet_dhcp_enabled, vnet_dhcp_start_ip, vnet_dhcp_end_ip parameters to the netris.controller.vnet role's argument spec. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Block-level tasks (inside when: blocks) had force: true at the wrong indentation level (column 5 instead of column 9). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Replace the port MAC lookup approach with direct IPAM host name matching. The Netris IPAM host name field contains the server name, so within a specific subnet the match is unambiguous. This avoids the /api/v2/ports endpoint which doesn't expose MAC addresses on port objects. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 6:54 AM UTC · Completed 7:11 AM UTC Commit: |
| ) (ctrl.Result, bool, error) { | ||
| log := logf.FromContext(ctx) | ||
|
|
||
| if !controllerutil.ContainsFinalizer(bareMetalInstance, BareMetalInstanceCleanupFinalizer) { |
There was a problem hiding this comment.
[medium] error-handling-gap
When listUnstructured fails for ExternalIPAttachments or ExternalIPs, the cleanup finalizer is silently removed and the function returns done=true. Transient API errors cause auto-provisioned ExternalIP resources to be orphaned. The code does not distinguish between CRD-not-installed and transient API errors.
Suggested fix: Check the error type: for meta.IsNoMatchError (CRD not registered), removing the finalizer is appropriate. For other errors, return the error to retry.
| - get | ||
| - patch | ||
| - update | ||
| - apiGroups: |
There was a problem hiding this comment.
[medium] permission-expansion
New RBAC rule grants delete permission on externalipattachments and externalips resources cluster-wide. The scope is narrowed by label selectors in controller code, but RBAC itself is broad.
Suggested fix: Confirm linked JIRA stories authorize this RBAC expansion. Consider namespace-scoped Roles for least privilege.
| bareMetalInstance.SetStatusCondition( | ||
| v1alpha1.HostConditionIPDiscoveryComplete, | ||
| metav1.ConditionFalse, | ||
| v1alpha1.HostConditionReasonTemplateFailed, |
There was a problem hiding this comment.
[low] error-handling-gap
In OnSuccess callback, when applyIPDiscoveryResults fails, the condition is set to TemplateFailed but no error is returned to the lifecycle. IP discovery shows a failed condition that persists without a clear retry/recovery path for artifact parse failures.
Suggested fix: Consider returning an error from the callback or adding a mechanism to re-trigger IP discovery when artifacts cannot be parsed.
| @@ -101,6 +111,9 @@ | |||
| vnet_site_id: "{{ netris_site_id }}" | |||
There was a problem hiding this comment.
[low] scope-creep
vnet_dhcp_enabled is set to true unconditionally for all new Netris subnets. Previously subnets did not enable DHCP. This behavioral change affects all subnet creation, not just bare-metal networking.
| TryLockFailPollIntervalDuration time.Duration | ||
| ManagementRecheckIntervalDuration time.Duration | ||
| ProvisionPollIntervalDuration time.Duration | ||
| } |
There was a problem hiding this comment.
[low] pattern-violation
RBAC markers for subnets and networkclasses appear twice: above the struct definition and above the Reconcile method. The codebase convention is a single consolidated block.
| ) | ||
| // Persist NetworkingJobs changes made by RunDeprovisioningLifecycle. | ||
| // The CRD has a status subresource, so r.Update does not write status fields. | ||
| if statusErr := r.Status().Update(ctx, bareMetalInstance); statusErr != nil { |
There was a problem hiding this comment.
[low] pattern-inconsistency
reconcileNetworkingDeletion has an explicit r.Status().Update inside the method, differing from the established pattern where the main Reconcile loop performs a single status update at the end.
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
Implements BMaaS networking for epic OSAC-1448 (BareMetalInstance Network Attachments):
reconcileNetworkingphase in BMI controller — dispatchescreate_network_attachment/delete_network_attachmentAAP jobs before OS provisioningreconcileIPDiscoveryphase — dispatchesquery_dhcp_leaseafter provisioning to discover DHCP-assigned IPsChanges by component
bare-metal-fulfillment-operator:
NetworkingJobs,IPDiscoveryJobsin BMI statusNetworkAttachmentsReady,IPDiscoveryCompletebaremetalinstance_networking.go,baremetalinstance_ip_discovery.go,baremetalinstance_cleanup.go(+ tests)NetworkingProviderandIPDiscoveryProviderwired viaNewAAPProviderwith explicit template namesosac-operator:
aap.Job.Artifactsfield (json.RawMessage) for reading AAP job resultsPrimaryIPAddress(), precondition requeue if IP not yet discoveredPrimaryIPAddress()helper on BareMetalInstance typeosac-aap:
playbook_osac_create_network_attachment.yml,playbook_osac_delete_network_attachment.yml,playbook_osac_query_dhcp_lease.ymlcontroller.ymlosac-installer:
networkManagersvalues array for creating fabric/k8s manager ConfigMapsnetwork-managers.yamlbmaas-ci/values.yamlTest plan
🤖 Generated with Claude Code