Fix deployment generator after introducing deployments in apps/v1beta1 by soltysh · Pull Request #42362 · kubernetes/kubernetes

soltysh · 2017-03-01T21:41:10Z

This PR does two things:

Switches all generator to produce versioned objects, to bypass the problem of having an object in multiple versions, which then results in not having stable generator (iow. producing exactly the same object).
Introduces new generator for apps/v1beta1 deployments.

@kubernetes/sig-apps-pr-reviews @kubernetes/sig-cli-pr-reviews ptal

This is a followup to #39683, so I'm adding 1.6 milestone.

Introduce new generator for apps/v1beta1 deployments

calebamiles · 2017-03-01T21:58:57Z

Added do-not-merge label for new feature. @soltysh is this PR a bug fix or a new feature? We are currently in code slush and new features should follow the exception process. The previous expectation from the 1.5 release was to approve a singular exception which should have covered PRs which were currently in flight. I'm a bit worried about these large changes coming in so late in the release process but maybe someone from @kubernetes/release-team will have other opinions.

cc: @ethernetdan

janetkuo · 2017-03-01T22:10:22Z

We need to handle version skew using discovery. We already have the discovery code in cmd/run.go. So if the server doesn't support apps/v1beta1, we fall back to extensions/v1beta1, and manually change default values to match the behavior in apps/v1beta1.

Please also update the PR title to make it more descriptive.

janetkuo · 2017-03-01T22:12:48Z

