Replace internal/ironic with backend-agnostic internal/management interface#9
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (9)
WalkthroughMigrates power management to a backend-agnostic management.Client, implements an OpenStack backend, updates HostLeaseReconciler to use management.PowerStatus semantics, rewrites reconcile/sync logic and tests, and wires the new client into cmd/main. ChangesManagement client abstraction and controller migration
Sequence DiagramsequenceDiagram
participant Main as cmd/main
participant MgmtFactory as management.NewOpenStackClient
participant OpenStackClient
participant IronicAPI
participant Reconciler as HostLeaseReconciler
Main->>MgmtFactory: NewOpenStackClient(ctx, Config{Type:"openstack"})
MgmtFactory->>OpenStackClient: construct OpenStackClient
OpenStackClient->>IronicAPI: authenticate / build nodes client
OpenStackClient-->>Main: return client or error
Main->>Reconciler: NewHostLeaseReconciler(managementClient)
Reconciler->>OpenStackClient: GetPowerState(ctx, externalHostID)
OpenStackClient->>IronicAPI: fetch node
IronicAPI-->>OpenStackClient: node state + target
OpenStackClient-->>Reconciler: PowerStatus{State, IsTransitioning}
Reconciler->>OpenStackClient: SetPowerState(ctx, hostID, target)
OpenStackClient->>IronicAPI: change node power
IronicAPI-->>OpenStackClient: success or 409 conflict
OpenStackClient-->>Reconciler: nil or ErrTransitioning / wrapped error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Security & Risk Assessment
Poem
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🧪 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: 6
🤖 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 `@cmd/main.go`:
- Around line 235-237: Replace the unbounded startup context used when creating
the OpenStack client: instead of calling management.NewOpenStackClient with
context.TODO(), create a context with a reasonable timeout (e.g., using
context.WithTimeout) and pass that context into management.NewOpenStackClient so
the underlying clientconfig.AuthenticatedClient call cannot block startup
indefinitely; ensure you cancel the context after use and add the time import in
cmd/main.go for the timeout value (referencing management.NewOpenStackClient and
the current use of context.TODO()).
In `@internal/controller/hostlease_controller.go`:
- Around line 291-313: The controller should not treat backend "transition in
progress" errors as terminal: update internal/management/openstack.go (the
OpenStackClient.SetPowerState path) to detect API conflict/409 or
transition-specific error text and return a normalized sentinel
(management.ErrTransitioning); then update reconcilePower in
hostlease_controller.go to handle management.ErrTransitioning the same way it
handles powerStatus.IsTransitioning (log that the node is transitioning and
return without marking the HostLease failed so the controller can
requeue/progress instead of setting HostLeasePhaseFailed). Ensure you reference
the management.ErrTransitioning sentinel in both places so the behavior is
consistent.
In `@internal/management/client.go`:
- Around line 35-41: The NewClient function currently returns nil, nil when no
factory exists for cfg.Type which silently swallows misconfiguration; update
NewClient (and its lookup of newClientFuncs) to return a descriptive error
instead of nil when ok is false (e.g., fmt.Errorf("unknown client type %q",
cfg.Type) or similar), so callers receive a non-nil error on unregistered
cfg.Type rather than a nil Client.
- Around line 10-13: The constants PowerOn and PowerOff in PowerState are
hardcoded string literals; replace their values with the canonical gophercloud
constants by assigning them using string(nodes.PowerOn) and
string(nodes.PowerOff) (referencing the existing PowerOn, PowerOff and
PowerState symbols and the external nodes.PowerOn/nodes.PowerOff constants) so
the code uses gophercloud’s authoritative values and avoids future drift.
In `@internal/management/management_test.go`:
- Line 71: Define a test constant (e.g. const testProviderOpenstack =
"openstack") in management_test.go and replace the three hard-coded occurrences
of "openstack" where a resource's Type is set (the struct initializers using
Type: "openstack") with that constant; ensure all three call sites in the tests
now reference testProviderOpenstack to satisfy goconst and keep semantics
unchanged.
In `@internal/management/openstack.go`:
- Around line 62-72: GetPowerState currently casts Ironic's node.PowerState
directly into the local PowerState allowing unexpected values through; change
GetPowerState to validate node.PowerState with an allow-list using the canonical
strings string(nodes.PowerOn) and string(nodes.PowerOff) (map those to your
local PowerState constants) and return a clear error for any unknown value, keep
IsTransitioning as node.TargetPowerState != "" and also verify the PowerState
constants in client.go match the canonical gophercloud values before returning
the PowerStatus.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 05116e8c-18ef-4064-b6c2-554606281295
📒 Files selected for processing (8)
cmd/main.gointernal/controller/hostlease_controller.gointernal/controller/hostlease_controller_test.gointernal/ironic/client.gointernal/ironic/client_test.gointernal/management/client.gointernal/management/management_test.gointernal/management/openstack.go
💤 Files with no reviewable changes (2)
- internal/ironic/client.go
- internal/ironic/client_test.go
Comment on lines
+10
to
+13
| const ( | ||
| PowerOn PowerState = "power on" | ||
| PowerOff PowerState = "power off" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Extract PowerOn and PowerOff constant values from gophercloud nodes package.
# Search for the PowerState constant declarations in gophercloud
rg -nP --type=go 'const\s+Power(On|Off)\s+\w+\s*=\s*"[^"]+"' -A1 -B1Repository: osac-project/host-management-openstack
Length of output: 64
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show PowerState type + constant definitions and nearby usage
FILE="internal/management/client.go"
nl -ba "$FILE" | sed -n '1,120p'
# Find all uses of PowerOn/PowerOff/PowerState in the repo
rg -n --hidden --glob '!*vendor/*' 'PowerOn|PowerOff|PowerState' .Repository: osac-project/host-management-openstack
Length of output: 125
🌐 Web query:
gophercloud nodes.PowerOn PowerOff constant value "power on" "power off"
💡 Result:
In gophercloud’s Bare Metal nodes API, the power action constants map to these exact string values: - nodes.PowerOn = "power on" [1] - nodes.PowerOff = "power off" [1] The OpenStack Bare Metal API documentation likewise describes power actions/states as usually the strings “power on” or “power off” [2], and the corresponding enum in the OpenStack SDK uses POWER_ON = 'power on' and POWER_OFF = 'power off' [3].
Citations:
- 1: https://pkg.go.dev/github.com/gophercloud/gophercloud/v2/openstack/baremetal/v1/nodes
- 2: https://docs.openstack.org/openstacksdk/2023.2/user/resources/baremetal/v1/node.html
- 3: https://github.com/openstack/openstacksdk/blob/master/openstack/baremetal/v1/node.py
Use gophercloud nodes.PowerOn/nodes.PowerOff (values already match)
internal/management/client.go hardcodes PowerOn = "power on" and PowerOff = "power off", which match gophercloud’s nodes.PowerOn/nodes.PowerOff string values, so power-transition mismatch risk is low. For maintainability and to prevent future drift, switch to string(nodes.PowerOn) / string(nodes.PowerOff) instead of raw string literals.
🤖 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 `@internal/management/client.go` around lines 10 - 13, The constants PowerOn
and PowerOff in PowerState are hardcoded string literals; replace their values
with the canonical gophercloud constants by assigning them using
string(nodes.PowerOn) and string(nodes.PowerOff) (referencing the existing
PowerOn, PowerOff and PowerState symbols and the external
nodes.PowerOn/nodes.PowerOff constants) so the code uses gophercloud’s
authoritative values and avoids future drift.
DanNiESh
force-pushed
the
host-management-interface
branch
from
0a07073 to
a748b8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/hostlease_controller.go (1)
160-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor: don't mark a transitioning node as
Ready/PowerSynced=True.
reconcilePowercorrectly recognizespowerStatus.IsTransitioning, but the final convergence check andsyncHostLeaseStatusignore that flag and look only atState. If the backend reports a transition in progress whileStatealready equals the desired target, this path setsHostConditionPowerSynced=True, advances the lease toReady, and stops requeueing before the power operation actually finishes. Risk: major correctness/availability impact from prematurely declaring host power convergence.Suggested fix
- r.syncHostLeaseStatus(hostLease, powerStatus, nil, log) + r.syncHostLeaseStatus(hostLease, powerStatus, nil, log) if hostLease.Spec.PoweredOn != nil { - if *hostLease.Spec.PoweredOn != (powerStatus.State == management.PowerOn) { + if powerStatus.IsTransitioning || *hostLease.Spec.PoweredOn != (powerStatus.State == management.PowerOn) { return ctrl.Result{RequeueAfter: r.RecheckInterval}, nil } } @@ if powerStatus == nil { return } poweredOn := powerStatus.State == management.PowerOn hostLease.Status.PoweredOn = &poweredOn + + if powerStatus.IsTransitioning { + hostLease.Status.Phase = v1alpha1.HostLeasePhaseProgressing + hostLease.SetStatusCondition( + v1alpha1.HostConditionPowerSynced, + metav1.ConditionFalse, + v1alpha1.HostConditionReasonProgressing, + "waiting for node power transition to complete", + ) + return + } if hostLease.Spec.PoweredOn != nil && *hostLease.Spec.PoweredOn != poweredOn {Based on learnings:
reconcilePowershould use the external host identifier field consistently; this path already does that, so the remaining contract to preserve is themanagement.PowerStatustransition semantics.Also applies to: 380-415
🤖 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 `@internal/controller/hostlease_controller.go` around lines 160 - 167, The code marks HostLease as Ready and sets PowerSynced without honoring management.PowerStatus.IsTransitioning; update the convergence check in the block that sets hostLease.Status.Phase (and the similar logic at lines ~380-415) to treat any powerStatus.IsTransitioning as "not yet converged": if powerStatus.IsTransitioning is true, return ctrl.Result{RequeueAfter: r.RecheckInterval}, nil (do not set HostLeasePhaseReady or PowerSynced=True) and ensure syncHostLeaseStatus (and any code referencing HostConditionPowerSynced) uses management.PowerStatus.IsTransitioning when computing convergence so the controller continues requeuing until the transition completes.
🤖 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 `@internal/controller/hostlease_controller.go`:
- Around line 136-142: The code dereferences powerStatus.State without checking
for a nil powerStatus returned by r.ManagementClient.GetPowerState; update the
reconcile logic around the GetPowerState call so you first check if powerStatus
== nil (treating nil as a valid “no data” result) before accessing
powerStatus.State, call r.syncHostLeaseStatus(hostLease, nil, nil, log) or the
existing nil-path handling as appropriate, and only log or compare
powerStatus.State when powerStatus is non-nil; apply the same nil-guard fix to
both usages of GetPowerState / powerStatus.State (the block calling
r.syncHostLeaseStatus and the later block at the second occurrence).
In `@internal/management/client.go`:
- Around line 39-44: NewClient currently dereferences cfg.Type without
validating cfg and will panic if cfg is nil; update NewClient to first check
whether cfg is nil and return a descriptive error (e.g., "nil config" or
"invalid configuration") instead of proceeding, then validate cfg.Type (ensure
it's non-empty) before looking up newClientFuncs[cfg.Type] and returning the
chosen fn(ctx, cfg) or the unsupported-backend error; reference the NewClient
function and the newClientFuncs map when making this change.
In `@internal/management/management_test.go`:
- Around line 65-79: The test is flaky because hasOpenStackCredentials() returns
true if either OS_CLOUD or OS_AUTH_URL is set, causing partial envs to run real
integration logic; change the test to only run the real OpenStack client
creation when a strict integration flag or full set of required env vars is
present (e.g., require TEST_OPENSTACK_INTEGRATION=true or require both OS_CLOUD
and OS_AUTH_URL and any other mandatory vars), or alternatively replace the real
call to management.NewClient in these specs with a mocked client; update the
BeforeEach that calls hasOpenStackCredentials() (and any duplicate checks around
lines 111-136) to use the stricter predicate or flag so the suite remains
deterministic.
In `@internal/management/openstack.go`:
- Around line 42-57: The NewBareMetalV1 call is ignoring the region/endpoint
type from clientOpts; update the call to pass gophercloud.EndpointOpts populated
from cloud.RegionName and cloud.EndpointType so Ironic endpoint discovery uses
the intended region/interface. Replace openstack.NewBareMetalV1(providerClient,
gophercloud.EndpointOpts{}) with openstack.NewBareMetalV1(providerClient,
gophercloud.EndpointOpts{Region: cloud.RegionName, Availability:
cloud.EndpointType}) (referencing clientOpts, cloud.RegionName,
cloud.EndpointType, and openstack.NewBareMetalV1).
---
Outside diff comments:
In `@internal/controller/hostlease_controller.go`:
- Around line 160-167: The code marks HostLease as Ready and sets PowerSynced
without honoring management.PowerStatus.IsTransitioning; update the convergence
check in the block that sets hostLease.Status.Phase (and the similar logic at
lines ~380-415) to treat any powerStatus.IsTransitioning as "not yet converged":
if powerStatus.IsTransitioning is true, return ctrl.Result{RequeueAfter:
r.RecheckInterval}, nil (do not set HostLeasePhaseReady or PowerSynced=True) and
ensure syncHostLeaseStatus (and any code referencing HostConditionPowerSynced)
uses management.PowerStatus.IsTransitioning when computing convergence so the
controller continues requeuing until the transition completes.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ca0a030d-4a71-47b9-bf8d-b4a4143b60c9
📒 Files selected for processing (8)
cmd/main.gointernal/controller/hostlease_controller.gointernal/controller/hostlease_controller_test.gointernal/ironic/client.gointernal/ironic/client_test.gointernal/management/client.gointernal/management/management_test.gointernal/management/openstack.go
💤 Files with no reviewable changes (2)
- internal/ironic/client_test.go
- internal/ironic/client.go
DanNiESh
force-pushed
the
host-management-interface
branch
2 times, most recently
from
351a480 to
4adf46b
Compare
|
/retest |
|
Test failures seem unrelated (possibly a bug with files that are deleted in a PR); forcing merge |
|
@coderabbitai have all issues been addressed? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch all review comments on PR `#9` (including resolved state)
gh api repos/osac-project/host-management-openstack/pulls/9/comments \
--jq '.[] | {id, user: .user.login, body: .body, path: .path, line: .line, resolved: (.resolved // false)}' 2>/dev/null | head -200
echo "--- REVIEWS ---"
gh api repos/osac-project/host-management-openstack/pulls/9/reviews \
--jq '.[] | {id, user: .user.login, state, body: .body}' 2>/dev/null | head -100
echo "--- ISSUE COMMENTS ---"
gh api repos/osac-project/host-management-openstack/issues/9/comments \
--jq '.[] | {id, user: .user.login, body: .body}' 2>/dev/null | head -200Length of output: 107274
✅ Fully addressed (confirmed)
❌ Still outstanding1. Convergence check ignores This was posted as an "outside diff range" comment in the second review and has no confirmation of being addressed. If
Suggested check: // In the convergence block
if powerStatus.IsTransitioning || *hostLease.Spec.PoweredOn != (powerStatus.State == management.PowerOn) {
return ctrl.Result{RequeueAfter: r.RecheckInterval}, nil
}
// In syncHostLeaseStatus, before setting PowerSynced=True
if powerStatus.IsTransitioning {
// set Progressing condition, don't advance to Ready
return
}2. Pre-merge check failure — ❌ Error (sensitive data in logs)
Warnings (non-blocking but flagged by CI):
🧠 Learnings used |
DanNiESh
force-pushed
the
host-management-interface
branch
from
4adf46b to
07d2684
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmd/main.go`:
- Around line 241-243: The current call to setupLog.Error(err, "failed to create
management client") may log sensitive OpenStack credentials/tokens returned by
management.NewOpenStackClient; change this to log a generic failure message
(e.g., setupLog.Error(nil, "failed to create management client")) or build and
pass a sanitized error string instead of the raw err; implement a small
sanitizer that strips Keystone URLs, auth tokens and known sensitive keys from
err.Error() (or only log err.Error() at debug/trace level), and use that
sanitized string in the setupLog.Error call so sensitive data is never emitted
to normal logs.
In `@internal/controller/hostlease_controller.go`:
- Around line 422-424: syncHostLeaseStatus currently returns early when
powerStatus.IsTransitioning and skips updating the PowerSynced condition; change
this to explicitly set the PowerSynced condition to False with
Reason="Progressing" (and an appropriate Message) before returning so the status
reflects an ongoing transition. Locate syncHostLeaseStatus and the block that
checks powerStatus.IsTransitioning and replace the early return with code that
updates the HostLease status conditions (PowerSynced condition) to
False/Progressing, persists the status update path used elsewhere in the
function, then return to keep behavior consistent.
In `@internal/management/openstack.go`:
- Around line 85-88: Validate the incoming PowerState in SetPowerState before
calling nodes.ChangePowerState: check that the target parameter is one of the
allowed constants (PowerOn or PowerOff) and return a descriptive error if not;
then only call nodes.ChangePowerState(ctx, c.client, hostID,
nodes.PowerStateOpts{Target: nodes.TargetPowerState(target)}) when the target is
valid. This keeps validation symmetric with GetPowerState and prevents sending
invalid targets to Ironic.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0d371344-ea01-4d1a-8d9e-850ccb2d273e
📒 Files selected for processing (8)
cmd/main.gointernal/controller/hostlease_controller.gointernal/controller/hostlease_controller_test.gointernal/ironic/client.gointernal/ironic/client_test.gointernal/management/client.gointernal/management/management_test.gointernal/management/openstack.go
💤 Files with no reviewable changes (2)
- internal/ironic/client_test.go
- internal/ironic/client.go
DanNiESh
force-pushed
the
host-management-interface
branch
from
07d2684 to
b891357
Compare
…erface Introduce a management.Client interface with GetPowerState/SetPowerState methods and a config-driven factory/registry pattern, mirroring the inventory interface in bare-metal-fulfillment-operator. The OpenStack implementation wraps gophercloud Ironic calls. The controller and cmd/main.go are updated to use management.Client, and internal/ironic/ is removed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DanNiESh
force-pushed
the
host-management-interface
branch
from
b891357 to
b3fa6bc
Compare
Contributor
Author
|
@coderabbitai have all issues been addressed? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check convergence logic in hostlease_controller.go for IsTransitioning guard
echo "=== hostlease_controller.go: syncHostLeaseStatus and convergence block ==="
cat internal/controller/hostlease_controller.go
echo ""
echo "=== cmd/main.go: auth error logging ==="
cat cmd/main.goLength of output: 29815
✅ Issue 1 — Convergence check ignores
|
tzumainn
dismissed
coderabbitai[bot]’s stale review
Looks like all requested changes have been made!
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.
Closes osac-project/issues#404
Introduce a
management.Clientinterface withGetPowerState/SetPowerStatemethods and a config-driven factory/registry pattern, mirroring the inventory interface inbare-metal-fulfillment-operator. The OpenStack implementation wraps gophercloud Ironic calls. The controller andcmd/main.goare updated to usemanagement.Client, andinternal/ironic/is removed.Summary by CodeRabbit