remove ValidateJobTemplate and add more test cases to batch validation #116052

kannon92 · 2023-02-24T21:09:39Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Remove unused code and add a few missing test cases in validation.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

kannon92 · 2023-02-24T21:09:51Z

/sig apps

pkg/apis/batch/validation/validation_test.go

pkg/apis/batch/validation/validation.go

k8s-triage-robot · 2023-02-27T16:10:18Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

kannon92 · 2023-02-27T17:09:43Z

I'm running into this issue that isn't allowing me to update the generated code.

#116094

@liggitt or @alculquicondor is this a user error or a problem we should address? I'm not sure how to regenerate this code if hack/update-codegen.sh doesn't work.

liggitt · 2023-02-27T17:16:32Z

@liggitt or @alculquicondor is this a user error or a problem we should address? I'm not sure how to regenerate this code if hack/update-codegen.sh doesn't work.

it's likely an issue with your env... that script is tested continuously at HEAD and other users can run it locally

kannon92 · 2023-02-28T14:53:57Z

@liggitt or @alculquicondor is this a user error or a problem we should address? I'm not sure how to regenerate this code if hack/update-codegen.sh doesn't work.

it's likely an issue with your env... that script is tested continuously at HEAD and other users can run it locally

@liggitt Thank you for your help. I am getting past this point.

I am new to updating APIs in Kubernetes. Am I correct in thinking I should do update-codegen.sh and then commit those changes in staging for batch? Not sure of the process for removing code from APIs.

liggitt · 2023-02-28T14:56:42Z

Am I correct in thinking I should do update-codegen.sh and then commit those changes in staging for batch? Not sure of the process for removing code from APIs.

Yes, make update is a good target to run to regenerate after changing API packages. Commit the results to a separate commit and include that in your PR.

Make sure you have fetched latest tags from github (git fetch $upstream_remote_name --tags) before running make update

kannon92 · 2023-02-28T15:58:36Z

/retest

kannon92 · 2023-02-28T16:27:44Z

/retest

cici37 · 2023-02-28T21:14:51Z

/remove-sig api-machinery

kannon92 · 2023-03-01T20:03:04Z

This is ready for review! Sorry for all the test failures. I picked a bad time to upgrade my goenv so I wasn't clear what was local issues or real bugs.

alculquicondor

Technically, the new tests are independent from the rest of the changes. But they are already in a separate commit, so it's ok to leave in the same PR.

alculquicondor · 2023-03-01T20:20:10Z

pkg/apis/batch/validation/validation_test.go

+				UID:       types.UID("1a2b3c"),
+			},
+			Spec: batch.JobSpec{
+				BackoffLimit: &negative,


it looks like this variable already existed, but following the DAMP principle, this should be pointer.Int64(-1)

TIL: Descriptive And Meaningful Phrases

https://stackoverflow.com/questions/6453235/what-does-damp-not-dry-mean-when-talking-about-unit-tests

alculquicondor · 2023-03-01T20:20:41Z

pkg/apis/batch/validation/validation_test.go

+				Type:  field.ErrorTypeInvalid,
+				Field: "spec.completions",
+			},
+			opts: JobValidationOptions{AllowElasticIndexedJobs: true},


does this option matter?

Yea it does. Its not really an important test but this test case is verifying that we can't change completions if we are a NonIndexedJob. I think scaling IndexJobs is gated by AllowElasticIndexedJobs.

https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/batch/validation/validation.go#L570

If I don't have the AllowElasticIndexedJobs set we just assume completions are immutable.

oh, then rename the testcase to: immutable completions for indexed job when AllowElasticIndexedJobs is true

This test case is for non-indexed jobs but I take your point. I'll push up a change.

alculquicondor

/lgtm

pkg/apis/batch/validation/validation_test.go

alculquicondor

/lgtm cancel

liggitt · 2023-03-01T21:37:22Z

staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.JobTemplate.yaml

