Split CMD into ENTRYPOINT and CMD for Dockerfile

[Bobonium]

--help is now an CMD which means explicitly providing a command override in kubernetes is no longer required. You can now simply provide the necessary arguments

With this change it's now possible to simply override the --help argument by providing explicit arguments instead.
Therefore explicitly using command in kubernetes resources is no longer required.

It might make seem like a trivial change, but this way it fits way better in my deployment automation and I'd appreciate if you could merge this.

[k8s-ci-robot]

Welcome @Bobonium!

It looks like this is your first PR to kubernetes-sigs/descheduler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/descheduler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @Bobonium. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[damemi]

/ok-to-test

[JaneLiuL]

the cronjob will start and exec the policy-config-file as executable file.

OCI runtime create failed: runc create failed: unable to start container process: exec: "--policy-config-file": executable file not found in $PATH: unknown 

and you can find the note from k8s doc https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#notes:

The command and arguments that you define in the configuration file override the default command and arguments provided by the container image. If you define args, but do not define a command, the default command is used with your new arguments.

[Bobonium]

Hey @JaneLiuL ,

does your test run against the currently released version or does it run against the changes I made to the Dockerfile?
What you describe should only happen if the tests run against the current release.

For reference:

This fails, because the current release does not set yet set the ENTRYPOINT in the Dockerfile

▶ docker run --rm k8s.gcr.io/descheduler/descheduler:v0.25.0 --policy-config-file /policy-dir/policy.yaml                 
docker: Error response from daemon: failed to create shim: OCI runtime create failed: container_linux.go:380: starting container process caused: exec: "--policy-config-file": executable file not found in $PATH: unknown.

This starts the descheduler correctly, because I use the newly built image based on the changes on this branch:

▶ docker run --rm 28e98f9bed50 --policy-config-file /policy-dir/policy.yaml
I1028 06:17:25.873278       1 secure_serving.go:210] Serving securely on [::]:10258
I1028 06:17:25.873321       1 tlsconfig.go:240] "Starting DynamicServingCertificateController"
E1028 06:17:25.873397       1 server.go:99] "descheduler server" err="Unable to build in cluster config: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"
I1028 06:17:25.873417       1 tlsconfig.go:255] "Shutting down DynamicServingCertificateController"
I1028 06:17:25.873702       1 secure_serving.go:255] Stopped listening on [::]:10258

If your tests run against the current release, I could remove the helm and yaml changes until a new release including the Dockerfile change has been rolled out.

[a7i]

@JaneLiuL Docker's ENTRYPOINT is equivalent to Kubernetes command so AFAIK there shouldn't be a change in behavior. Is that correct?

[a7i]

Is there a particular reason that you want to remove these from the YAML? redundant?

For example, my organization rebuilds the docker image to host to our private registry but we still rely on the defaults from the YAML/Chart

[Bobonium]

that's correct, since the Dockerfile already specified the "Entrypoint" (which is correctly called "command" in kubernetes) and the "CMD" (which is correctly called "arguments" in kubernetes), it's redundant to set them in kubernetes unless of course you intend to change them.
Setting them would simply override default behavior defined in the Dockerfile.

This is also why I initially introduced this change. This way users only need to specify the actual arguments, required to configure the descheduler. Up until now you'd have needed to either supply the command and arguments in the deployment, or just supply the arguments including the initial /bin/descheduler as the first one to make it work

[Bobonium]

Here's some output from docker that will better show case what these changes bring to the table.

Starting a container in the current release without any extra arguments shows the help text:

docker run --rm k8s.gcr.io/descheduler/descheduler:v0.25.1                           
The descheduler evicts pods which may be bound to less desired nodes

