Adjust Host Lease phases so that it is not Ready until after its template is complete

[DanNiESh]

Closes osac-project/issues#396

Summary

• Reset HostLease phase to Progressing at the start of each reconcile. This ensures a HostLease that was previously Ready transitions back to Progressing when a new provisioning job is triggered (e.g., after a spec change).
• Return early from handleUpdate() when the provisioning OnFailed() callback sets the phase to Failed, preventing line 174 from overwriting it with Ready.

Problem

Two bugs in the phase transition logic:

1. hostLease.Status.Phase was only set to Progressing when empty (""). If a HostLease was already Ready and a new provisioning job was triggered, the phase stayed Ready while the ProvisionTemplateComplete condition showed
   Progressing.
2. When a provisioning job failed, PollJob in RunProvisioningLifecycle called OnFailed() (which set the phase to
   Failed) but returned (ctrl.Result{}, nil). Both guards in handleUpdate (provErr != nil and !result.IsZero()) passed, so execution fell through to hostLease.Status.Phase = HostLeasePhaseReady, overwriting Failed with Ready.

Summary by CodeRabbit

• Bug Fixes
  • Improved status reporting during host-lease lifecycle: the system now advances "Progressing" at key transitions, marks "Failed" when finalizer updates or provisioning checks fail, and halts reconciliation promptly on provisioning errors.
  • Ensures "Progressing" is set before scheduling requeues for power-state convergence to avoid misleading status.

[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 37 minutes and 15 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: bf2e2221-36a9-4d8b-9562-993b372e70ce

📥 Commits

Reviewing files that changed from the base of the PR and between 8e726fa and 07ab38f.

📒 Files selected for processing (1)

• internal/controller/hostlease_controller.go

Walkthrough

The reconciler's update path now sets HostLease.Status.Phase explicitly at finalizer update (Failed/Progressing), when provisioning returns a non-zero result (Progressing), when a provisioning template is present but not True (Failed and early return), and before returning for power-state requeue (Progressing).

Changes

HostLease Phase Reconciliation

Layer / File(s)	Summary
Finalizer update phase handling internal/controller/hostlease_controller.go	After adding the finalizer in handleUpdate, the code sets HostLeasePhaseFailed on finalizer-update error and sets HostLeasePhaseProgressing after a successful finalizer update.
Provisioning short-circuit and phase set internal/controller/hostlease_controller.go	If reconcileProvisioning returns a non-zero result, handleUpdate sets HostLeasePhaseProgressing and returns that result immediately.
Provision template completion check internal/controller/hostlease_controller.go	After provisioning, if HostConditionProvisionTemplateComplete exists but is not True, handleUpdate sets HostLeasePhaseFailed, logs the condition, and returns early.
Power reconciliation requeue phase set internal/controller/hostlease_controller.go	When desired PoweredOn doesn't match observed state and a requeue is scheduled, handleUpdate sets HostLeasePhaseProgressing before returning.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• #396: Also concerns HostLease.Status.Phase transitions during provisioning and keeping leases in a progressing state while work is ongoing.

Poem

Finalizers settle, phases march to Progressing,
Provision checks pause where templates are missing,
Requeues hum on power's slow swing,
Failed flags note where answers don't cling,
Small host leases dance in steady processing. ✨

Security & Stability Note: Risk - Moderate. These changes alter state-machine progression and early-return behavior; mis-set phases could obscure error conditions or allow requeues to mask transient failures. Verify observers and downstream consumers rely on the same phase semantics.

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Ai-Attribution	⚠️ Warning	PR mentions "AI-generated summary" but commit contains no Assisted-by/Generated-by/Co-Authored-By trailers required for Red Hat attribution.	Add appropriate Git trailers (Assisted-by or Generated-by) to commit message documenting AI tool usage per Red Hat policy.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main change: ensuring HostLease phases transition properly and remain in Progressing state until template provisioning completes, preventing premature Ready status.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets found in hostlease_controller.go. All string literals are legitimate configuration values; AAP token is properly sourced from environment variable.
No-Weak-Crypto	✅ Passed	No weak cryptography usage detected. PR modifies Kubernetes controller state management with no cryptographic operations, cipher algorithms, or insecure comparisons.
No-Injection-Vectors	✅ Passed	Go code reviewed for injection vectors (SQL, shell, eval, pickle, yaml.load, os.system, innerHTML). None found; uses type-safe Kubernetes APIs.
Container-Privileges	✅ Passed	PR modifies Go controller logic only. No container/K8s manifests changed. Existing security configs: privilege escalation disabled, non-root user, all capabilities dropped, seccomp enabled.
No-Sensitive-Data-In-Logs	✅ Passed	The PR adds one new log.Info statement logging hostLease.Name and a status message—no passwords, tokens, API keys, PII, or other sensitive data is exposed.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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.}

[tzumainn]

Is this change needed? It feels odd to always unconditionally set the Phase to Progressing without knowing if anything actually needs updating. Or is the idea that we know that something has changed if reconciliation is triggered?

[DanNiESh]

I think the idea is something has changed if reconciliation is triggered. So if provisioning/power controll fails, it's overwritten to Failed and if everything succeeds, it's overwritten to Ready. On a no-op reconcile that nothing changed, this sets the phase to Progressing and then immediately to Ready.
Do we want to set Progressing only at the points where work is actually happening?

[tzumainn]

@larsks do you know what the standard k8s pattern is here?

[larsks]

I think the common pattern is, for a resource that has previously reached status.Phase = "Ready", to only set status.Phase to "Progressing" if the reconciliation loop is going to exit before the resource has reached the desired state (e.g., because you are waiting for some other activity to complete before the resource is ready).

This avoids short-lived user-visible changes on every reconcile call. Eg., if you are watching the resource for changes, you don't necessarily want to know every time the reconcile loop gets called -- you only want to know when actual work needs to happen.

[DanNiESh]

Update: set the phase to Ready at template provisioning/power control/adding finalizer. And at the end of reconcile, check if the v1alpha1.HostConditionProvisionTemplateComplete is true, if not, set phase to failed. This prevents from reaching Ready when a previous provisioning template failed and a second reconcile only does power control without re-triggering provisioning - the phase still keeps failed.

[tzumainn]

I think we still want to keep this - if the Phase is empty, it seems reasonable to assume that it's Progressing

[DanNiESh]

Fixed!

[tzumainn]

Thanks!