@@ -1,1202 +0,0 @@
-apiVersion: batch/v1beta1


delete the JobTemplate files under testdata/{v1.25.0,v1.26.0} as well

liggitt · 2023-03-01T21:38:09Z

one comment on other testdata files to drop, then lgtm

kannon92 · 2023-03-01T23:28:52Z

/retest

liggitt · 2023-03-01T23:38:29Z

cc @pohly for leaked goroutine in integration test:

E0301 22:07:04.127341   82193 etcd.go:212] "EtcdMain goroutine check" err=<
	found unexpected goroutines:
	[Goroutine 11523 in state sleep, with time.Sleep on top of the stack:
	goroutine 11523 [sleep]:
	time.Sleep(0x1a13b8600)
		/home/prow/go/src/k8s.io/kubernetes/_output/local/.gimme/versions/go1.20.1.linux.amd64/src/runtime/time.go:195 +0x135
	k8s.io/kubernetes/vendor/k8s.io/utils/clock.RealClock.Sleep(...)
		/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/utils/clock/clock.go:135
	k8s.io/kubernetes/vendor/k8s.io/component-helpers/apimachinery/lease.(*controller).backoffEnsureLease(0xc0083845a0)
		/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-helpers/apimachinery/lease/controller.go:148 +0x95
	k8s.io/kubernetes/vendor/k8s.io/component-helpers/apimachinery/lease.(*controller).sync(0xc0083845a0)
		/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-helpers/apimachinery/lease/controller.go:119 +0xf6
	k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0xc00925dec8?)
		/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:157 +0x3e
	k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xc008b8cf80?, {0x5a3bfe0, 0xc00264ef60}, 0x1, 0xc007a53da0)
		/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:158 +0xb6
	k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil(0x47d105?, 0x2540be400, 0x3fa47ae147ae147b, 0x90?, 0xc004b16600?)
		/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:135 +0x89
	k8s.io/kubernetes/vendor/k8s.io/component-helpers/apimachinery/lease.(*controller).Run(0xc000d6eb40?, 0xc007a53da0?)
		/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-helpers/apimachinery/lease/controller.go:100 +0x57
	created by k8s.io/kubernetes/pkg/controlplane.completedConfig.New.func2
		/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/controlplane/instance.go:512 +0x1d3
	
	]

looks like these started with the new detection https://storage.googleapis.com/k8s-triage/index.html?pr=1&job=integration&test=TestMain

liggitt · 2023-03-02T00:44:33Z

/lgtm
/approve

k8s-ci-robot · 2023-03-02T00:44:41Z

LGTM label has been added.

Git tree hash: 52ab827da883cdb9d01e991d834d402adbcc947e

k8s-ci-robot · 2023-03-02T00:45:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kannon92, liggitt

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/apis/OWNERS~~ [liggitt]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]

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

fedebongio · 2023-03-07T21:10:15Z

/triage accepted

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 24, 2023

k8s-ci-robot requested review from alculquicondor and liggitt February 24, 2023 21:10

kannon92 commented Feb 24, 2023

View reviewed changes

pkg/apis/batch/validation/validation_test.go Outdated Show resolved Hide resolved

kannon92 force-pushed the validation-coverage-and-cleanup branch from af5e32a to 9a684fa Compare February 24, 2023 21:13

kannon92 commented Feb 24, 2023

View reviewed changes

pkg/apis/batch/validation/validation.go Show resolved Hide resolved

kannon92 changed the title ~~remove ValidateJobSpec and add more test cases to batch validation~~ remove ValidateJobTemplate and add more test cases to batch validation Feb 24, 2023

kannon92 force-pushed the validation-coverage-and-cleanup branch from 9a684fa to 584db3d Compare February 27, 2023 15:38

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 27, 2023

kannon92 force-pushed the validation-coverage-and-cleanup branch from 584db3d to 49e0517 Compare February 28, 2023 14:59