Usage:
  descheduler [flags]
  descheduler [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  help        Help about any command
  version     Version of descheduler

Flags:
      --add-dir-header                           If true, adds the file directory to the header of the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --alsologtostderr                          log to standard error as well as files (no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --bind-address ip                          The IP address on which to listen for the --secure-port port. The associated interface(s) must be reachable by the rest of the cluster, and by CLI/web clients. If blank or an unspecified address (0.0.0.0 or ::), all interfaces will be used. (default 0.0.0.0)
      --cert-dir string                          The directory where the TLS certs are located. If --tls-cert-file and --tls-private-key-file are provided, this flag will be ignored. (default "apiserver.local.config/certificates")
      --descheduling-interval duration           Time interval between two consecutive descheduler executions. Setting this value instructs the descheduler to run in a continuous loop at the interval specified.
      --disable-metrics                          Disables metrics. The metrics are by default served through https://localhost:10258/metrics. Secure address, resp. port can be changed through --bind-address, resp. --secure-port flags.
      --dry-run                                  execute descheduler in dry run mode.
  -h, --help                                     help for descheduler
      --http2-max-streams-per-connection int     The limit that the server gives to clients for the maximum number of streams in an HTTP/2 connection. Zero means to use golang's default.
      --kubeconfig string                        File with  kube configuration.
      --leader-elect                             Start a leader election client and gain leadership before executing the main loop. Enable this when running replicated components for high availability.
      --leader-elect-lease-duration duration     The duration that non-leader candidates will wait after observing a leadership renewal until attempting to acquire leadership of a led but unrenewed leader slot. This is effectively the maximum duration that a leader can be stopped before it is replaced by another candidate. This is only applicable if leader election is enabled. (default 2m17s)
      --leader-elect-renew-deadline duration     The interval between attempts by the acting master to renew a leadership slot before it stops leading. This must be less than or equal to the lease duration. This is only applicable if leader election is enabled. (default 1m47s)
      --leader-elect-resource-lock string        The type of resource object that is used for locking during leader election. Supported options are 'leases', 'endpointsleases' and 'configmapsleases'. (default "leases")
      --leader-elect-resource-name string        The name of resource object that is used for locking during leader election. (default "descheduler")
      --leader-elect-resource-namespace string   The namespace of resource object that is used for locking during leader election. (default "kube-system")
      --leader-elect-retry-period duration       The duration the clients should wait between attempting acquisition and renewal of a leadership. This is only applicable if leader election is enabled. (default 26s)
      --log-backtrace-at traceLocation           when logging hits line file:N, emit a stack trace (default :0) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-dir string                           If non-empty, write log files in this directory (no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-file string                          If non-empty, use this log file (no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-file-max-size uint                   Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-flush-frequency duration             Maximum number of seconds between log flushes (default 5s)
      --logging-format string                    Sets the log format. Permitted formats: "text", "json". Non-default formats don't honor these flags: --add-dir-header, --alsologtostderr, --log-backtrace-at, --log_dir, --log_file, --log_file_max_size, --logtostderr, --skip-headers, --skip-log-headers, --stderrthreshold, --log-flush-frequency.\nNon-default choices are currently alpha and subject to change without warning. (default "text")
      --logtostderr                              log to standard error instead of files (default true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --one-output                               If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --permit-address-sharing                   If true, SO_REUSEADDR will be used when binding the port. This allows binding to wildcard IPs like 0.0.0.0 and specific IPs in parallel, and it avoids waiting for the kernel to release sockets in TIME_WAIT state. [default=false]
      --permit-port-sharing                      If true, SO_REUSEPORT will be used when binding the port, which allows more than one instance to bind on the same address and port. [default=false]
      --policy-config-file string                File with descheduler policy configuration.
      --secure-port int                          The port on which to serve HTTPS with authentication and authorization. If 0, don't serve HTTPS at all. (default 10258)
      --skip-headers                             If true, avoid header prefixes in the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --skip-log-headers                         If true, avoid headers when opening log files (no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --stderrthreshold severity                 logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false) (default 2) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --tls-cert-file string                     File containing the default x509 Certificate for HTTPS. (CA cert, if any, concatenated after server cert). If HTTPS serving is enabled, and --tls-cert-file and --tls-private-key-file are not provided, a self-signed certificate and key are generated for the public address and saved to the directory specified by --cert-dir.
      --tls-cipher-suites strings                Comma-separated list of cipher suites for the server. If omitted, the default Go cipher suites will be used. 
                                                 Preferred values: TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_128_GCM_SHA256, TLS_RSA_WITH_AES_256_CBC_SHA, TLS_RSA_WITH_AES_256_GCM_SHA384. 
                                                 Insecure values: TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, TLS_ECDHE_ECDSA_WITH_RC4_128_SHA, TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, TLS_ECDHE_RSA_WITH_RC4_128_SHA, TLS_RSA_WITH_3DES_EDE_CBC_SHA, TLS_RSA_WITH_AES_128_CBC_SHA256, TLS_RSA_WITH_RC4_128_SHA.
      --tls-min-version string                   Minimum TLS version supported. Possible values: VersionTLS10, VersionTLS11, VersionTLS12, VersionTLS13
      --tls-private-key-file string              File containing the default x509 private key matching --tls-cert-file.
      --tls-sni-cert-key namedCertKey            A pair of x509 certificate and private key file paths, optionally suffixed with a list of domain patterns which are fully qualified domain names, possibly with prefixed wildcard segments. The domain patterns also allow IP addresses, but IPs should only be used if the apiserver has visibility to the IP address requested by a client. If no domain patterns are provided, the names of the certificate are extracted. Non-wildcard matches trump over wildcard matches, explicit domain patterns trump over extracted names. For multiple key/certificate pairs, use the --tls-sni-cert-key multiple times. Examples: "example.crt,example.key" or "foo.crt,foo.key:*.foo.com,foo.com". (default [])
  -v, --v Level                                  number for the log level verbosity
      --vmodule moduleSpec                       comma-separated list of pattern=N settings for file-filtered logging

Use "descheduler [command] --help" for more information about a command.

If I try to provide a parameter like --dry-run it will fail:

▶ docker run --rm k8s.gcr.io/descheduler/descheduler:v0.25.1 --dry-run
docker: Error response from daemon: failed to create shim: OCI runtime create failed: container_linux.go:380: starting container process caused: exec: "--dry-run": executable file not found in $PATH: unknown.

This is because it currently replaces all arguments, including the call to the binary itself.
So I need to run the following to make it work instead:

▶ docker run --rm k8s.gcr.io/descheduler/descheduler:v0.25.1 /bin/descheduler --dry-run
I1228 09:19:47.335744       1 secure_serving.go:210] Serving securely on [::]:10258
I1228 09:19:47.335765       1 tlsconfig.go:240] "Starting DynamicServingCertificateController"
E1228 09:19:47.335830       1 server.go:93] "descheduler server" err="Unable to build in cluster config: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"
I1228 09:19:47.335849       1 tlsconfig.go:255] "Shutting down DynamicServingCertificateController"
I1228 09:19:47.335914       1 secure_serving.go:255] Stopped listening on [::]:10258

