describer: improve job and pod template output #122618

ivanvc · 2024-01-06T00:22:25Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add Node-Selectors and Tolerations to pod template's describe output.
Add Suspend to job's describe output.

Which issue(s) this PR fixes:

Fixes #122613

Special notes for your reviewer:

Does this PR introduce a user-facing change?

`kubectl describe`: added Suspend to job, and Node-Selectors and Tolerations to pod template output

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

linux-foundation-easycla · 2024-01-06T00:22:31Z

The committers listed above are authorized under a signed CLA.

✅ login: ivanvc / name: Iván Valdés Castillo (6d65c06)

k8s-ci-robot · 2024-01-06T00:22:34Z

Welcome @ivanvc!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-01-06T00:22:35Z

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.

HirazawaUi · 2024-01-06T02:32:02Z

/ok-to-test

staging/src/k8s.io/kubectl/pkg/describe/describe.go

staging/src/k8s.io/kubectl/pkg/describe/describe_test.go

kaisoz · 2024-01-17T22:29:15Z

/lgtm

k8s-ci-robot · 2024-01-17T22:29:23Z

LGTM label has been added.

Git tree hash: dda7e67465193879ad1d0d5caeefaf7c53ddb025

mimowo

Thanks. LGTM overall, suggesting to print out other missing, important GA fields: BackoffLimit and TTLSecondsAfterFinished.

We also have a bunch of Beta-level fields in Job (such as PodFailurePolicy, BackoffLimitPerIndex, or PodReplacementPolicy), but I think it is ok to put them in a follow-up PR/Issue to make the distinction (or just wait for GA-level and print as they graduate).

mimowo · 2024-01-18T08:51:39Z

staging/src/k8s.io/kubectl/pkg/describe/describe_test.go

