Converting from usage of kubectl run --limits to --overrides not clear

[ckittel]

What happened: kubectl run --limits produces deprecation warning, but alternative doesn't seem to be working either.

This was introduced in kubernetes/kubernetes#99732

What you expected to happen: kubectl run --limits claims to not work, but it actually does still. But using --overrides doesn't seem to behave the way I'd expect it either.

How to reproduce it (as minimally and precisely as possible):

First an execution using the deprecated --limits flag.

kubectl run curl -n a0008 --image=mcr.microsoft.com/azure-cli --limits='cpu=200m' --dry-run=client -o yaml

Flag --limits has been deprecated, ---> has no effect <--- and will be removed in the future.
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  labels:
    run: curl
  name: curl
  namespace: a0008
spec:
  containers:
  - image: mcr.microsoft.com/azure-cli
    name: curl
    resources:
      limits:
        cpu: 200m    <---- but it does.
  dnsPolicy: ClusterFirst
  restartPolicy: Always
status: {}

Which is fine, that the command that says it has no effect actually does have an effect. But the workaround (as I understand it) doesn't work either, as it clobbers all of the other values as well (below you'll see image removed, for example)

kubectl run curl -n a0008 --image=mcr.microsoft.com/azure-cli --dry-run=client -o yaml --overrides='{"spec":{"containers":[{"name":"curl","resources":{"limits":{"cpu":"200m"}}}]}}'

apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  labels:
    run: curl
  name: curl
  namespace: a0008
spec:
  containers:
  - name: curl
    resources:
      limits:
        cpu: 200m
  dnsPolicy: ClusterFirst
  restartPolicy: Always
status: {}

It's as if the overrides JSON patch isn't doing a merge on the merge key of container name.

Anything else we need to know?:

Either the patch merge process could be made clearer, or limits could maybe be no longer marked deprecated since it seems to do its job just exactly fine.

Environment:

• Kubernetes client version: 1.21.1

[eddiezane]

Cheers @ckittel,

Apologies for the issues you've run into.

I'm not sure about the messaging in those deprecated flags but I will find out. The implementation has indeed not been removed yet.

The issue you're running into with your overrides appears to be that JSON Merge Patch (RFC 7396) (I just read it for the first time and could be wrong) doesn't work with objects inside arrays which is what we use here and here.

If the patch is anything other than an object, the result will always be to replace the entire target with the entire patch. Also, it is not possible to patch part of a target that is not an object, such as to replace just some of the values in an array.

I don't see a way to apply your patch as expected without providing the entire object (including image) which is an issue because --image is a required flag but is a no-op here.

We could potentially use a 6902 patch which I believe we use in kustomize. cc @KnVerey

This highlights one of reasons we've been wanting to remove as much of kubectl run and kubectl create as possible. Kubernetes is intended to be used as an declarative tool and these commands are very imperative. Representing structured information via flags got messy (the output of kubectl run --help is a wall of text) and fragile very quickly and is a burden on maintainers. Sorry for the rant - just providing some context.

I've got this on the agenda for our meeting next week if you'd like to join us.

/triage accepted
/priority important-soon

[ckittel]

Thanks for the added context. I'll see if I can join the meeting next week to talk about our use case. I fully recognize the desire to minimize utility of things like imparative commands like. We leverage this feature to give our learners "proof of implementation" style tests. E.g. To show that your Network Policies are in place, spin up a one-time shell in this namespace and then execute $command to see the results. Or to show that your Ingress Controller is only responding from external to the cluster, you can simulate an attempt to violate that by shelling in and executing $command.

If I cannot attended in person, I'll watch for updates here. Thanks!

[eddiezane]

Followup from the call yesterday.

We will remove the functionality so the flag message matches the behavior for 1.23. The flags will be removed in 1.24

/assign @eddiezane

We will try and add support for 6902 patches while maintaining backwards compatibility. Perhaps a new flag that specifies the patch type.

/assign @brianpursley

[almson]

kubectl create is unnecessary, but kubectl run has a clear purpose kubernetes/kubernetes#28695 Maybe you can repurpose it for only running a single pod without restarting and always attaching (or provide a new command). In any case, setting requests/limits is important in this use-case (as in all use-cases).

[DavidPerezIngeniero]

I have this problem when specifiying limits with overrides and using stdin: https://stackoverflow.com/questions/76108140/how-to-specify-limits-with-kubectl-run

Maybe the workaround is to use an old version of kubectl, but I don't like this solution.

[DavidPerezIngeniero]

When using limits +stdin, kubectl run doesn't work. I've had to use kubectl apply, kubectl wait and kubectl attach.

[thomas10-10]

why did you remove the --limit to make it more complicated to use?

[nikolaydubina]

shame can't use run now for gpu limits

[brianpursley]

shame can't use run now for gpu limits

have you tried using --overrides with a JSON patch?

kubectl run test --image=busybox --overrides='[{"op":"replace", "path":"/spec/containers/0/resources/limits", "value":{"nvidia.com/gpu": 2}}]' --override-type=json --dry-run=client -o yaml

apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  labels:
    run: test
  name: test
spec:
  containers:
  - image: busybox
    name: test
    resources:
      limits:
        nvidia.com/gpu: "2"
  dnsPolicy: ClusterFirst
  restartPolicy: Always
status: {}