kubernetes / kubectl

Issue tracker and mirror of kubectl code
Apache License 2.0
2.89k stars 924 forks source link

kubectl: when using kubectl get PriorityClass, preemptionPolicy field should be printed #1640

Closed googs1025 closed 3 months ago

googs1025 commented 3 months ago

What would you like to be added?

Added preemptionPolicy field when using kubectl get PriorityClass

root@VM-0-15-ubuntu:/home/ubuntu# kubectl get PriorityClass
NAME                      VALUE        GLOBAL-DEFAULT   AGE
system-cluster-critical   2000000000   false            14h
system-node-critical      2000001000   false            14h
root@VM-0-15-ubuntu:/home/ubuntu# kubectl get PriorityClass
NAME                      VALUE        GLOBAL-DEFAULT   PreemptionPolicy      AGE
system-cluster-critical   2000000000   false            PreemptLowerPriority  14h
system-node-critical      2000001000   false            PreemptLowerPriority  14h

Why is this needed?

When we're dealing with PriorityClass objects in the cluster, we not only care about the value but also pay attention to the PreemptionPolicy field. This helps us quickly understand if this PriorityClass allows preemption. During operational tasks involving preemption or eviction, we frequently check the PriorityClass. For certain PriorityClass setups, due to business requirements, we set PreemptionPolicy = Never. It's crucial for us to swiftly locate these PriorityClass resources when needed.

k8s-ci-robot commented 3 months ago

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
googs1025 commented 3 months ago

/sig cli

googs1025 commented 3 months ago

/assign

xuzhenglun commented 3 months ago

I'm not sure that is anybody dependent on column number here, which could break the compatibility.

For example a guy wrote a script like this:

kubectl get PriorityClass | awk '{print $(NF-1)}'
googs1025 commented 3 months ago

I'm not sure that is anybody dependent on column number here, which could break the compatibility.

For example a guy wrote a script like this:

kubectl get PriorityClass | awk '{print $(NF-1)}'

IMO, different versions of the Kubernetes API may cause changes in the output format, which is inevitable, which is why we need release-notes to let users know what has changed. In addition, if we are writing a script, it is more stable to use field names instead of column numbers to extract data. Field names are more stable in the changes of the entire table structure, so they are less susceptible to future changes.

xuzhenglun commented 3 months ago

s of the Kubernetes API may cause changes in the output format, which is inevitable, which is why we need release-notes to let users know what has changed. In addition, if we are writing a script, it is more stable to use field names instead of colu

yeah, It's make sense to me and I totally agree with you.

I was just expressing my concerns and trying to figure out what are the boundaries of acceptable influence. cuz the usage of client-side is really wild u know.

brianpursley commented 3 months ago

People shouldn't be awk-ing get's output like that, but it wouldn't surprise me if some people do. A way better alternative would be for them to use -o json and jq. Far less fragile.

I think in general, we don't consider the table output to be an API, so it can be changed, although we usually try to make sure any changes are generally useful so that there doesn't end up being a crazy number of columns. Alternatively, new columns can be added to the -o wide output as well, so that's another option.

In this case there aren't too many columns in the table output (4), and it seems reasonable to want to see this value. I'd like to hear what some other thoughts on this though, either as comments, or in the next sig-cli bug scrub meeting.

In the meantime, one workaround for this is to use -o custom-columns to build your own table output, something like this. It's definitely not convenient, but maybe you can make an alias or put it in a utility script.

kubectl get pc -o custom-columns=NAME:.metadata.name,PREEMPTION-POLICY:.preemptionPolicy
NAME                      PREEMPTION-POLICY
system-cluster-critical   PreemptLowerPriority
system-node-critical      PreemptLowerPriority

Transferring to the kubectl repo for visibility... /transfer kubectl

googs1025 commented 3 months ago

People shouldn't be awk-ing get's output like that, but it wouldn't surprise me if some people do. A way better alternative would be for them to use -o json and jq. Far less fragile.

I think in general, we don't consider the table output to be an API, so it can be changed, although we usually try to make sure any changes are generally useful so that there doesn't end up being a crazy number of columns. Alternatively, new columns can be added to the -o wide output as well, so that's another option.

In this case there aren't too many columns in the table output (4), and it seems reasonable to want to see this value. I'd like to hear what some other thoughts on this though, either as comments, or in the next sig-cli bug scrub meeting.

In the meantime, one workaround for this is to use -o custom-columns to build your own table output, something like this. It's definitely not convenient, but maybe you can make an alias or put it in a utility script.

kubectl get pc -o custom-columns=NAME:.metadata.name,PREEMPTION-POLICY:.preemptionPolicy
NAME                      PREEMPTION-POLICY
system-cluster-critical   PreemptLowerPriority
system-node-critical      PreemptLowerPriority

Transferring to the kubectl repo for visibility... /transfer kubectl

Thanks for your response! Appreciate you sharing that using -o custom-columns can solve this issue. Additionally, I was thinking that if we don't want to disrupt the original output logic, maybe adding to -o wide could be a good option. I think the PreemptionPolicy field should be displayed; it's a key field to determine if preemption is in effect. For non-critical fields of resources, using -o custom-columns is perfectly suitable. However, for key fields, I think they should be provided by default.

mpuckett159 commented 3 months ago

/triage accepted /priority backlog

googs1025 commented 3 months ago

Close this issue because the https://github.com/kubernetes/kubernetes/pull/126529 has been completed. Feel free to reopen it if we still have questions to discuss. :)

googs1025 commented 3 months ago

/close

k8s-ci-robot commented 3 months ago

@googs1025: Closing this issue.

In response to [this](https://github.com/kubernetes/kubectl/issues/1640#issuecomment-2292801326): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.