printers: Add Job status to jobs table #123226

ivanvc · 2024-02-09T23:49:11Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds the suspend column before completions for the Jobs table.

Which issue(s) this PR fixes:

Fixes #123221

Special notes for your reviewer:

Does this PR introduce a user-facing change?

`kubectl get job` now displays the status for the listed jobs.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-02-09T23:49:20Z

Hi @ivanvc. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

kannon92 · 2024-02-10T03:34:42Z

/ok-to-test

kannon92 · 2024-02-12T04:06:23Z

/sig cli
/sig apps
/wg batch

kaisoz · 2024-02-11T16:11:51Z

pkg/printers/internalversion/printers.go

@@ -167,6 +167,7 @@ func AddHandlers(h printers.PrintHandler) {

 	jobColumnDefinitions := []metav1.TableColumnDefinition{
 		{Name: "Name", Type: "string", Format: "name", Description: metav1.ObjectMeta{}.SwaggerDoc()["name"]},
+		{Name: "Suspend", Type: "boolean", Description: batchv1.JobSpec{}.SwaggerDoc()["suspend"]},


I rather prefer Suspended here as it conveys better the state of the job. I find using the verb Suspend a bit confusing

I'm now leaning more towards the approach as we have for pods where we have the STATUS column reflecting the current pod state, doing lookup to multiple places: https://github.com/kubernetes/kubernetes/blob/master/pkg/printers/internalversion/printers.go#L834-L959.

The states I can we could have for Job:

Complete - when has the Complete=True condition

Failed - when has the Failed=True condition

FailureTarget - when has the FailureTarget=True condition

Suspended - when has the Suspended=True condition, and.spec.suspend=true

Resuming - when has the Suspended=True condition, but .spec.suspend != true

Suspending - when has the .spec.suspend=true, but no Suspended=True condition

Terminating - when deletionTimestamp != nil

Running - none of the above, potentially maybe when status.startTime != nil (and the we could have Starting when .status.startTime=nil.

WDYT @kannon92 @alculquicondor @soltysh ?

I wouldn't make a distinction between suspended, resuming, suspending. I would just use the condition.
Other than that, SGTM

I like the change suggested by @mimowo.

I'm ok either with dropping Resuming and Suspending or keeping them.

Regardless, we should set the state based on condition as the actual state, not the spec.suspend (desired state).

Do you suggest closing this PR, and starting a new one?

Do we have a consensus on keeping or removing Suspending and Resuming?

I originally had it as "Suspended", but then I saw that CronJob's suspend is shown as "Suspend",

I see, but here are some differences with CronJob: I don't think CronJob supports conditions, can it be "finished"? I feel since job can be in multiple "states" it makes sense to put them into a single column to avoid creating a column per field or condition. Similarly as for pod.

Do you suggest closing this PR, and starting a new one?

I would suggest updating this one. You can put a new PR so that we don't lose history in case some changes get reverted due to new insights, but I think keeping the PR open is beneficial to also keep discussions in one place, because they would be similar in the new PR.

Do we have a consensus on keeping or removing Suspending and Resuming?

The transient states resuming and suspending are very short, so I'm ok either way.

@mimowo, @kaisoz, @kannon92, @alculquicondor PTAL @ #123349 which supersedes this PR.

You can add commit's to this PR.

Sounds good, I'll close the other one.

kaisoz · 2024-02-11T16:22:35Z

pkg/printers/internalversion/printers.go

@@ -1156,7 +1157,7 @@ func printJob(obj *batch.Job, options printers.GenerateOptions) ([]metav1.TableR
 		jobDuration = duration.HumanDuration(obj.Status.CompletionTime.Sub(obj.Status.StartTime.Time))
 	}

-	row.Cells = append(row.Cells, obj.Name, completions, jobDuration, translateTimestampSince(obj.CreationTimestamp))
+	row.Cells = append(row.Cells, obj.Name, printBoolPtr(obj.Spec.Suspend), completions, jobDuration, translateTimestampSince(obj.CreationTimestamp))


IMO the column should only show a boolean True or False.

printBoolPtr will print <unset> in case obj.Spec.Suspend is nil. Since a job is not suspended by default I think the column should read False in such case (<unset> doesn't tell you the state of the job)

I think if we are going to directly expose the value of the .spec.suspend field, then probably the 3 values: <unset>, true, false makes sense, but I also think that what the user is mostly interested in is the state of the job, as mentioned here: #123226 (comment).

ivanvc · 2024-02-17T05:10:14Z

/test pull-kubernetes-verify

kannon92 · 2024-03-01T19:05:26Z

pkg/printers/internalversion/printers_test.go

+				Status: batch.JobStatus{
+					Succeeded: 0,
+					Conditions: []batch.JobCondition{
+						{


I would probably include Failed or Complete. Not both. It is a bit confusing.

kannon92 · 2024-03-01T19:06:20Z

pkg/printers/internalversion/printers_test.go

+					Succeeded: 0,
+					Conditions: []batch.JobCondition{
+						{
+							Type:   batch.JobFailureTarget,


Why is JobFailureTarget Needed?

I'm going to delete it. It's a remaining when the conditions weren't in the current order.

kannon92 · 2024-03-01T19:07:15Z

Very close. Had some minor items on the tests but I think it LGTM.

/assign @alculquicondor
not sure who needs to look this over on sig-cli side.

kannon92 · 2024-03-01T20:05:50Z

/lgtm

k8s-ci-robot · 2024-03-01T20:05:56Z

LGTM label has been added.

Git tree hash: d2db278a5711a7e79ad84d4439ebc50c02359d88

alculquicondor

/approve

alculquicondor · 2024-03-05T15:44:05Z

/retitle printers: Add Job status to jobs table.

alculquicondor · 2024-03-05T15:44:21Z

/retitle printers: Add Job status to jobs table

alculquicondor · 2024-03-05T15:44:50Z

/release-note-edit

`kubectl get job` now displays the status for the listed jobs.

soltysh

/approve

soltysh · 2024-03-05T15:57:23Z

/triage accepted
/priority backlog

k8s-ci-robot · 2024-03-05T15:57:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, ivanvc, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/printers/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 9, 2024

k8s-ci-robot requested review from ardaguclu and seans3 February 9, 2024 23:49

ivanvc mentioned this pull request Feb 9, 2024

Jobs should display whether or not they are suspended. #123221

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 10, 2024

kaisoz suggested changes Feb 12, 2024

View reviewed changes

ivanvc mentioned this pull request Feb 16, 2024

printers: add status to jobs table #123349

Closed

ivanvc force-pushed the add-suspend-to-jobs-table branch from 04559e6 to 9925552 Compare February 17, 2024 04:25

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 17, 2024

ivanvc force-pushed the add-suspend-to-jobs-table branch from 9925552 to 46ff408 Compare February 17, 2024 05:43

ivanvc requested review from alculquicondor, kannon92 and kaisoz February 17, 2024 06:46

ivanvc requested review from kannon92 and alculquicondor March 1, 2024 18:53

kannon92 reviewed Mar 1, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor Mar 1, 2024

printers: add status to jobs table

8854d36

ivanvc force-pushed the add-suspend-to-jobs-table branch from 16caa9e to 8854d36 Compare March 1, 2024 19:58

ivanvc requested a review from kannon92 March 1, 2024 19:59

k8s-ci-robot assigned kannon92 Mar 1, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2024

alculquicondor reviewed Mar 5, 2024

View reviewed changes

k8s-ci-robot changed the title ~~printers: add suspend to jobs table~~ printers: Add Job status to jobs table. Mar 5, 2024

k8s-ci-robot changed the title ~~printers: Add Job status to jobs table.~~ printers: Add Job status to jobs table Mar 5, 2024

soltysh approved these changes Mar 5, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2024

k8s-ci-robot merged commit a814115 into kubernetes:master Mar 5, 2024
13 of 14 checks passed

k8s-ci-robot added this to the v1.30 milestone Mar 5, 2024

ivanvc deleted the add-suspend-to-jobs-table branch March 5, 2024 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

printers: Add Job status to jobs table #123226

printers: Add Job status to jobs table #123226

ivanvc commented Feb 9, 2024 •

edited by k8s-ci-robot

k8s-ci-robot commented Feb 9, 2024

kannon92 commented Feb 10, 2024

kannon92 commented Feb 12, 2024

kaisoz Feb 11, 2024

mimowo Feb 12, 2024

alculquicondor Feb 12, 2024

kannon92 Feb 12, 2024

mimowo Feb 12, 2024

ivanvc Feb 13, 2024

mimowo Feb 14, 2024

ivanvc Feb 16, 2024

kannon92 Feb 16, 2024

ivanvc Feb 16, 2024

kaisoz Feb 11, 2024

mimowo Feb 12, 2024

ivanvc commented Feb 17, 2024

kannon92 Mar 1, 2024

ivanvc Mar 1, 2024

kannon92 Mar 1, 2024

ivanvc Mar 1, 2024

ivanvc Mar 1, 2024

kannon92 commented Mar 1, 2024

kannon92 commented Mar 1, 2024

k8s-ci-robot commented Mar 1, 2024

alculquicondor left a comment

alculquicondor commented Mar 5, 2024

alculquicondor commented Mar 5, 2024

alculquicondor commented Mar 5, 2024

soltysh left a comment

soltysh commented Mar 5, 2024

k8s-ci-robot commented Mar 5, 2024

printers: Add Job status to jobs table #123226

printers: Add Job status to jobs table #123226

Conversation

ivanvc commented Feb 9, 2024 • edited by k8s-ci-robot

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Feb 9, 2024

kannon92 commented Feb 10, 2024

kannon92 commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanvc commented Feb 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannon92 commented Mar 1, 2024

kannon92 commented Mar 1, 2024

k8s-ci-robot commented Mar 1, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor commented Mar 5, 2024

alculquicondor commented Mar 5, 2024

alculquicondor commented Mar 5, 2024

soltysh left a comment

Choose a reason for hiding this comment

soltysh commented Mar 5, 2024

k8s-ci-robot commented Mar 5, 2024

ivanvc commented Feb 9, 2024 •

edited by k8s-ci-robot