Enable batch/v1beta1.CronJobs by default #51465

soltysh · 2017-08-28T17:20:01Z

This PR moves to CronJobs beta entirely, enabling batch/v1beta1 by default.

Related issue: #41039

Promote CronJobs to batch/v1beta1.

soltysh · 2017-08-28T17:21:30Z

@deads2k for approval

janetkuo · 2017-08-28T17:44:54Z

cmd/kube-controller-manager/app/batch.go

 	go cronjob.NewCronJobController(
-		clientset.NewForConfigOrDie(cronjobConfig),
+		// clientset.NewForConfigOrDie(cronjobConfig),


Remove this line

janetkuo · 2017-08-28T17:46:45Z

cmd/kube-controller-manager/app/batch.go

@@ -41,14 +39,13 @@ func startJobController(ctx ControllerContext) (bool, error) {
 }

 func startCronJobController(ctx ControllerContext) (bool, error) {
-	if !ctx.AvailableResources[schema.GroupVersionResource{Group: "batch", Version: "v2alpha1", Resource: "cronjobs"}] {
+	if !ctx.AvailableResources[schema.GroupVersionResource{Group: "batch", Version: "v2alpha1", Resource: "cronjobs"}] &&
+		!ctx.AvailableResources[schema.GroupVersionResource{Group: "batch", Version: "v1beta1", Resource: "cronjobs"}] {


batch/v1beta1 cronjob clientsets are used in cronjob controller code, so batch/v1beta1 has to be enabled?

Yeah, I initially thought that maybe be possible to work with both, but it's not. I forgot to update it. Will do.

janetkuo · 2017-08-28T17:52:52Z

cmd/kube-apiserver/app/server.go

@@ -549,8 +548,7 @@ func BuildStorageFactory(s *options.ServerRunOptions) (*serverstorage.DefaultSto
 		s.Etcd.StorageConfig, s.Etcd.DefaultStorageMediaType, api.Codecs,
 		serverstorage.NewDefaultResourceEncodingConfig(api.Registry), storageGroupsToEncodingVersion,
 		// FIXME: this GroupVersionResource override should be configurable
-		// TODO we need to update this to batch/v1beta1 when it's enabled by default
-		[]schema.GroupVersionResource{batch.Resource("cronjobs").WithVersion("v2alpha1")},
+		[]schema.GroupVersionResource{batch.Resource("cronjobs").WithVersion("v1beta1")},


What does this line do?

This makes sure CronJob is stored with batch/v1beta1. If this is not present current mechanics will try to find codec for CronJob in batch/v1, which will fail. This needs to be fixed, I've spoked with @deads2k about it earlier today, but that won't happen in 1.8 timeframe, I guess.

Does this change default storage version from batch/v2alpha1 to batch/v1beta1?

deads2k · 2017-08-28T18:29:36Z

cmd/kube-controller-manager/app/batch.go

@@ -41,14 +39,13 @@ func startJobController(ctx ControllerContext) (bool, error) {
 }

 func startCronJobController(ctx ControllerContext) (bool, error) {
-	if !ctx.AvailableResources[schema.GroupVersionResource{Group: "batch", Version: "v2alpha1", Resource: "cronjobs"}] {
+	if !ctx.AvailableResources[schema.GroupVersionResource{Group: "batch", Version: "v2alpha1", Resource: "cronjobs"}] &&


Given client-go, it's basically impossible for a controller to tolerate two different API levels. Are you sure this is doing what you expect?

Given client-go, it's basically impossible for a controller to tolerate two different API levels. Are you sure this is doing what you expect?

Yeah, if your controller is only going to work on the beta version (what it looks like), then you should only list the beta version as a gate. Things don't go sideways until you switch to GA.

Yup, I initially thought about working with both APIs, but failed. I forgot to update it.

deads2k · 2017-08-28T18:32:18Z

@lavalamp you should probably take a look at what's happening with these now. The effects of lacking a hub version are pretty ugly.

soltysh · 2017-08-28T19:36:33Z

I've addressed comments.

janetkuo · 2017-08-28T21:17:54Z

pkg/controller/cronjob/cronjob_controller.go

@@ -116,7 +116,7 @@ func (jm *CronJobController) syncAll() {
 	js := jl.Items
 	glog.V(4).Infof("Found %d jobs", len(js))

-	sjl, err := jm.kubeClient.BatchV2alpha1().CronJobs(metav1.NamespaceAll).List(metav1.ListOptions{})
+	sjl, err := jm.kubeClient.BatchV1beta1().CronJobs(metav1.NamespaceAll).List(metav1.ListOptions{})


Will this break HA clusters during upgrades? Not all masters are updated simultaneously: #51049 (comment)

I think this is slightly different usecase than what you're pointing to. There, the issue is about upgrading from beta1 to beta2, where both are enabled by default. Here, we are migrating from alpha to beta, iow. from non-default to a default enabled API.
If I don't change controller to use the beta (enabled by default) cronjobs will not be working. Because you'll have the beta API available, but controller will rely on non-default API to be turned on.
This will break HA clusters, but only those that explicitly enable the alpha version. I'll defer to @bgrant0607 or @smarterclayton for the decision but imho this should get in.

We don't support HA alpha upgrading to beta without interruption. So someone on a cluster like that has already broken the glass.

soltysh · 2017-08-29T11:42:52Z

/test pull-kubernetes-e2e-gce-bazel

janetkuo · 2017-08-29T17:17:09Z

/lgtm

smarterclayton · 2017-08-29T17:30:01Z

Is there an issue in the features repo tracking this?

janetkuo · 2017-08-29T18:31:51Z

Is there an issue in the features repo tracking this?

kubernetes/enhancements#19?

janetkuo · 2017-08-29T20:20:17Z

/retest

janetkuo · 2017-08-29T22:06:46Z

/retest

smarterclayton · 2017-08-29T23:31:42Z

/approve

…

On Tue, Aug 29, 2017 at 6:49 PM, k8s-ci-robot ***@***.***> wrote: @soltysh <https://github.com/soltysh>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-bazel 2de214b <2de214b> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/51465/pull-kubernetes-e2e-gce-bazel/13242/> /test pull-kubernetes-e2e-gce-bazel Full PR test history <https://k8s-gubernator.appspot.com/pr/51465>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/soltysh>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51465 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p8b0ttyUzhXkD4PTWnddZiluyxDdks5sdJVlgaJpZM4PEzss> .

k8s-github-robot · 2017-08-29T23:32:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: janetkuo, smarterclayton, soltysh

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-apiserver/OWNERS~~ [smarterclayton]
~~cmd/kube-controller-manager/OWNERS~~ [smarterclayton]
~~hack/OWNERS~~ [smarterclayton]
~~pkg/apis/OWNERS~~ [smarterclayton]
~~pkg/controller/cronjob/OWNERS~~ [janetkuo,smarterclayton,soltysh]
~~pkg/kubectl/OWNERS~~ [janetkuo,smarterclayton]
~~pkg/master/OWNERS~~ [smarterclayton]
~~pkg/printers/OWNERS~~ [janetkuo,smarterclayton]
~~test/OWNERS~~ [janetkuo,smarterclayton,soltysh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

janetkuo · 2017-08-30T00:44:39Z

/retest

janetkuo · 2017-08-30T00:46:55Z

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

@soltysh would you add the issue number to the PR body so that it can get approved?

soltysh · 2017-08-30T11:17:52Z

@soltysh would you add the issue number to the PR body so that it can get approved?

Done.

dims · 2017-08-30T13:59:26Z

/retest

k8s-github-robot · 2017-08-30T22:14:31Z

Automatic merge from submit-queue (batch tested with PRs 50775, 51397, 51168, 51465, 51536)

shyamjvs · 2017-08-31T11:27:34Z

This PR is breaking presubmit e2e's due to the group version not being updated at a few places.
#51692

justinsb · 2017-08-31T14:02:13Z

If this PR breaks/impacts HA clusters, can we expand the release note to be a little more instructive please? cc @jdumars

soltysh · 2017-08-31T20:05:14Z

If this PR breaks/impacts HA clusters, can we expand the release note to be a little more instructive please? cc @jdumars

I'll sync with @kubernetes/sig-docs-maintainers about it.

jdumars · 2017-09-06T19:35:49Z

Which SIG should this be assigned to?

soltysh · 2017-09-08T10:51:20Z

@jdumars I'm currently updating the release notes, I'll ping you for a double check. It's sig-apps

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2017

soltysh assigned janetkuo Aug 28, 2017

soltysh mentioned this pull request Aug 28, 2017

Migrate CronJobs to beta #41039

Closed

3 tasks

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 28, 2017

janetkuo reviewed Aug 28, 2017

View reviewed changes

deads2k reviewed Aug 28, 2017

View reviewed changes

soltysh force-pushed the cronjob_beta branch from 9731c61 to 66784be Compare August 28, 2017 19:36

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 28, 2017

janetkuo added this to the v1.8 milestone Aug 28, 2017

janetkuo reviewed Aug 28, 2017

View reviewed changes

Enable batch/v1beta1.CronJobs by default

2de214b

soltysh force-pushed the cronjob_beta branch from 66784be to 2de214b Compare August 29, 2017 10:25

janetkuo approved these changes Aug 29, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2017

k8s-github-robot merged commit ffcd6d1 into kubernetes:master Aug 30, 2017

soltysh deleted the cronjob_beta branch August 31, 2017 09:07

shyamjvs mentioned this pull request Aug 31, 2017

[SQ-blocking] kops-aws presubmit failing continuously due to cronjob e2e test #51692

Closed

shyamjvs mentioned this pull request Aug 31, 2017

Correct CronJob group version at remaining places #51697

Closed

justinsb mentioned this pull request Aug 31, 2017

Temporarily make kops-aws presubmit non-blocking until it's made useful kubernetes/test-infra#4293

Merged

apelisse mentioned this pull request Aug 31, 2017

Revert "Enable batch/v1beta1.CronJobs by default" #51720

Merged

soltysh added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Sep 8, 2017

soltysh mentioned this pull request Sep 8, 2017

Add batch-related release notes kubernetes/enhancements#422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable batch/v1beta1.CronJobs by default #51465

Enable batch/v1beta1.CronJobs by default #51465

soltysh commented Aug 28, 2017 •

edited

soltysh commented Aug 28, 2017

janetkuo Aug 28, 2017

janetkuo Aug 28, 2017

soltysh Aug 28, 2017

janetkuo Aug 28, 2017

soltysh Aug 28, 2017

janetkuo Aug 28, 2017

deads2k Aug 28, 2017

deads2k Aug 28, 2017

soltysh Aug 28, 2017

deads2k commented Aug 28, 2017

soltysh commented Aug 28, 2017

janetkuo Aug 28, 2017

soltysh Aug 29, 2017

smarterclayton Aug 29, 2017

soltysh commented Aug 29, 2017

janetkuo commented Aug 29, 2017

smarterclayton commented Aug 29, 2017

janetkuo commented Aug 29, 2017

janetkuo commented Aug 29, 2017

janetkuo commented Aug 29, 2017

smarterclayton commented Aug 29, 2017 via email

k8s-github-robot commented Aug 29, 2017

janetkuo commented Aug 30, 2017

janetkuo commented Aug 30, 2017

soltysh commented Aug 30, 2017

dims commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

shyamjvs commented Aug 31, 2017

justinsb commented Aug 31, 2017

soltysh commented Aug 31, 2017

jdumars commented Sep 6, 2017

soltysh commented Sep 8, 2017

Enable batch/v1beta1.CronJobs by default #51465

Enable batch/v1beta1.CronJobs by default #51465

Conversation

soltysh commented Aug 28, 2017 • edited

soltysh commented Aug 28, 2017

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

deads2k commented Aug 28, 2017

soltysh commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Aug 29, 2017

janetkuo commented Aug 29, 2017

smarterclayton commented Aug 29, 2017

janetkuo commented Aug 29, 2017

janetkuo commented Aug 29, 2017

janetkuo commented Aug 29, 2017

smarterclayton commented Aug 29, 2017 via email

k8s-github-robot commented Aug 29, 2017

janetkuo commented Aug 30, 2017

janetkuo commented Aug 30, 2017

soltysh commented Aug 30, 2017

dims commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

shyamjvs commented Aug 31, 2017

justinsb commented Aug 31, 2017

soltysh commented Aug 31, 2017

jdumars commented Sep 6, 2017

soltysh commented Sep 8, 2017

soltysh commented Aug 28, 2017 •

edited