k8s-ci-robot added area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Feb 28, 2023

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 28, 2023

alculquicondor reviewed Mar 1, 2023

View reviewed changes

kannon92 added 2 commits March 1, 2023 20:39

remove ValidateJobSpec and add more test cases to batch validation

2da3e83

update batch apis

cb9334e

kannon92 force-pushed the validation-coverage-and-cleanup branch from 29ad2f0 to fb0b9c2 Compare March 1, 2023 20:40

alculquicondor reviewed Mar 1, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Mar 1, 2023

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

alculquicondor reviewed Mar 1, 2023

View reviewed changes

pkg/apis/batch/validation/validation_test.go Outdated Show resolved Hide resolved

alculquicondor reviewed Mar 1, 2023

View reviewed changes

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

k8s-ci-robot requested a review from alculquicondor March 1, 2023 21:24

liggitt reviewed Mar 1, 2023

View reviewed changes

generated code

3489ace

kannon92 force-pushed the validation-coverage-and-cleanup branch from 1e694b3 to 3489ace Compare March 1, 2023 21:43

liggitt mentioned this pull request Mar 2, 2023

[Flake] goroutine leak detection flaking #116196

Closed

k8s-ci-robot assigned liggitt Mar 2, 2023

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

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

k8s-ci-robot merged commit ddb0d06 into kubernetes:master Mar 2, 2023

k8s-ci-robot added this to the v1.27 milestone Mar 2, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove ValidateJobTemplate and add more test cases to batch validation #116052

remove ValidateJobTemplate and add more test cases to batch validation #116052

kannon92 commented Feb 24, 2023

kannon92 commented Feb 24, 2023

k8s-triage-robot commented Feb 27, 2023

kannon92 commented Feb 27, 2023

liggitt commented Feb 27, 2023

kannon92 commented Feb 28, 2023

liggitt commented Feb 28, 2023

kannon92 commented Feb 28, 2023

kannon92 commented Feb 28, 2023

cici37 commented Feb 28, 2023

kannon92 commented Mar 1, 2023

alculquicondor left a comment •

edited

Loading

alculquicondor Mar 1, 2023

kannon92 Mar 1, 2023

kannon92 Mar 1, 2023

alculquicondor Mar 1, 2023

alculquicondor Mar 1, 2023

kannon92 Mar 1, 2023

kannon92 Mar 1, 2023

alculquicondor Mar 1, 2023

kannon92 Mar 1, 2023

alculquicondor left a comment

alculquicondor left a comment

liggitt Mar 1, 2023

liggitt commented Mar 1, 2023

kannon92 commented Mar 1, 2023

liggitt commented Mar 1, 2023 •

edited

Loading

liggitt commented Mar 2, 2023

k8s-ci-robot commented Mar 2, 2023

k8s-ci-robot commented Mar 2, 2023

fedebongio commented Mar 7, 2023

remove ValidateJobTemplate and add more test cases to batch validation #116052

remove ValidateJobTemplate and add more test cases to batch validation #116052

Conversation

kannon92 commented Feb 24, 2023

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

kannon92 commented Feb 24, 2023

k8s-triage-robot commented Feb 27, 2023

kannon92 commented Feb 27, 2023

liggitt commented Feb 27, 2023

kannon92 commented Feb 28, 2023

liggitt commented Feb 28, 2023

kannon92 commented Feb 28, 2023

kannon92 commented Feb 28, 2023

cici37 commented Feb 28, 2023

kannon92 commented Mar 1, 2023

alculquicondor 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

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Mar 1, 2023

kannon92 commented Mar 1, 2023

liggitt commented Mar 1, 2023 • edited Loading

liggitt commented Mar 2, 2023

k8s-ci-robot commented Mar 2, 2023

k8s-ci-robot commented Mar 2, 2023

fedebongio commented Mar 7, 2023

alculquicondor left a comment •

edited

Loading

liggitt commented Mar 1, 2023 •

edited

Loading