@calebamiles this is more of a bug fix because kubectl run default behavior is changed (after #39683 is merged), and we need this so that we don't break users

calebamiles · 2017-03-01T22:54:38Z

@janetkuo, thank you for your description but I feel like I'm even more confused about why #39683 was merged. @ethernetdan, my gut instinct would be to revert #39683 and deal with that PR and this one during the 1.7 release cycle Thoughts, @ethernetdan?

cc" @kubernetes/release-team

ethernetdan · 2017-03-01T23:01:34Z

@calebamiles I think I have to agree - the original PR did not follow the exception process and should not have been merged. Given the criteria we have been applying to other exceptions I don't think if it would've been approved.

I'd say let's put a revert PR in to see how cleanly it can be removed and we can discuss the change there. How does that sound?

smarterclayton · 2017-03-01T23:10:53Z

Deployments not going to beta in 1.6 is pretty significant - id prefer that @soltysh start the exception process now and have the discussion there.

calebamiles · 2017-03-01T23:20:48Z

Revert PR: #42368

k8s-reviewable · 2017-03-02T05:47:36Z

This change is

soltysh · 2017-03-02T12:36:43Z

@smarterclayton @ethernetdan @calebamiles I've emailed about this and CronJobs exception Monday afternoon (CET) and have not heard back since than other than initial approval from @idvoretskyi and a few +1 on it. I assumed this was enough to proceed with this.

0xmichalis · 2017-03-02T12:50:04Z

Worth noting that the PR has been ready for review for two weeks and the change has been discussed in sig-apps even longer before the PR was opened (more than a month).

soltysh · 2017-03-02T13:51:45Z

@Kargakis @janetkuo hopefully all comments addressed. I've added additional cmd test for the new generator and updated the title. ptal

0xmichalis · 2017-03-02T13:57:07Z

 		switch restartPolicy {
 		case api.RestartPolicyAlways:
-			if contains(resourcesList, v1beta1.SchemeGroupVersion.WithResource("deployments")) {
+			if contains(resourcesList, appsv1beta1.SchemeGroupVersion.WithResource("deployments")) {


you got these backwards?

OMG, I need a ☕️

soltysh · 2017-03-02T14:07:01Z

@Kargakis should be fixed.

0xmichalis · 2017-03-02T14:48:35Z

TestGenerate needs to be fixed

janetkuo · 2017-03-08T21:30:45Z

Just chatted with @bgrant0607 offline about this PR and #42392. In conclusion, we think we should:

Make extensions/v1betat1 the preferred deployment version instead of apps/v1beta1 (but don't revert Add apps/v1beta1 deployments with new defaults #39683)
Make kubectl run and kubectl create use the old extensions/v1beta1 deployment generator by default
New kubectl with apps/v1beta1 generator should work on old clusters (fall back to extensions/v1beta1 and apply new defaults)

More details about the discussions with @bgrant0607:

We agree that new defaults is a big UX improvement, and it might be okay to change the default values of deployment in kubectl run and kubectl create. However, we don't want to rush it at this moment. Let's wait for user feedback first.

As a side note, this behavior change problem could have been solved if we allow users to specify their preference for kubectl. For example, in each new release, kubectl will tell the users "hey behavior X is changed, do you want to keep the old or use the new behavior?", and users' answer will be kept in some preference file.

soltysh · 2017-03-09T08:44:59Z

Make extensions/v1betat1 the preferred deployment version instead of apps/v1beta1 (but don't revert #39683)

That depends on what you mean preferred. If by preferred you mean that kubectl get deployment will reach out to extensions/v1beta1 that's not possible at the moment. The discovery mechanics do not allow us to specify preferred version for resource cohabiting in multiple groups (@kubernetes/sig-api-machinery-misc for confirmation). AFAIK it will pick one based on alphabetical order, so in this case apps/v1beta1 is the preferred one.
Having said that, both deployments are sharing single storage and data available under both API endpoints are the same (except for defaults when creating new deployments) thus it doesn't matter which endpoint you're hitting, as long as we're using kubectl 1.6 (or newer) which will know from discovery what is available. So the only worry is the next point, which is already addressed in this PR.

Make kubectl run and kubectl create use the old extensions/v1beta1 deployment generator by default

Done. One can explicitly use new generator for both kubectl run and kubectl create deployment specifying --generator flag. Appropriate test are in hack/make-rules/test-cmd-util.sh.

New kubectl with apps/v1beta1 generator should work on old clusters (fall back to extensions/v1beta1 and apply new defaults)

That I will object to, this will only complicate the code and confuse users. Only posting a deployment object to apps/v1beta1 will actually apply new defaults on server (!), no defaults are applied on client and this should stay as is. It's perfectly fine and already done to fallback to old generator for older clusters.

As a side note, this behavior change problem could have been solved if we allow users to specify their preference for kubectl. For example, in each new release, kubectl will tell the users "hey behavior X is changed, do you want to keep the old or use the new behavior?", and users' answer will be kept in some preference file.

This is awesome idea! I've created #42787 to discuss this further.

0xmichalis · 2017-03-09T11:44:08Z

If we are ok with taking the kubectl compatibility breakage while relying on users handcrafting configs in order to take advantage of the new defaults, then I won't block on this anymore. Changes LGTM.

bgrant0607 · 2017-03-09T17:00:08Z

The issue I'm using for preferences is #10693

janetkuo · 2017-03-09T19:07:52Z

That I will object to, this will only complicate the code and confuse users. Only posting a deployment object to apps/v1beta1 will actually apply new defaults on server (!), no defaults are applied on client and this should stay as is. It's perfectly fine and already done to fallback to old generator for older clusters.

OK, maybe print some warning message like "generator foo isn't supported by server, falling back to bar", for this new client + old server case?

soltysh · 2017-03-09T20:10:34Z

OK, maybe print some warning message like "generator foo isn't supported by server, falling back to bar", for this new client + old server case?

That's already done in this PR.

soltysh · 2017-03-09T20:12:44Z

See this and this for the warnings.

ethernetdan · 2017-03-09T20:22:45Z

@soltysh, @janetkuo, @bgrant0607: would it be possible to make a decision on this soon?

Currently this issue is breaking skew testing and blurring signal on upgrades.

cc/ @skriss

soltysh · 2017-03-09T20:24:03Z

@ethernetdan this is done. Based on what @janetkuo said this is ready for merge. Just needs her lgtm and approval for merge.

janetkuo

Only a few nits, LGTM otherwise

janetkuo · 2017-03-09T21:34:12Z

+	// fallback to the old generator if server does not support apps/v1beta deployments
+	if generatorName == cmdutil.DeploymentBasicAppsV1Beta1GeneratorName &&
+		!contains(resourcesList, appsv1beta1.SchemeGroupVersion.WithResource("deployments")) {
+		fmt.Fprintf(cmdErr, "New deployments generator specified, but apps/v1beta1.Deployments are not available, falling back to the old one.\n")


Some suggestions:

add generator names in the error message (i.e. cmdutil.DeploymentBasicV1Beta1GeneratorName and cmdutil.DeploymentBasicAppsV1Beta1GeneratorName)

add "WARNING: " to the beginning of the error message

janetkuo · 2017-03-09T21:34:45Z

+		return fmt.Errorf("failed to discover supported resources: %v", err)
+	}
+	generatorName := cmdutil.GetFlagString(cmd, "generator")
+	// fallback to the old generator if server does not support apps/v1beta deployments


nit: v1beta1

janetkuo · 2017-03-09T21:57:53Z

-			if contains(resourcesList, v1beta1.SchemeGroupVersion.WithResource("deployments")) {
-				generatorName = "deployment/v1beta1"
+			// TODO: we need to deprecate this along with extensions/v1beta1.Deployments
+			// in favor of the new generator for apps/v1beta1.Deployments


I assume we'll deprecate it by updating generator docs?

Or should we print deprecation message whenever extensions/v1beta1 generator is used (it'd be a bit too noisy)?

We'll update the generator docs, which btw. needs updating when this merges. That's what we did when removing extensions/v1beta1.Jobs.

janetkuo · 2017-03-09T21:59:25Z

 	schedule := cmdutil.GetFlagString(cmd, "schedule")
 	if len(schedule) != 0 && len(generatorName) == 0 {
-		generatorName = "cronjob/v2alpha1"
+		generatorName = cmdutil.CronJobV2Alpha1GeneratorName


We should check if cronjob is supported as well. But this can be fixed in a follow-up.

janetkuo · 2017-03-09T22:05:12Z

+	// TODO: this should be removed alongside with extensions/v1beta1 depployments generator
+	if generatorName == cmdutil.DeploymentAppsV1Beta1GeneratorName &&
+		!contains(resourcesList, appsv1beta1.SchemeGroupVersion.WithResource("deployments")) {
+		fmt.Fprintf(cmdErr, "New deployments generator specified, but apps/v1beta1.Deployments are not available, falling back to the old one.\n")


Same as https://github.com/kubernetes/kubernetes/pull/42362/files#r105276435

janetkuo · 2017-03-09T22:12:59Z

 	return result, nil
 }

+// populateResourceListV1 takes strings of form <resourceName1>=<value1>,<resourceName1>=<value2>


nit: document what's returned

…available

soltysh · 2017-03-10T11:09:06Z

@janetkuo I've addressed the nits, since I'm at it I've also added the check for cronjob, ptal

k8s-ci-robot · 2017-03-10T11:52:46Z

@soltysh: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GKE smoke e2e	d50d2078a25a4fb0232b8875e2d5306e4f9b7174	link	`@k8s-bot cvm gke e2e test this`
Jenkins GCI GKE smoke e2e	d50d2078a25a4fb0232b8875e2d5306e4f9b7174	link	`@k8s-bot gci gke e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

soltysh · 2017-03-10T12:00:11Z

@k8s-bot non-cri e2e test this

janetkuo · 2017-03-10T20:03:11Z

/approve
/lgtm

deads2k · 2017-03-10T20:10:01Z

/approve

k8s-github-robot · 2017-03-10T20:10:25Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: deads2k, janetkuo, soltysh

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

~~hack/OWNERS~~ [deads2k]
~~pkg/kubectl/OWNERS~~ [deads2k,janetkuo]

We suggest the following people:
cc
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-03-10T22:01:21Z

Automatic merge from submit-queue (batch tested with PRs 38805, 42362, 42862)

janetkuo · 2017-03-22T17:56:45Z

Hey @soltysh will you prepare the doc for the new generator?

soltysh added area/workload-api/deployment release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Mar 1, 2017

soltysh added this to the v1.6 milestone Mar 1, 2017

soltysh assigned 0xmichalis and janetkuo Mar 1, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 1, 2017

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 1, 2017

calebamiles added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 1, 2017

calebamiles mentioned this pull request Mar 1, 2017

Revert "Merge pull request #39683 from soltysh/move_deployments" #42368

Closed

soltysh changed the title ~~Deployment generators~~ Fix deployment generator after introducing deployments in apps/v1beta1 Mar 2, 2017

soltysh force-pushed the deployment_generators branch from 7b42b8b to c426c8f Compare March 2, 2017 13:51

0xmichalis reviewed Mar 2, 2017

View reviewed changes

soltysh force-pushed the deployment_generators branch from c426c8f to d1944ff Compare March 2, 2017 14:06

soltysh force-pushed the deployment_generators branch from d1944ff to d50d207 Compare March 2, 2017 14:51

k8s-github-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 Mar 2, 2017

soltysh mentioned this pull request Mar 9, 2017

kubectl should suggest using newer options #42787

Closed

janetkuo reviewed Mar 9, 2017

View reviewed changes

soltysh added 3 commits March 10, 2017 12:08

Switch generators to use versioned objects

1049dad

Introduce new generator for apps/v1beta1 deployments

aa43907

Error out when cronjob generator not specified, but cronjobs are not …

597a359

…available

soltysh force-pushed the deployment_generators branch from 7cfc63a to 597a359 Compare March 10, 2017 11:08

0xmichalis mentioned this pull request Mar 10, 2017

Deployment upgrade test failed to create new replica set #42738

Closed

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

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

mengqiy mentioned this pull request Mar 10, 2017

Deployment version skew are failing #42912

Closed

k8s-github-robot merged commit d2d3884 into kubernetes:master Mar 10, 2017

soltysh deleted the deployment_generators branch March 10, 2017 22:04

janetkuo mentioned this pull request Mar 21, 2018

Migrate kubectl to use apps/v1 instead of extensions/v1beta1 #61430

Closed

5 tasks

Conversation

soltysh commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calebamiles commented Mar 1, 2017

Uh oh!

janetkuo commented Mar 1, 2017

Uh oh!

janetkuo commented Mar 1, 2017

Uh oh!

calebamiles commented Mar 1, 2017

Uh oh!

ethernetdan commented Mar 1, 2017

Uh oh!

smarterclayton commented Mar 1, 2017

Uh oh!

calebamiles commented Mar 1, 2017

Uh oh!

k8s-reviewable commented Mar 2, 2017

Uh oh!

soltysh commented Mar 2, 2017

Uh oh!

0xmichalis commented Mar 2, 2017

Uh oh!

soltysh commented Mar 2, 2017

Uh oh!

0xmichalis Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh commented Mar 2, 2017

Uh oh!

0xmichalis commented Mar 2, 2017

Uh oh!

janetkuo commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soltysh commented Mar 9, 2017

Uh oh!

0xmichalis commented Mar 9, 2017

Uh oh!

bgrant0607 commented Mar 9, 2017

Uh oh!

janetkuo commented Mar 9, 2017

Uh oh!

soltysh commented Mar 9, 2017

Uh oh!

soltysh commented Mar 9, 2017

Uh oh!

ethernetdan commented Mar 9, 2017

Uh oh!

soltysh commented Mar 9, 2017

Uh oh!

janetkuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janetkuo Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh commented Mar 10, 2017

Uh oh!

k8s-ci-robot commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

soltysh commented Mar 1, 2017 •

edited

Loading

0xmichalis Mar 2, 2017 •

edited

Loading

janetkuo commented Mar 8, 2017 •

edited

Loading

janetkuo Mar 9, 2017 •

edited

Loading

k8s-ci-robot commented Mar 10, 2017 •

edited

Loading