Alternatively I could set the entrypoint instead of providing the argument like this for the same result:

▶ docker run --rm --entrypoint=/bin/descheduler k8s.gcr.io/descheduler/descheduler:v0.25.1 --dry-run                 
I1228 09:20:40.098356       1 secure_serving.go:210] Serving securely on [::]:10258
E1228 09:20:40.098418       1 server.go:93] "descheduler server" err="Unable to build in cluster config: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"
I1228 09:20:40.098421       1 tlsconfig.go:240] "Starting DynamicServingCertificateController"
I1228 09:20:40.098437       1 tlsconfig.go:255] "Shutting down DynamicServingCertificateController"
I1228 09:20:40.098521       1 secure_serving.go:255] Stopped listening on [::]:10258

The change I introduced, simply allows you to not have to redundantly call /bin/descheduler as an argument.
It might look minor, but in reality it means right now every new user will stumble upon this when he doesn't use the helm chart and then they'll have to check what is the actual executable to call and manually do that before supplying arguments. It bothered me having to do this, so I immediately also provided a solution to that problem according to container best practices.

[a7i]

Thoughts on reverting the YAML/template changes?

[a7i]

/lgtm
/assign @ingvagabund

[ingvagabund]

This might get easily broken in the future by someone not knowing the reasoning behind.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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:

• OWNERS [ingvagabund]

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

[a7i]

@ingvagabund that's a good point. Now I'm second guessing whether this is a "breaking change" for those who assume what the value of CMD is (as opposed to overriding it).

[damemi]

@a7i this will break any deployments that manage their own manifests. deployments that use our helm chart/manifests should be ok.

I would have preferred to wait and hold this for at least the next release, which gives us time to announce the change at least. It's not a huge break, so I'm open to whatever others think is appropriate here

[ingvagabund]

Unless I misunderstood everyone setting the command and args explicitly in the manifest will not get affected by this change. Otherwise, we should revert and announce the change in the release notes before we apply it.

[a7i]

this will break any deployments that manage their own manifests.

everyone setting the command and args explicitly in the manifest will not get affected by this

I believe you're both saying the same thing, that is my understanding as well.

[damemi]

I was talking about the change as shown in #995 (comment)

Where you used to have to docker run <image> descheduler <args> now that won't work, and you have to drop the descheduler command (ie docker run <image> <args>)

K8s might implicitly handle this difference, so it might only matter for users who are manually running the docker image themselves. Probably not common, but still possible and a noticeable change.

[ingvagabund]

Reading https://docs.docker.com/engine/reference/builder/#understand-how-cmd-and-entrypoint-interact as a recommended practice:

ENTRYPOINT should be defined when using the container as an executable.

I don't have any strong opinion about whether it's better to run docker run with or without the descheduler or /usr/bin/descheduler binary. It seems more natural to see the docker run as running the descheduler binary directly. As long as there's no need to mount volumes. Which is not the case here. There's no point of running the descheduler without specifying --policy-config-file which needs to be mounted anyway. We should probably revert.

[damemi]

As long as there's no need to mount volumes. Which is not the case here. There's no point of running the descheduler without specifying --policy-config-file which needs to be mounted anyway. We should probably revert.

Just curious because I'm missing it, but why does the need to mount a volume change this?

[ingvagabund]

As a user when I run an image as a executable I want to run as any other executable. When passing a path (e.g. config file) I don't expect to mount path to the config as well. My user preference. Otherwise, when I have to mount directories as well, I am totally fine explicitly specifying the binary/command.

[damemi]

@ingvagabund I'm still not sure I understand what you're saying... Does mounting directories affect the entrypoint? Or you just prefer explicitly passing the binary argument in that case?

This is blocking the release, so let's decide whether to revert or not. I'll approve a PR to revert if that's what we want and we can come back to this.

[a7i]

Let's go ahead and revert, please.

@Bobonium thank you for the contribution and I agree that this is a good change. It got merged late in the release cycle and we just need more time for announcement, testing.

[ingvagabund]

Or you just prefer explicitly passing the binary argument in that case?

Assuming ls is a container i would like to avoid docker run -v /path/to/directory:/path/to/directory ls /path/to/directory. Instead, I would just expect to run docker run -v ls /path/to/directory. Which will not work as expected.

[damemi]

Ah I see, good point, thanks for clarifying