New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generators for apps/v1 deployments #61288

Merged
merged 1 commit into from Apr 12, 2018

Conversation

@ayushpateria
Contributor

ayushpateria commented Mar 16, 2018

What this PR does / why we need it:

  • Adds basic generator DeploymentBasicAppsGeneratorV1 that is used on running kubectl create deployment.
  • Changes default basic generator for create deployment to deployment-basic/apps.v1.
  • Don't fall back in case generator is not available for create deployment, instead, return an error.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #61205

Special notes for your reviewer:

Release note:

Add generators for `apps/v1` deployments.
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 16, 2018

@ayushpateria: GitHub didn't allow me to assign the following users: liggit.

Note that only kubernetes members and repo collaborators can be assigned.

In response to this:

/assign @liggit

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.

@ayushpateria

This comment has been minimized.

Contributor

ayushpateria commented Mar 16, 2018

/assign @liggitt

} else {
generatorName = cmdutil.RunV1GeneratorName
}
generatorName = cmdutil.DeploymentAppsV1GeneratorName

This comment has been minimized.

@ayushpateria

ayushpateria Mar 16, 2018

Contributor

I've removed the discovery validation here and moved it to FallbackGeneratorNameIfNecessary.

}
if !hasResource {
warning(cmdErr, DeploymentAppsV1GeneratorName, DeploymentAppsV1Beta1GeneratorName)
return FallbackGeneratorNameIfNecessary(DeploymentAppsV1Beta1GeneratorName, discoveryClient, cmdErr)

This comment has been minimized.

@ayushpateria

ayushpateria Mar 16, 2018

Contributor

This recursively checks if the older generator can be used. The order is apps/v1 -> apps/v1beta1 -> extensions/v1beta -> v1 ReplicationController

This comment has been minimized.

@liggitt

liggitt Mar 21, 2018

Member

shouldn't you wait to print the warning until you know what version you're going to use?

This comment has been minimized.

@soltysh

soltysh Mar 26, 2018

Contributor

Wouldn't be cleaner to just return the generator name if hasResource and use fallthrough otherwise, printing the warning when generatorName != <what we're about to return to user> just like Jordan suggested?

This comment has been minimized.

@ayushpateria

ayushpateria Mar 26, 2018

Contributor

Agreed. Will update this.

}
if !hasResource {
warning(cmdErr, DeploymentAppsV1GeneratorName, DeploymentAppsV1Beta1GeneratorName)
return FallbackGeneratorNameIfNecessary(DeploymentAppsV1Beta1GeneratorName, discoveryClient, cmdErr)

This comment has been minimized.

@soltysh

soltysh Mar 26, 2018

Contributor

Wouldn't be cleaner to just return the generator name if hasResource and use fallthrough otherwise, printing the warning when generatorName != <what we're about to return to user> just like Jordan suggested?

@@ -209,6 +210,94 @@ func (DeploymentAppsV1Beta1) Generate(genericParams map[string]interface{}) (run
return &deployment, nil
}
type DeploymentAppsV1 struct{}

This comment has been minimized.

@soltysh

soltysh Mar 26, 2018

Contributor

During one of our previous sig-cli calls we discussed the chance of deprecating kubectl run so that in the long run it'll only run a pod and nothing else. kubectl create <resource> will be the path to go. So I'd prefer not to add anything new to run, the create part is ok.

@soltysh

This comment has been minimized.

Contributor

soltysh commented Mar 26, 2018

@ayushpateria and please squash your changes into a single commit.
/ok-to-test

@@ -121,94 +122,6 @@ func (DeploymentV1Beta1) Generate(genericParams map[string]interface{}) (runtime
return &deployment, nil
}
type DeploymentAppsV1Beta1 struct{}

This comment has been minimized.

@ayushpateria

ayushpateria Mar 30, 2018

Contributor

Removing DeploymentAppsV1Beta1 because it's not being used anywhere, and we have plans to deprecate kubectl run.

This comment has been minimized.

@liggitt

liggitt Mar 30, 2018

Member

how do you know it's not being used anywhere? it's usable by specifying the generator name to the run command, right?

This comment has been minimized.

@ayushpateria

ayushpateria Mar 30, 2018

Contributor

My bad, will revert this.

This comment has been minimized.

@liggitt

liggitt Mar 30, 2018

Member

there's no DeploymentAppV1Beta1

that's because you removed it in this commit :)

https://github.com/kubernetes/kubernetes/pull/61288/files?w=1#diff-61ef000e48b9358fadec0877c58832fbL557

it is used in released versions:

case "run":
generator = map[string]kubectl.Generator{
RunV1GeneratorName: kubectl.BasicReplicationController{},
RunPodV1GeneratorName: kubectl.BasicPod{},
DeploymentV1Beta1GeneratorName: kubectl.DeploymentV1Beta1{},
DeploymentAppsV1Beta1GeneratorName: kubectl.DeploymentAppsV1Beta1{},
JobV1GeneratorName: kubectl.JobV1{},
CronJobV2Alpha1GeneratorName: kubectl.CronJobV2Alpha1{},
CronJobV1Beta1GeneratorName: kubectl.CronJobV1Beta1{},
}

This comment has been minimized.

@ayushpateria

ayushpateria Mar 30, 2018

Contributor

Oops! :)

I probably got confused because we don't fall back in case there is no apps/v1beta1
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/util/factory_client_access.go#L559

Will fix this.

This comment has been minimized.

@ayushpateria

ayushpateria Mar 30, 2018

Contributor

Is that because if the user specifies the generator then we don't fall back?

This comment has been minimized.

@liggitt

liggitt Mar 30, 2018

