Skip to content

Conversation

@ykakarap
Copy link

@ykakarap ykakarap commented Apr 6, 2021

This KEP proposes adding a --subresource flag to kubectl to be able to fetch and update status and scale subresources.

There is a POC implementation at : kubernetes/kubernetes#99556

For issue: #2590
xref kubernetes/kubectl#564 (comment), kubernetes/kubernetes#15858 (comment), kubernetes/kubernetes#67455
Follow up to kubernetes/kubernetes#60902, kubernetes/kubernetes#67454

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 6, 2021
@k8s-ci-robot k8s-ci-robot requested review from pwittrock and seans3 April 6, 2021 15:50
@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 6, 2021
@nikhita
Copy link
Member

nikhita commented Apr 6, 2021

/cc @eddiezane @soltysh @seans3 @pwittrock

/assign @eddiezane

@nikhita
Copy link
Member

nikhita commented Apr 6, 2021

/assign @soltysh

@eddiezane
Copy link
Member

Demo of this from sig-cli bi-weekly https://youtu.be/zUa7dudYCQM?t=299.

```shell
# For built-in types
# update spec.replicas through scale subresource
$ kubectl patch deployment nginx-deployment --subresource='scale' --type='merge' -p '{"spec":{"replicas":2}}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the actual benefit of doing that though?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ability to modify the scale subresource was added to be consistent with the similar behavior provided to the status subresrouce.

@nikhita if you had any specific uses cases for modifying scale, we should document them in the KEP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than convenience I don't see much value, but I won't object either.


```shell
# cron-with-status.yaml has .status.replicas=3
$ kubectl apply -f cron-with-status.yaml --subresource=status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will override the last-applied annotation set by another kubectl apply occurence (probably on main resource), which will result in odd behaviors later. Also note that some resources don't allow annotations to be updated through status subresource. That would probably work with server-side apply.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This potentially odd behavior was brought up during the SIG-CLI demo too. I am currently investigating what should be done to handle this properly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was another question I brought during the call, if the argument contains just status or full object?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when using apply with subresource it turns out the last-applied annotation is not modified. The annotation is under the metadata section and that section and any changes to the updates are ignored when modifying the subresources.

Therefore any subsequent apply calls to the resource itself are not affected as the patch calculated for the apply call will use the correct last-applied configuration of resource.

The same cannot be said when apply-ing a subresource. The patch calculated using the annotation could be wrong because the last applied configuration of the status is not captured in the annotation.

```shell
# For built-in types
# update spec.replicas through scale subresource
$ kubectl patch deployment nginx-deployment --subresource='scale' --type='merge' -p '{"spec":{"replicas":2}}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than convenience I don't see much value, but I won't object either.


```shell
# cron-with-status.yaml has .status.replicas=3
$ kubectl apply -f cron-with-status.yaml --subresource=status
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was another question I brought during the call, if the argument contains just status or full object?

@ykakarap
Copy link
Author

ykakarap commented May 6, 2021

@soltysh I have address all the comments on the comments. Please take a look :)

@ykakarap ykakarap force-pushed the kubectl_subresource branch from 0b82453 to 7a906c0 Compare May 6, 2021 22:15
- [ ] Feature gate (also fill in values in `kep.yaml`)
- Feature gate name:
- Components depending on the feature gate:
- [x] Other
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the user know it's alpha?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have an explicit mechanism for stating if a flag is alpha or not, there are a couple of approaches:

  1. Use alpha prefix in the flag name, but I personally don't like that approach since it means we will have to change flag name eventually.
  2. Force using env var, which allows this flag.
  3. Hiding the flag by default, similarly to how deprecated flags are hidden.
  4. Providing a description which is explicitly calling out it's alpha.

I'm personally leaning towards 3 or 4, and given this is an explicit opt-in (you need to place the flag when invoking kubectl command) I'm not worried that users will have issues with that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for having a description explicitly calling out it's alpha.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, can you pick whatever mechanism is preferred by the SIG and document it here. I don't have a preference really. Otherwise all looks good for PRR, I will approve after that change and the SIG approves.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to option 4 (description explicitly calling out as alpha) and squashed the commits. PTAL :)

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve
from sig-cli pov

- [ ] Feature gate (also fill in values in `kep.yaml`)
- Feature gate name:
- Components depending on the feature gate:
- [x] Other
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have an explicit mechanism for stating if a flag is alpha or not, there are a couple of approaches:

  1. Use alpha prefix in the flag name, but I personally don't like that approach since it means we will have to change flag name eventually.
  2. Force using env var, which allows this flag.
  3. Hiding the flag by default, similarly to how deprecated flags are hidden.
  4. Providing a description which is explicitly calling out it's alpha.

I'm personally leaning towards 3 or 4, and given this is an explicit opt-in (you need to place the flag when invoking kubectl command) I'm not worried that users will have issues with that.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 11, 2021
This KEP proposes supporting a new flag `--subresource` to get, patch,
edit and replace kubectl commands to fetch and update `status` and
`scale` subresources.
@nikhita nikhita force-pushed the kubectl_subresource branch from 8b65c13 to 6094074 Compare May 12, 2021 06:18
@nikhita nikhita mentioned this pull request May 12, 2021
12 tasks
@soltysh
Copy link
Contributor

soltysh commented May 12, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2021
@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, soltysh, ykakarap

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2021
@k8s-ci-robot k8s-ci-robot merged commit d78aa0c into kubernetes:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants