fix: set initial Pending status condition on queued PipelineRuns - #511
Conversation
PR Summary by QodoSet initial Pending status condition for queued Tekton PipelineRuns
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
10 rules 1.
|
![qodo-app-for-konflux-ci[bot]](https://avatars.githubusercontent.com/u/155583288?s=60&v=4){kind=small}
54bfa48 to
92b0333
Compare
|
/kind bug |
| // controller skips PipelineRuns with spec.managedBy set, so without this | ||
| // reconciler the status stays blank in CLI and UI tools until Kueue admits | ||
| // the workload and dispatches it to a spoke cluster. | ||
| type pipelineRunStatusReconciler struct { |
There was a problem hiding this comment.
Please keep this reconciler in its own file.
There was a problem hiding this comment.
Made the changes, moved the reconciler into its own new file.
| Type: kapi.ConditionSucceeded, | ||
| Status: corev1.ConditionUnknown, | ||
| Reason: "PipelineRunPending", | ||
| Message: "PipelineRun is pending, waiting for Kueue admission", |
There was a problem hiding this comment.
Lets say waiting for multikueue admission
There was a problem hiding this comment.
Updated the Message field accordingly.
|
@pramodbindal I have made the changes as per your suggestions, please have a look. |
| client client.Client | ||
| } | ||
|
|
||
| func (r *pipelineRunStatusReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { |
There was a problem hiding this comment.
Can we add this logic to the existing controller https://github.com/konflux-ci/tekton-kueue/blob/main/internal/controller/pipelinerun_controller.go ?
There are many hook function that kueue's job framework provides, can we use any of those ?
There was a problem hiding this comment.
@gbenhaim I checked the available hook interfaces in kueue's job framework. The hooks that have client access (such as Stop, Finalize, and GetCustomAnnotations) are only invoked during eviction, finalization, or workload slicing.
They aren't called while a PipelineRun is in pending / yet to be admitted state, which is when we need to update the status condition. So I don't think the existing hooks can handle this use case.
There was a problem hiding this comment.
@gbenhaim
Here we are updating the pipeline which suspended and will be picked by one of the spoke clusters.
I think adding a dedicated reconciler is right thing to do
There was a problem hiding this comment.
Verified by adding debug logs to existing hooks, that the hooks that get called are PodSets, Finished, IsSuspended and isActive, but these are pure data methods with no client access, so cannot make API calls.
c6e9feb to
574d25c
Compare
gbenhaim
left a comment
gbenhaim
left a comment
There was a problem hiding this comment.
This review was posted by Gal's Cursor.
| return err | ||
| } | ||
|
|
||
| return SetupPendingStatusWithManager(mgr) |
There was a problem hiding this comment.
Suggestion: Following kubebuilder conventions, each controller should be registered independently from the entrypoint (cmd/main.go), rather than having one controller register another. This makes the lifecycle explicit and consistent with how SetupIndexer and the ConfigMapReconciler are already registered separately.
Move SetupPendingStatusWithManager(mgr) to cmd/main.go:
// cmd/main.go
err = controller.SetupWithManager(ctx, mgr)
// ...
if err = controller.SetupPendingStatusWithManager(mgr); err != nil {
setupLog.Error(err, "Failed to setup pending status controller")
os.Exit(1)
}And revert SetupWithManager to return directly:
return reconciler.SetupWithManager(mgr)There was a problem hiding this comment.
Moved the controller registration to cmd/main.go, SetupWithManager now returns directly (reverted all the changes in pipelinerun_controller.go).
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| plr.Status.Conditions = append(plr.Status.Conditions, kapi.Condition{ |
There was a problem hiding this comment.
Nit: PipelineRunStatus has a SetCondition method that handles deduplication and keeps conditions sorted. While the GetCondition != nil guard above prevents duplicates, using SetCondition is more defensive and idiomatic with the Tekton API:
plr.Status.SetCondition(&kapi.Condition{
Type: kapi.ConditionSucceeded,
Status: corev1.ConditionUnknown,
Reason: "PipelineRunPending",
Message: "PipelineRun is pending, waiting for MultiKueue admission",
LastTransitionTime: kapi.VolatileTime{Inner: metav1.Now()},
})There was a problem hiding this comment.
Switched to SetCondition as suggested.
574d25c to
f2e5753
Compare
|
@gbenhaim I have updated the PR with all the changes suggested in the review comments. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gbenhaim, pramodbindal The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
Signed-off-by: Ankur Sinha <anksinha@redhat.com>
f2e5753 to
fa2102c
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
5 participants
What
Sets an initial Succeeded=Unknown / Reason=
PipelineRunPendingstatus condition on PipelineRuns that are queued by tekton-kueue.A lightweight status reconciler is registered alongside the existing Kueue workload reconciler in
SetupWithManager. When it sees a PipelineRun withspec.status=PipelineRunPendingand emptystatus.conditions, it writes the pending condition via the status subresource.Why
The tekton-kueue webhook sets
spec.managedBy=kueue.x-k8s.io/multikueueon every PipelineRun. Tekton's own controller filters out any PipelineRun whosemanagedByis nottekton.dev/pipeline, so it never reconciles these runs and never populates status.conditions.Since
kubectl get pipelinerun,tkn/opc, and the OpenShift Console UI all readstatus.conditionsto display status, queued PipelineRuns show a blank/--- status instead of Pending - making it look like the run is broken rather than waiting for admission.Changes
pipelineRunStatusReconcilerand registered it inSetupWithManager. Added RBAC marker for pipelineruns/status.Verification
make lintpassesmake testpassestekton-kueue mutateCLI (if CEL/webhook changes)make manifestsandmake generaterun cleanly (if API/RBAC changes)