Member

Is that because if the user specifies the generator then we don't fall back?

something like that. they explicitly asked for a specific output, and we probably should not change what they asked for.

This comment has been minimized.

@ayushpateria

ayushpateria Mar 30, 2018

Contributor

We do the same for create deployment - https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/create_deployment.go#L115
Should I add fall back for DeploymentAppV1Beta1 for consistency?

@ayushpateria

This comment has been minimized.

Contributor

ayushpateria commented Mar 30, 2018

I have refactored a lot of code and made the fallback logic consistent in create deployment and run. Ideally, if a user is explicitly providing the generator and it's not available, we shouldn't fall back, instead, we should just return an error. Currently, however, we would fall-back in case of create deployment and on running run with cronjob/v2alpha1. I have moved all the discovery checks under FallbackGeneratorNameIfNecessary, and this function is called only when the user has not provided the generator, if they have, then we don't fall back.

@soltysh

A few nits wrt fallback strategies.

@@ -55,7 +55,7 @@ func NewCmdCreateDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer) *cobra.
cmdutil.AddApplyAnnotationFlags(cmd)
cmdutil.AddValidateFlags(cmd)
cmdutil.AddPrinterFlags(cmd)
cmdutil.AddGeneratorFlags(cmd, cmdutil.DeploymentBasicV1Beta1GeneratorName)
cmdutil.AddGeneratorFlags(cmd, "")

This comment has been minimized.

@soltysh

soltysh Apr 10, 2018

Contributor

Make cmdutil.DeploymentBasicAppsV1GeneratorName the default generator.

generatorName, err = cmdutil.FallbackGeneratorNameIfNecessary(generatorName, clientset.Discovery(), cmdErr)
if err != nil {
return err
if len(generatorName) == 0 {

This comment has been minimized.

@soltysh

soltysh Apr 10, 2018

Contributor

The previous logic was sufficient, if you set the default generator to cmdutil.DeploymentBasicAppsV1GeneratorName.

This comment has been minimized.

@soltysh

soltysh Apr 10, 2018

Contributor

Besides, like I said elsewhere we always need to check the fallback.

if err != nil {
return err
// Falling back because the generator was not provided and the default one could be unavailable.
generatorNameTemp, err := cmdutil.FallbackGeneratorNameIfNecessary(generatorName, clientset.Discovery(), cmdErr)

This comment has been minimized.

@soltysh

soltysh Apr 10, 2018

Contributor

The fallback logic needs to kick in even when a generator is specified. We need to ensure that the default one is still available. What will happen if you take 1.11 client and any older server which does not have the new resource?

This comment has been minimized.

@liggitt

liggitt Apr 10, 2018

Member

Really? If I specify I want apps/v1 explicitly, I'd expect that to fail on an older server, just as if I tried to create -f file-with-appsv1.yaml

This comment has been minimized.

@soltysh

This comment has been minimized.

@soltysh

soltysh Apr 10, 2018

Contributor

I'm fine changing that behavior, though. We'll need to ensure it's consistent everywhere.

This comment has been minimized.

@ayushpateria

ayushpateria Apr 10, 2018

Contributor

Yeah, it's not consistent right now in run and create deployment. I've made it consistent in this PR. We don't fallback now if the user has explicitly provided the generator, which makes more sense to me.

This comment has been minimized.

@soltysh

soltysh Apr 10, 2018

Contributor

SGTM. In that case, just rebase your PR and I'll merge it in. Are all fallback working the same way right now?

This comment has been minimized.

@ayushpateria

ayushpateria Apr 10, 2018

Contributor

Rebased. Yes, FallbackGeneratorNameIfNecessary is being used only from run.go and create_deployment.go, I've made the logic consistent across both the files.

@ayushpateria

This comment has been minimized.

Contributor

ayushpateria commented Apr 10, 2018

/retest

@BenTheElder

This comment has been minimized.

Member

BenTheElder commented Apr 10, 2018

/retest

@soltysh

/lgtm
/approve no-issue

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 12, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 12, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 12, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 34c6fe3 into kubernetes:master Apr 12, 2018

14 of 15 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation ayushpateria authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@janetkuo

@ayushpateria please also update the generators doc: https://kubernetes.io/docs/reference/kubectl/conventions/#generators

During one of our previous sig-cli calls we discussed the chance of deprecating kubectl run so that in the long run it'll only run a pod and nothing else. kubectl create will be the path to go. So I'd prefer not to add anything new to run, the create part is ok.

@soltysh if so we should add kubectl create <resource> in the aforementioned doc (and remove the parts about kubectl run after it's deprecated).

@ayushpateria

This comment has been minimized.

Contributor

ayushpateria commented Apr 17, 2018

@janetkuo I have not touched run generators in this PR, so everything regarding run remains the same. I've indeed added a generator for create deployment, but I don't see any documentation page for it.

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 17, 2018

@ayushpateria I was suggesting that we update https://kubernetes.io/docs/reference/kubectl/conventions/#generators to include kubectl create <resource>

For example, add kubectl create <resource> to this bullet point:

  • Specify --generator to pin to a specific behavior forever when you use generator-based commands such as kubectl run or kubectl expose

Also add kubectl create <resource> to the best practices: https://kubernetes.io/docs/reference/kubectl/conventions/#best-practices

We can move generators outside of best practices (as a separate section) because it's not only used by kubectl run, like this:

  • Using kubectl in Reusable Scripts
  • Best Practices
    • kubectl create <resource>
    • kubectl run
      • // keep the table of kubectl run flag and default generator
    • kubectl apply
  • Generators
    • // The mapping between generator and resource (of a specific API version)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment