Exclude PoweredOn from provisioning config version hash

[DanNiESh]

PoweredOn is handled by reconcilePower() which directly calls the management backend API - it does not need the provisioning template. Including it in the hash caused the template to re-trigger on every power state toggle.
Closes: osac-project/issues#407

[coderabbitai]

Warning

Review limit reached

@DanNiESh, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 7 minutes and 51 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: ecc4868e-d262-43dc-b1ee-5593284aac31

📥 Commits

Reviewing files that changed from the base of the PR and between a126183 and ba0d948.

📒 Files selected for processing (1)

• internal/controller/hostlease_controller.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[DanNiESh]

@coderabbitai can you review now?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details
gh pr view 12 --repo osac-project/host-management-openstack

echo "=== PR DIFF ==="
gh pr diff 12 --repo osac-project/host-management-openstack

Length of output: 2290

---

🏁 Script executed:

#!/bin/bash
echo "=== HostLeaseSpec definition ==="
rg -n "HostLeaseSpec" --type go -A 40

echo "=== ComputeDesiredConfigVersion ==="
rg -n "ComputeDesiredConfigVersion" --type go -A 20

echo "=== reconcilePower in hostlease_controller.go ==="
rg -n "reconcilePower" --type go -A 30

echo "=== Full provisioning package files ==="
fd . --type f -e go | xargs grep -l "ComputeDesiredConfigVersion" 2>/dev/null

Length of output: 68741

---

🏁 Script executed:

#!/bin/bash
echo "=== HostLeaseSpec full definition ==="
fd . --type f -e go | xargs grep -l "type HostLeaseSpec struct" 2>/dev/null | xargs cat

echo "=== Find provisioning package ==="
fd . --type f -e go | xargs grep -l "func ComputeDesiredConfigVersion" 2>/dev/null | xargs cat

echo "=== hostlease_controller.go full relevant section (lines 340-400) ==="
cat internal/controller/hostlease_controller.go | head -420 | tail -100

Length of output: 4073

---

🏁 Script executed:

#!/bin/bash
echo "=== All Go files in repo ==="
fd . --type f -e go

echo "=== go.mod ==="
cat go.mod 2>/dev/null || echo "no go.mod at root"

echo "=== Find any HostLeaseSpec references ==="
rg -rn "PoweredOn" --type go

echo "=== Find provisioning package location ==="
fd provisioning --type d
fd "*.go" --type f | xargs grep -l "func ComputeDesiredConfigVersion" 2>/dev/null

Length of output: 9754

---

---

@DanNiESh The logic behind the fix is sound — PoweredOn is purely a power-control concern handled by reconcilePower, so excluding it from the provisioning config hash is the right call. Here are a few security/correctness observations:

---

🟡 Medium — Positional struct initialization (silent mis-assignment risk)

The anonymous struct literal uses positional (unnamed) field assignment:

provisioning.ComputeDesiredConfigVersion(struct {
    HostType                  string
    ExternalHostID            string
    ExternalHostName          string
    HostClass                 string
    NetworkClass              string
    Selector                  v1alpha1.HostSelectorSpec
    InventoryLabels           map[string]string
    InventoryPersistentLabels map[string]string
    TemplateID                string
    TemplateParameters        string
}{
    hostLease.Spec.HostType,          // positional — tied to declaration order
    hostLease.Spec.ExternalHostID,
    ...
})

If someone reorders the field declarations, the values will be silently mis-assigned (e.g. HostType gets ExternalHostID's value), producing an incorrect hash with no compile error. This is a correctness risk that could silently corrupt the config version fingerprint.

Recommendation: Use named field initialization:

}{
    HostType:                  hostLease.Spec.HostType,
    ExternalHostID:            hostLease.Spec.ExternalHostID,
    ExternalHostName:          hostLease.Spec.ExternalHostName,
    HostClass:                 hostLease.Spec.HostClass,
    NetworkClass:              hostLease.Spec.NetworkClass,
    Selector:                  hostLease.Spec.Selector,
    InventoryLabels:           hostLease.Spec.InventoryLabels,
    InventoryPersistentLabels: hostLease.Spec.InventoryPersistentLabels,
    TemplateID:                hostLease.Spec.TemplateID,
    TemplateParameters:        hostLease.Spec.TemplateParameters,
}

---

🟡 Medium — Silent maintenance trap: new spec fields could be silently omitted

The anonymous struct is a manual allowlist. If a new field is added to HostLeaseSpec that should affect the provisioning hash (e.g. a new network config or image override field), nothing in the compiler or type system will remind anyone to add it here. The behavior will silently be wrong.

Recommendation: Consider defining a named type HostLeaseProvisioningSpec (either in the API types or locally) and using it as the input to ComputeDesiredConfigVersion. This makes the intent explicit and makes omissions easier to spot during review.

---

🔵 Low — No regression test for the hash exclusion

The bug was that a PoweredOn toggle caused reconcileProvisioning to re-trigger. A targeted test verifying that changing only PoweredOn does not change the output of ComputeDesiredConfigVersion (or that reconcileProvisioning is not re-triggered) would guard against this regression.

---

✅ Looks good

• reconcilePower correctly uses hostLease.Spec.ExternalHostID throughout (Line 316, 333, etc.) — consistent with the operator convention.
• The separation of concerns (power via reconcilePower, provisioning via reconcileProvisioning) is clean and the fix aligns with that design.

---

🧠 Learnings used

Learnt from: larsks
Repo: osac-project/host-management-openstack PR: 1
File: internal/controller/host_controller.go:105-125
Timestamp: 2026-04-06T21:51:20.735Z
Learning: In `internal/controller/host_controller.go` (Go, controller-runtime operator), the `reconcilePower` method should use `host.Spec.ExternalID` (not `host.Spec.ID`) when referencing the Ironic node UUID, consistent with the Host CR spec field `externalID`. Power state constants should use `string(nodes.PowerOn)` and `string(nodes.PowerOff)` from gophercloud rather than raw string literals.

[tzumainn]

Thanks!