This repository was archived by the owner on Aug 4, 2026. It is now read-only.
OSAC-1459: Extend provisioning lifecycle for multi-job tracking - #400
Open
SiddarthR56 wants to merge 1 commit into
Open
OSAC-1459: Extend provisioning lifecycle for multi-job tracking#400SiddarthR56 wants to merge 1 commit into
SiddarthR56 wants to merge 1 commit into
Conversation
|
@SiddarthR56: This pull request references OSAC-1459 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 task 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SiddarthR56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds fabric and k8s job targets to API and CRD schemas, scopes provisioning lifecycle and backoff behavior by target, orchestrates multiple targets over shared job history, and adds compatibility and orchestration tests. ChangesMulti-target provisioning
Sequence Diagram(s)sequenceDiagram
participant Controller
participant RunMultiTargetProvisioningLifecycle
participant RunProvisioningLifecycle
participant JobHistory
Controller->>RunMultiTargetProvisioningLifecycle: provide target specifications
RunMultiTargetProvisioningLifecycle->>RunProvisioningLifecycle: execute each target
RunProvisioningLifecycle->>JobHistory: find or append target-scoped JobStatus
RunProvisioningLifecycle-->>RunMultiTargetProvisioningLifecycle: return result or error
RunMultiTargetProvisioningLifecycle-->>Controller: combined result and readiness
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/provisioning/provision_lifecycle.go (1)
96-108: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSwallowed
apiReader.Geterror.Returning
falseon a Get failure silently opts into "no non-terminal job exists" and proceeds to trigger. At minimum log it — you already havelogin scope, and a transient API error here is exactly the case worth seeing in logs.As per path instructions: "Never ignore error returns".
♻️ Proposed tweak
if err := apiReader.Get(ctx, key, fresh); err != nil { + log.V(1).Info("API server safety check unavailable, proceeding", "error", err.Error(), "target", target) return false }🤖 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 `@pkg/provisioning/provision_lifecycle.go` around lines 96 - 108, Update CheckAPIServerForNonTerminalProvisionJobForTarget to log the error returned by apiReader.Get before returning false, using the existing log from ctrllog.FromContext and including sufficient context to identify the failed lookup. Do not silently discard the error.Source: Path instructions
🤖 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 `@pkg/provisioning/provision_lifecycle_test.go`:
- Around line 635-650: Add a test case alongside the existing “returns the
soonest RequeueAfter across targets” coverage that makes both targets produce
distinct non-zero RequeueAfter durations, such as one backoff result and one
polling result. Invoke RunMultiTargetProvisioningLifecycle with those targets
and assert result.RequeueAfter equals the smaller duration, exercising the
non-zero comparison in combineResults.
- Around line 461-476: The test for FindLatestJobByTypeAndTarget only verifies
target filtering, not latest-job selection. Extend the shared jobs fixture with
two fabric provision jobs having different timestamps and IDs, then assert the
lookup for JobTargetFabric returns the newer fabric job while preserving the
existing independent K8s-target assertion.
- Around line 585-599: Move the spec currently named “reports Ready only once
every target has succeeded” out of the lifecycle Describe into a dedicated
AllTargetsApplied Describe, since it only invokes AllTargetsApplied and never
RunMultiTargetProvisioningLifecycle. Keep the existing assertions and job state
transition unchanged.
In `@pkg/provisioning/provision_lifecycle.go`:
- Around line 358-365: Update AllTargetsApplied to return false when targets is
empty, before iterating through the targets; retain the existing
IsConfigAppliedForTarget checks and true result only when at least one target
exists and all targets are applied.
- Around line 306-321: The combineResults function should stop propagating the
deprecated ctrl.Result.Requeue boolean. Remove the Requeue assignment and have
it combine only the RequeueAfter values, preserving the existing soonest-nonzero
selection behavior for this non-error path.
- Around line 124-135: Update the provisioning history handling around AppendJob
and FindLatestJobByTypeAndTarget so maxHistory is applied per target, or ensure
eviction cannot make the current target’s newly appended job invisible. Preserve
target-scoped lookup behavior used by EvaluateAction and
CheckAPIServer...ForTarget, and guard the latestJob logging path against an
absent result if eviction can still occur.
- Around line 258-262: Prevent EvaluateAction from unconditionally invoking a
nil Callbacks.CheckAPIServer. Either document CheckAPIServer as required and
validate TargetSpec construction before reconciliation, or provide a safe
default so TargetSpec instances without it fail validation rather than panic in
the reconcile loop.
---
Outside diff comments:
In `@pkg/provisioning/provision_lifecycle.go`:
- Around line 96-108: Update CheckAPIServerForNonTerminalProvisionJobForTarget
to log the error returned by apiReader.Get before returning false, using the
existing log from ctrllog.FromContext and including sufficient context to
identify the failed lookup. Do not silently discard the error.
🪄 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: Pro Plus
Run ID: b978225a-3f3a-48e8-8d85-6bab54f4a935
📒 Files selected for processing (25)
api/v1alpha1/job_types.gocharts/operator-crds/templates/osac.openshift.io_clusterorders.yamlcharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcharts/operator-crds/templates/osac.openshift.io_externalipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_externalippools.yamlcharts/operator-crds/templates/osac.openshift.io_externalips.yamlcharts/operator-crds/templates/osac.openshift.io_natgateways.yamlcharts/operator-crds/templates/osac.openshift.io_securitygroups.yamlcharts/operator-crds/templates/osac.openshift.io_subnets.yamlcharts/operator-crds/templates/osac.openshift.io_tenants.yamlcharts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlconfig/crd/bases/osac.openshift.io_clusterorders.yamlconfig/crd/bases/osac.openshift.io_computeinstances.yamlconfig/crd/bases/osac.openshift.io_externalipattachments.yamlconfig/crd/bases/osac.openshift.io_externalippools.yamlconfig/crd/bases/osac.openshift.io_externalips.yamlconfig/crd/bases/osac.openshift.io_natgateways.yamlconfig/crd/bases/osac.openshift.io_securitygroups.yamlconfig/crd/bases/osac.openshift.io_subnets.yamlconfig/crd/bases/osac.openshift.io_tenants.yamlconfig/crd/bases/osac.openshift.io_virtualnetworks.yamlpkg/provisioning/job_helpers.gopkg/provisioning/provision_helpers.gopkg/provisioning/provision_lifecycle.gopkg/provisioning/provision_lifecycle_test.go
Assisted-by: Cursor/Claude
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@pkg/provisioning/job_helpers.go`:
- Around line 29-37: Update the generated ComputeInstance API documentation and
related size assumptions to describe job history as limited to maxHistory
entries per normalized target group, not across the entire status history.
Locate the status-history schema documentation associated with
config/crd/bases/osac.openshift.io_computeinstances.yaml and keep it consistent
with AppendJob’s grouping behavior.
- Around line 41-44: Update the k8s deprovision flow in the AppendJob caller
within provision_lifecycle.go to populate JobStatus.Target before appending the
job, using the same k8s target value as the deprovisioned job. Ensure every
AppendJob invocation propagates Target so normalizeTarget and target-scoped
retention and lookup keep the job in the correct bucket.
In `@pkg/provisioning/provision_lifecycle_test.go`:
- Around line 594-614: Add a lifecycle-level Ginkgo spec alongside the existing
legacy fabric upgrade test that runs RunProvisioningLifecycle with the same
succeeded legacy fabric job but State.Target set to JobTargetK8s. Assert no
error, verify the provider creates the pending “k8s-job”, and confirm the jobs
slice contains both entries with FindLatestJobByTypeAndTarget locating the new
K8s provision job.
In `@pkg/provisioning/provision_lifecycle.go`:
- Around line 397-406: Open a follow-up issue to track implementing
target-scoped deprovisioning counterparts before any multi-target resource
supports deletion, covering PollDeprovisionJob,
updateProvisionJobFromDeprovisionResult, and RunDeprovisioningLifecycle and
mirroring RunMultiTargetProvisioningLifecycle.
🪄 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: Pro Plus
Run ID: 26988314-c94b-40a7-95fb-f90f61471761
📒 Files selected for processing (27)
api/v1alpha1/job_types.gocharts/operator-crds/templates/osac.openshift.io_clusterorders.yamlcharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcharts/operator-crds/templates/osac.openshift.io_externalipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_externalippools.yamlcharts/operator-crds/templates/osac.openshift.io_externalips.yamlcharts/operator-crds/templates/osac.openshift.io_natgateways.yamlcharts/operator-crds/templates/osac.openshift.io_securitygroups.yamlcharts/operator-crds/templates/osac.openshift.io_subnets.yamlcharts/operator-crds/templates/osac.openshift.io_tenants.yamlcharts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlconfig/crd/bases/osac.openshift.io_clusterorders.yamlconfig/crd/bases/osac.openshift.io_computeinstances.yamlconfig/crd/bases/osac.openshift.io_externalipattachments.yamlconfig/crd/bases/osac.openshift.io_externalippools.yamlconfig/crd/bases/osac.openshift.io_externalips.yamlconfig/crd/bases/osac.openshift.io_natgateways.yamlconfig/crd/bases/osac.openshift.io_securitygroups.yamlconfig/crd/bases/osac.openshift.io_subnets.yamlconfig/crd/bases/osac.openshift.io_tenants.yamlconfig/crd/bases/osac.openshift.io_virtualnetworks.yamlpkg/dispatcher/dispatch.gopkg/dispatcher/dispatch_test.gopkg/provisioning/job_helpers.gopkg/provisioning/provision_helpers.gopkg/provisioning/provision_lifecycle.gopkg/provisioning/provision_lifecycle_test.go
Comment on lines
+29
to
+37
| // AppendJob adds a new job to the jobs array and trims history to maxHistory entries | ||
| // per target group (see normalizeTarget) rather than across the whole slice. Without | ||
| // this scoping, a target that retries repeatedly (e.g. failing backoff) could evict | ||
| // another target's sole tracked job out of history entirely, making | ||
| // FindLatestJobByTypeAndTarget see "no job" for it and trigger a duplicate. Provision | ||
| // and Deprovision jobs for the same target still share one budget, matching | ||
| // pre-existing single-target behavior — single-target resources never set Target, so | ||
| // all their jobs normalize to one group and this is equivalent to the old | ||
| // whole-slice trim. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Update the history-size contract.
This implementation retains up to maxHistory jobs per normalized target, but config/crd/bases/osac.openshift.io_computeinstances.yaml, Lines [366-369], still says the status is limited to the last N jobs overall. Update the generated API documentation and any size assumptions to state that the limit is per target group.
🤖 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 `@pkg/provisioning/job_helpers.go` around lines 29 - 37, Update the generated
ComputeInstance API documentation and related size assumptions to describe job
history as limited to maxHistory entries per normalized target group, not across
the entire status history. Locate the status-history schema documentation
associated with config/crd/bases/osac.openshift.io_computeinstances.yaml and
keep it consistent with AppendJob’s grouping behavior.
Comment on lines
+41
to
+44
| want := normalizeTarget(newJob.Target) | ||
| matchCount := 0 | ||
| for i := range jobs { | ||
| if normalizeTarget(jobs[i].Target) == want { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate Target through every AppendJob caller.
The deprovision path in pkg/provisioning/provision_lifecycle.go, Lines [434-441], still appends a JobStatus without Target. For a k8s deprovision, normalizeTarget("") places the job in the fabric history bucket, so target-scoped retention and lookup no longer track the job under k8s.
Proposed fix
AppendJob(*jobs, v1alpha1.JobStatus{
JobID: result.JobID,
Type: v1alpha1.JobTypeDeprovision,
+ Target: target,
State: v1alpha1.JobStatePending,🤖 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 `@pkg/provisioning/job_helpers.go` around lines 41 - 44, Update the k8s
deprovision flow in the AppendJob caller within provision_lifecycle.go to
populate JobStatus.Target before appending the job, using the same k8s target
value as the deprovisioned job. Ensure every AppendJob invocation propagates
Target so normalizeTarget and target-scoped retention and lookup keep the job in
the correct bucket.
Comment on lines
+594
to
+614
| ginkgo.It("does not re-trigger a legacy fabric job on upgrade into dual-dispatch", func() { | ||
| jobs := []v1alpha1.JobStatus{ | ||
| // Pre-existing job from before dual-dispatch: no Target set. | ||
| {JobID: "legacy-fabric", Type: v1alpha1.JobTypeProvision, State: v1alpha1.JobStateSucceeded, | ||
| ConfigVersion: "v1", Timestamp: metav1.NewTime(time.Now().UTC())}, | ||
| } | ||
| triggered := false | ||
| provider := &mockProvider{ | ||
| triggerProvisionFunc: func(_ context.Context, _ client.Object) (*ProvisionResult, error) { | ||
| triggered = true | ||
| return &ProvisionResult{JobID: "should-not-happen"}, nil | ||
| }, | ||
| } | ||
| fabricState := &State{Jobs: &jobs, DesiredConfigVersion: "v1", Target: v1alpha1.JobTargetFabric} | ||
| result, err := RunProvisioningLifecycle(ctx, provider, &v1alpha1.Subnet{}, fabricState, | ||
| 5, 30*time.Second, nil, noAPIServerJob, nil) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(triggered).To(BeFalse()) | ||
| Expect(result).To(Equal(ctrl.Result{})) | ||
| Expect(jobs).To(HaveLen(1)) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add the k8s half of this upgrade scenario.
This proves the legacy fabric job isn't re-triggered, but not the complementary and more consequential half: on upgrade into dual-dispatch, the k8s target must still trigger despite a succeeded legacy job sitting in the slice. That's only covered at the helper level (Line 547), not through the lifecycle.
💚 Suggested additional spec
ginkgo.It("still triggers the k8s target on upgrade into dual-dispatch", func() {
jobs := []v1alpha1.JobStatus{
{JobID: "legacy-fabric", Type: v1alpha1.JobTypeProvision, State: v1alpha1.JobStateSucceeded,
ConfigVersion: "v1", Timestamp: metav1.NewTime(time.Now().UTC())},
}
provider := &mockProvider{
triggerProvisionFunc: func(_ context.Context, _ client.Object) (*ProvisionResult, error) {
return &ProvisionResult{JobID: "k8s-job", InitialState: v1alpha1.JobStatePending}, nil
},
}
k8sState := &State{Jobs: &jobs, DesiredConfigVersion: "v1", Target: v1alpha1.JobTargetK8s}
_, err := RunProvisioningLifecycle(ctx, provider, &v1alpha1.Subnet{}, k8sState,
5, 30*time.Second, nil, noAPIServerJob, nil)
Expect(err).NotTo(HaveOccurred())
Expect(jobs).To(HaveLen(2))
Expect(FindLatestJobByTypeAndTarget(jobs, v1alpha1.JobTypeProvision, v1alpha1.JobTargetK8s).JobID).
To(Equal("k8s-job"))
})🤖 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 `@pkg/provisioning/provision_lifecycle_test.go` around lines 594 - 614, Add a
lifecycle-level Ginkgo spec alongside the existing legacy fabric upgrade test
that runs RunProvisioningLifecycle with the same succeeded legacy fabric job but
State.Target set to JobTargetK8s. Assert no error, verify the provider creates
the pending “k8s-job”, and confirm the jobs slice contains both entries with
FindLatestJobByTypeAndTarget locating the new K8s provision job.
Comment on lines
+397
to
+406
| // | ||
| // KNOWN LIMITATION: unlike the provisioning path, this function and the rest of the | ||
| // deprovisioning lifecycle (PollDeprovisionJob, updateProvisionJobFromDeprovisionResult, | ||
| // RunDeprovisioningLifecycle) are not target-aware — they use FindLatestJobByType, not | ||
| // FindLatestJobByTypeAndTarget. For a single-target resource this is exact. For a | ||
| // multi-target resource (once deprovisioning is wired up for one, e.g. Subnet), | ||
| // updateProvisionJobFromDeprovisionResult would update whichever target's provision job | ||
| // happens to have the latest timestamp, not necessarily the one the deprovision result | ||
| // actually corresponds to. Deprovisioning multi-target resources correctly requires | ||
| // target-scoped counterparts of these functions, mirroring RunMultiTargetProvisioningLifecycle. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
Track the non-target-aware deprovisioning gap before a multi-target resource wires deletion.
The comment accurately scopes the hazard: updateProvisionJobFromDeprovisionResult picks the latest provision job by timestamp regardless of target, so a dual-dispatch Subnet deletion would mutate the wrong target's job. Harmless today (no multi-target resource deprovisions), but the failure mode is silent when it lands.
Want me to open a follow-up issue for target-scoped deprovisioning counterparts?
🤖 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 `@pkg/provisioning/provision_lifecycle.go` around lines 397 - 406, Open a
follow-up issue to track implementing target-scoped deprovisioning counterparts
before any multi-target resource supports deletion, covering PollDeprovisionJob,
updateProvisionJobFromDeprovisionResult, and RunDeprovisioningLifecycle and
mirroring RunMultiTargetProvisioningLifecycle.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 by CodeRabbit
targetfield with allowed valuesfabricandk8sacross supported resources.