Skip to content
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

Support apiserver dry-run in kubectl #85652

Closed
julianvmodesto opened this issue Nov 26, 2019 · 16 comments
Closed

Support apiserver dry-run in kubectl #85652

julianvmodesto opened this issue Nov 26, 2019 · 16 comments

Comments

@julianvmodesto
Copy link
Contributor

@julianvmodesto julianvmodesto commented Nov 26, 2019

What would you like to be added:
We need to implement a consistent server-side "dry-run" flag in kubectl.

@apelisse proposed --dry-run=server to provide access to the apiserver's dry-run feature for GA, which re-uses the existing boolean flag, and leaves a path for --dry-run=client. The server-side behavior as in --dry-run=server would become the new default for --dry-run, since most kubectl users depend on the flag for dry-run validation in CI automation, which is most at danger of breaking.

Why is this needed:
Currently, the kubectl --dry-run flag does its best client-side to do the right thing.

kubectl users often run kubectl apply --dry-run --validate -f ... for best-effort server-side validation locally or in CI (see #11488), and other kubectl users run kubectl create configmap -dry-run --local --output yaml ... for client-side dry-run for manifest generation (see #51475).

Tasks:

  • add --dry-run=[client|server|none] values
  • update existing kubectl commands that already support --dry-run=client to also support --dry-run=server
  • error when both --dry-run=server and --local are specified because they're incompatible
  • add --dry-run=server and --dry-run=client support to commands without any existing --dry-run support at all
    • kubectl delete
    • kubectl scale pending #88274
    • kubectl replace
    • kubectl taint

Dry-run umbrella issues: kubernetes/enhancements#576 #68514
Dry-run kep: https://github.com/kubernetes/community/blob/master/keps/sig-api-machinery/0015-dry-run.md
Dry-run initial issue: #11488

/sig api-machinery
/sig cli

@apelisse

This comment has been minimized.

Copy link
Member

@apelisse apelisse commented Nov 26, 2019

@seans3 @soltysh @smarterclayton Any opinion on that? We still haven't implemented "server-dry-run" for most commands beside kubectl apply. I'm suggesting --dry-run=[server|client] with client being the default for now, and potentially changing it later. WDYT? Happy to come to sig-cli next wednesday to talk aboutt it.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

@smarterclayton smarterclayton commented Nov 30, 2019

We’d have to have backward compatibility for —dry-run=true and —dry-run, but other than that I don’t see too many issues. What would it mean to change the default later on? We have —local, if I pass —dry-run=server and —local should I get an error? Can we use —local to modify dry run instead of making it multi-value?

@julianvmodesto

This comment has been minimized.

Copy link
Contributor Author

@julianvmodesto julianvmodesto commented Dec 2, 2019

What would it mean to change the default later on?

Changing the default later would use more accurate server-side validation and more accurately match intent to validate a request. This shouldn't be breaking because the existing --dry-run flag requires an apiserver connection.

Can we use —local to modify dry run instead of making it multi-value?

--dry-run seems like a good fit because --local is not as proliferated, particularly for commands such as create and apply, whereas --dry-run is available for apply and create.

set is an example of a command with both --local and --dry-run. These are the docs for the flags, and it looks like the requested behavior is overlapping:

dry-run

If true, only print the object that would be sent, without sending it.

local

If true, set image will NOT contact api-server but run locally.

Could we deprecate --local in favor of --dry-run/--dry-run=true/--dry-run=client?

@apelisse

This comment has been minimized.

Copy link
Member

@apelisse apelisse commented Dec 2, 2019

@julianvmodesto

This comment has been minimized.

Copy link
Contributor Author

@julianvmodesto julianvmodesto commented Dec 4, 2019

As discussed during the sig-cli meeting, we'll proceed with extending the existing flag to support --dry-run=server, which is available for kubectl apply and kubectl create.

/cc @soltysh

We'll retain the existing default for backwards compatibility. We won't be using --local for server-side dry-run.

I'll update the dry-run KEP with this change.

@apelisse

This comment has been minimized.

Copy link
Member

@apelisse apelisse commented Dec 4, 2019

As a side-note: if we want to be perfectly backward-compatible on the --dry-run flag, going from boolean to string is not "that trivial":
--dry-run=true, --dry-run=True, --dry-run=1 are all valid options for the boolean. And I'm probably missing some.

@apelisse

This comment has been minimized.

Copy link
Member

@apelisse apelisse commented Dec 4, 2019

Also, --local doesn't exist for all commands and has a slightly different semantic, so I don't think we should tie the behavior of the two. It might make sense to disallow --local with some values of --dry-run though.

@julianvmodesto

This comment has been minimized.

Copy link
Contributor Author

@julianvmodesto julianvmodesto commented Dec 9, 2019

going from boolean to string

if m.localFn == nil || !m.localFn() {

Looks like we'll convert cmdutil's func GetDryRun() string to func GetDryRun() bool. cobra uses strconv.ParseBool, so we should call this for parsing truthy for client-side dry-run.

It might make sense to disallow --local with some values of --dry-run though.

It looks like we can return a LocalResourceError if --local and --dry-run=server for subcommands like kubectl set

LocalParam(o.Local).
ContinueOnError().
NamespaceParam(cmdNamespace).DefaultNamespace().
FilenameParam(enforceNamespace, &o.FilenameOptions).
Flatten()
if !o.Local {
builder.LabelSelectorParam(o.Selector).
ResourceTypeOrNameArgs(o.All, o.Resources...).
Latest()
} else {
// if a --local flag was provided, and a resource was specified in the form
// <resource>/<name>, fail immediately as --local cannot query the api server
// for the specified resource.
if len(o.Resources) > 0 {
return resource.LocalResourceError
}
}

@julianvmodesto

This comment has been minimized.

Copy link
Contributor Author

@julianvmodesto julianvmodesto commented Dec 9, 2019

Started to update the KEP kubernetes/enhancements#1399

Looking at cli-runtime, it looks like --local doesn't pass the REST mapper -- I'm not sure yet, but I think omitting the REST mapper doesn't call the apiserver at all, so no server-side call with any dry-run parameter is called. It seems like --local might be useful because of this, and we probably want to deprecate the --dry-run=client codepath entirely after 2 releases... Should we do this + track this?

if m.localFn == nil || !m.localFn() {

@julianvmodesto

This comment has been minimized.

Copy link
Contributor Author

@julianvmodesto julianvmodesto commented Dec 29, 2019

This change is being implemented.

The original PR is #86143, but it's quite big so we're splitting it into smaller changes, starting with #86408.

@riking

This comment has been minimized.

Copy link

@riking riking commented Feb 13, 2020

For backwards compatibility, we'll continue to default the value for --dry-run to --dry-run=client, which is equivalent to the existing behavior for --dry-run=true.

The boolean values for --dry-run will be deprecated next release, then removed in 2 releases.

The default value for --dry-run with the flag set and unspecified will be deprecated in the next release, then in 2 releases we will require that a value must be specified. Users must update any scripts to explicitly set --dry-run=client or --dry-run=server.

I'm unsure this is a long enough timeline for blog posts & stuff to get updated - you may need to target a large educational campaign to avoid widespread breakage.

I'd expect to live with the defaulting transition for about a year, assuming that the communication is good.

@apelisse

This comment has been minimized.

Copy link
Member

@apelisse apelisse commented Feb 13, 2020

Thanks for your comment @riking. We're are going to update as much as we can, at least to the extent of "kubernetes" website, documentation, tests and references. I agree with you that there is going to be a long tail of things to be updated. With the current plan, this offers more than 6 months for this to happen. Obviously, we can reconsider in 6 months and adjust that as needed. The goal is to eventually make "server" the default, ideally we can make that happen in less than 1.5 years :-)

@riking

This comment has been minimized.

Copy link

@riking riking commented Feb 13, 2020

With the context of that plan, it makes a lot more sense - should probably be included in the deprecation messaging, then!
I reached here from the alpha changelog.

@apelisse

This comment has been minimized.

Copy link
Member

@apelisse apelisse commented Feb 13, 2020

Thanks, I'll look at it

@julianvmodesto

This comment has been minimized.

Copy link
Contributor Author

@julianvmodesto julianvmodesto commented Feb 28, 2020

I'm going to close this one for now as done and file a follow up for kubectl scale since it's kind of blocked on #88274

Next is docs!

/close

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Feb 28, 2020

@julianvmodesto: Closing this issue.

In response to this:

I'm going to close this one for now as done and file a follow up for kubectl scale since it's kind of blocked on #88274

Next is docs!

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.