@@ -2516,8 +2519,8 @@ func TestDescribeDeployment(t *testing.T) {
 			expects: []string{
 				"Replicas:           2 desired | 1 updated | 3 total | 2 available | 1 unavailable",
 				"Image:        mytest-image:v2.0",
-				"OldReplicaSets:  bar-001 (2/2 replicas created)",
-				"NewReplicaSet:   bar-002 (1/1 replicas created)",
+				"OldReplicaSets:    bar-001 (2/2 replicas created)",


nit: revert unrelated formatting changes

When updating the above, please make sure to also address this one.

Hey @mimowo, the tests fail if I don't add the extra spaces here. I think it's due to the alignment of tabs with Node-Selectors. See:

--- FAIL: TestDescribeDeployment (0.00s) --- FAIL: TestDescribeDeployment/deployment_during_the_process_of_rolling_out (0.00s) describe_test.go:2839: expected to find "OldReplicaSets: bar-001 (2/2 replicas created)" in: Name: bar Namespace: foo CreationTimestamp: Fri, 01 Jan 2021 00:00:00 +0000 Labels: k8s-app=bar Annotations: <none> Selector: k8s-app=bar Replicas: 2 desired | 1 updated | 3 total | 2 available | 1 unavailable StrategyType: MinReadySeconds: 0 Pod Template: Labels: k8s-app=bar Containers: : Image: mytest-image:v2.0 Port: <none> Host Port: <none> Environment: <none> Mounts: /tmp/vol-bar from vol-bar (rw) /tmp/vol-foo from vol-foo (rw) Volumes: vol-foo: Type: EmptyDir (a temporary directory that shares a pod's lifetime) Medium: SizeLimit: <unset> vol-bar: Type: EmptyDir (a temporary directory that shares a pod's lifetime) Medium: SizeLimit: <unset> Node-Selectors: <none> Tolerations: <none> OldReplicaSets: bar-001 (2/2 replicas created) NewReplicaSet: bar-002 (1/1 replicas created) Events: Type Reason Age From Message ---- ------ ---- ---- ------- Normal ScalingReplicaSet 12m (x3 over 20m) deployment-controller Scaled up replica set bar-002 to 1 Normal ScalingReplicaSet 10m deployment-controller Scaled up replica set bar-001 to 2 Normal ScalingReplicaSet 2m deployment-controller Scaled up replica set bar-002 to 1 Normal ScalingReplicaSet 60s deployment-controller Scaled down replica set bar-002 to 1

I see, indeed the values for Node-Selectors Tolerations and OldReplicaSets are aligned in the full output.

Out of curiosity, I'm wondering where is the code / mechanism which determines the shift of OldReplicaSets to align it. Can you find it out? So that we better understand the intent, because some values are out of alignment, for example Volumes.vol-foo.Type.

This is the code for the empty dir volume: https://github.com/kubernetes/kubernetes/blob/d2b16b2/staging/src/k8s.io/kubectl/pkg/describe/describe.go#L1028-L1039
From reading the code, the value should be aligned, as it seems just a tab. This is the code for OldReplicaSets: https://github.com/kubernetes/kubernetes/blob/d2b16b2/staging/src/k8s.io/kubectl/pkg/describe/describe.go#L4372-L4375

mimowo · 2024-01-18T09:02:26Z

staging/src/k8s.io/kubectl/pkg/describe/describe.go

@@ -2313,6 +2315,9 @@ func describeJob(job *batchv1.Job, events *corev1.EventList) (string, error) {
 		if job.Spec.Parallelism != nil {
 			w.Write(LEVEL_0, "Parallelism:\t%d\n", *job.Spec.Parallelism)
 		}
+		if job.Spec.Suspend != nil {


Can we also print: BackoffLimit and TTLSecondsAfterFinished? These are other important GA-level fields. There is also ManualSelector, but I'm not sure it is relevant enough.

I'd advice against manualSelector, this should be used very cautiously and rarely. The other two are ok addition.

My biggest concern is you're adding this between two important parameters, parallelism and completions, which I frequently look at together. I'd suggest to move suspend printing below completion mode or before parallelism, iow:
your current output looks like:

Annotations: <none> Parallelism: 1 Suspend: false Completions: 1 Completion Mode: NonIndexed Start Time: Mon, 11 Dec 2023 15:18:09 +0100

but I'd suggest to have (suspend before parallelism):

Annotations: <none> Suspend: false Parallelism: 1 Completions: 1 Completion Mode: NonIndexed Start Time: Mon, 11 Dec 2023 15:18:09 +0100

or (suspend after completion mode)

Annotations: <none> Parallelism: 1 Completions: 1 Completion Mode: NonIndexed Suspend: false Start Time: Mon, 11 Dec 2023 15:18:09 +0100

@soltysh, any suggestion on where to place BackoffLimit and TTLSecondsAfterFinished? I'm assuming, going with your last suggestion, after Suspend and before Start Time. But I just wanted to check if you have an opinion here.

For me pretty much every configuration works, just don't split the Parallelism, completions and completionMode, as pointed out.

If I were to suggest something then maybe:

Annotations: batch.kubernetes.io/cronjob-scheduled-timestamp: 2024-01-19T08:48:00Z Controlled By: CronJob/hello Parallelism: 1 Completions: 1 Completion Mode: NonIndexed **Backoff Limit:** 6 **TTL Seconds After Finished**: 21 # seems reasonable to be close to `Completed At` Start Time: Fri, 19 Jan 2024 09:48:00 +0100 Completed At: Fri, 19 Jan 2024 09:48:13 +0100 Duration: 13s

Done, it now looks like the following:

Name: bar Namespace: foo Selector: Labels: <none> Annotations: <none> Completions: <unset> Suspend: true Backoff Limit: 1 TTL Seconds After Finished: 123 Pods Statuses: 0 Active / 0 Succeeded / 0 Failed Pod Template: Labels: <none> Containers: <none> Volumes: <none> Node-Selectors: <none> Tolerations: <none> Events: <none>

If these values are not specified, it omits them:

Name: bar Namespace: foo Selector: Labels: <none> Annotations: <none> Completions: <unset> Pods Statuses: 0 Active / 0 Succeeded / 0 Failed Pod Template: Labels: <none> Containers: <none> Volumes: <none> Node-Selectors: <none> Tolerations: <none> Events: <none>

soltysh · 2024-01-18T09:57:16Z

/triage accepted
/priority important-longterm

soltysh

@ivanvc once you address the comments pls ping me on slack

soltysh · 2024-01-18T09:59:02Z

staging/src/k8s.io/kubectl/pkg/describe/describe.go

@@ -2313,6 +2315,9 @@ func describeJob(job *batchv1.Job, events *corev1.EventList) (string, error) {
 		if job.Spec.Parallelism != nil {
 			w.Write(LEVEL_0, "Parallelism:\t%d\n", *job.Spec.Parallelism)
 		}
+		if job.Spec.Suspend != nil {


I'd advice against manualSelector, this should be used very cautiously and rarely. The other two are ok addition.

soltysh · 2024-01-18T10:03:01Z

staging/src/k8s.io/kubectl/pkg/describe/describe.go

@@ -2313,6 +2315,9 @@ func describeJob(job *batchv1.Job, events *corev1.EventList) (string, error) {
 		if job.Spec.Parallelism != nil {
 			w.Write(LEVEL_0, "Parallelism:\t%d\n", *job.Spec.Parallelism)
 		}
+		if job.Spec.Suspend != nil {


My biggest concern is you're adding this between two important parameters, parallelism and completions, which I frequently look at together. I'd suggest to move suspend printing below completion mode or before parallelism, iow:
your current output looks like:

Annotations: <none> Parallelism: 1 Suspend: false Completions: 1 Completion Mode: NonIndexed Start Time: Mon, 11 Dec 2023 15:18:09 +0100

but I'd suggest to have (suspend before parallelism):

Annotations: <none> Suspend: false Parallelism: 1 Completions: 1 Completion Mode: NonIndexed Start Time: Mon, 11 Dec 2023 15:18:09 +0100

or (suspend after completion mode)

Annotations: <none> Parallelism: 1 Completions: 1 Completion Mode: NonIndexed Suspend: false Start Time: Mon, 11 Dec 2023 15:18:09 +0100

soltysh · 2024-01-18T10:04:26Z

staging/src/k8s.io/kubectl/pkg/describe/describe_test.go

@@ -2516,8 +2519,8 @@ func TestDescribeDeployment(t *testing.T) {
 			expects: []string{
 				"Replicas:           2 desired | 1 updated | 3 total | 2 available | 1 unavailable",
 				"Image:        mytest-image:v2.0",
-				"OldReplicaSets:  bar-001 (2/2 replicas created)",
-				"NewReplicaSet:   bar-002 (1/1 replicas created)",
+				"OldReplicaSets:    bar-001 (2/2 replicas created)",


When updating the above, please make sure to also address this one.

* Add Node-Selectors and Tolerations to pod template's describe output. * Add Suspend, BackoffLimit and TTLSecondsAfterFinished to job's describe output.

soltysh

/lgtm
/approve

k8s-ci-robot · 2024-01-23T14:24:02Z

LGTM label has been added.

Git tree hash: 1373f785369cdb636b42ae78fe448f7f99f66b99

k8s-ci-robot · 2024-01-23T14:24:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~staging/src/k8s.io/kubectl/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-triage-robot · 2024-01-23T17:55:47Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 6, 2024

k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 6, 2024

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

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 6, 2024

k8s-ci-robot requested review from brianpursley and seans3 January 6, 2024 00:27

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 6, 2024

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 Jan 6, 2024

kaisoz suggested changes Jan 11, 2024

View reviewed changes

staging/src/k8s.io/kubectl/pkg/describe/describe.go Show resolved Hide resolved

staging/src/k8s.io/kubectl/pkg/describe/describe.go Outdated Show resolved Hide resolved

ivanvc force-pushed the improve-describe-job-output branch from 2106366 to 243b6fc Compare January 11, 2024 18:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 11, 2024

ivanvc force-pushed the improve-describe-job-output branch from 243b6fc to c8fa972 Compare January 11, 2024 19:44

ivanvc requested a review from kaisoz January 11, 2024 22:45

kaisoz suggested changes Jan 17, 2024

View reviewed changes

staging/src/k8s.io/kubectl/pkg/describe/describe_test.go Outdated Show resolved Hide resolved

ivanvc force-pushed the improve-describe-job-output branch from c8fa972 to ff888ee Compare January 17, 2024 21:03

ivanvc requested a review from kaisoz January 17, 2024 21:03

k8s-ci-robot assigned kaisoz Jan 17, 2024

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

mimowo reviewed Jan 18, 2024

View reviewed changes

soltysh requested changes Jan 18, 2024

View reviewed changes

describer: improve job and pod template output

6d65c06

* Add Node-Selectors and Tolerations to pod template's describe output. * Add Suspend, BackoffLimit and TTLSecondsAfterFinished to job's describe output.

ivanvc force-pushed the improve-describe-job-output branch from ff888ee to 6d65c06 Compare January 19, 2024 18:18

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2024

ivanvc requested a review from soltysh January 19, 2024 20:02

soltysh approved these changes Jan 23, 2024

View reviewed changes

k8s-ci-robot assigned soltysh Jan 23, 2024

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

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

k8s-ci-robot merged commit cf2003f into kubernetes:master Jan 23, 2024
14 checks passed

k8s-ci-robot added this to the v1.30 milestone Jan 23, 2024

mimowo mentioned this pull request Feb 9, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

describer: improve job and pod template output #122618

describer: improve job and pod template output #122618

ivanvc commented Jan 6, 2024

linux-foundation-easycla bot commented Jan 6, 2024 •

edited

Loading

k8s-ci-robot commented Jan 6, 2024

k8s-ci-robot commented Jan 6, 2024

HirazawaUi commented Jan 6, 2024

kaisoz commented Jan 17, 2024

k8s-ci-robot commented Jan 17, 2024

mimowo left a comment •

edited

Loading

mimowo Jan 18, 2024

soltysh Jan 18, 2024

ivanvc Jan 18, 2024

mimowo Jan 19, 2024

ivanvc Jan 19, 2024

mimowo Jan 18, 2024

soltysh Jan 18, 2024

soltysh Jan 18, 2024

ivanvc Jan 18, 2024

mimowo Jan 19, 2024 •

edited

Loading

ivanvc Jan 19, 2024 •

edited

Loading

soltysh commented Jan 18, 2024

soltysh left a comment

soltysh Jan 18, 2024

soltysh Jan 18, 2024

soltysh Jan 18, 2024

soltysh left a comment

k8s-ci-robot commented Jan 23, 2024

k8s-ci-robot commented Jan 23, 2024

k8s-triage-robot commented Jan 23, 2024

describer: improve job and pod template output #122618

describer: improve job and pod template output #122618

Conversation

ivanvc commented Jan 6, 2024

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.:

linux-foundation-easycla bot commented Jan 6, 2024 • edited Loading

k8s-ci-robot commented Jan 6, 2024

k8s-ci-robot commented Jan 6, 2024

HirazawaUi commented Jan 6, 2024

kaisoz commented Jan 17, 2024

k8s-ci-robot commented Jan 17, 2024

mimowo left a comment • edited Loading

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

mimowo Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

ivanvc Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

soltysh commented Jan 18, 2024

soltysh left a 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

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 23, 2024

k8s-ci-robot commented Jan 23, 2024

k8s-triage-robot commented Jan 23, 2024

linux-foundation-easycla bot commented Jan 6, 2024 •

edited

Loading

mimowo left a comment •

edited

Loading

mimowo Jan 19, 2024 •

edited

Loading

ivanvc Jan 19, 2024 •

edited

Loading