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

add subresource status for vpa #5680

Merged
merged 1 commit into from
May 2, 2023

Conversation

wu0407
Copy link
Contributor

@wu0407 wu0407 commented Apr 13, 2023

What type of PR is this?

What this PR does / why we need it:

Current vpa crd has empty subresource field, that leads to metadata.generation increase on vpa status update.
The controller-runtime has GenerationChangedPredicate to filter out update status event for cr, but it not work for vpa.
This PR add status field in subresource on crd yaml and add new ClusterRole system:vpa-actor to patch /status subresource.
The metadata.generation only increase on vpa spec update.

Which issue(s) this PR fixes:

Fixes: #5675

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added the /status subresource to VPA
action required: If you're installing VPA with different means than the `hack/vpa-up.sh` script, you need to add the ClusterRole `system:vpa-status-actor` and corresponding ClusterRoleBinding yourself. Otherwise VPA will no longer be able to update the recommendations!

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Apr 13, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 13, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: wu0407 / name: wowqing (50653fb)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 13, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @wu0407!

It looks like this is your first PR to kubernetes/autoscaler 🎉. 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/autoscaler 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 k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 13, 2023
@k8s-ci-robot k8s-ci-robot requested a review from kgolab April 13, 2023 03:59
@k8s-ci-robot k8s-ci-robot requested a review from krzysied April 13, 2023 03:59
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 13, 2023
Copy link
Contributor

@voelzmo voelzmo left a comment

Choose a reason for hiding this comment

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

Good catch, thanks! Just a few comments from my side, mostly dealing with semantics.

vertical-pod-autoscaler/deploy/vpa-rbac.yaml Outdated Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/vpa/api.go Show resolved Hide resolved
@wu0407 wu0407 force-pushed the add-status-subresource branch from 50653fb to 2a42654 Compare April 22, 2023 00:43
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 22, 2023
@voelzmo
Copy link
Contributor

voelzmo commented Apr 24, 2023

Thanks for the changes! The only thing missing from my side is a release note pointing people to the new, required permissions. There are lots of people using different means to deploy the VPA (e.g. Helm charts or home-grown scripts/code), we need to point them and the maintainers of these solutions to the changes they need to make to their solutions.
What about something like this

Added the /status subresource to VPA
action required: If you're installing VPA with different means than the `hack/vpa-up.sh` script, you need to add the ClusterRole `system:vpa-status-actor` and corresponding ClusterRoleBinding yourself. Otherwise VPA will no longer be able to update the recommendations!

@jbartosik: I don't think we're adding these release notes automatically to the github releases yet, but I guess this would be helpful when compiling the release notes? WDYT?

@jbartosik
Copy link
Collaborator

Thanks for the changes! The only thing missing from my side is a release note pointing people to the new, required permissions. There are lots of people using different means to deploy the VPA (e.g. Helm charts or home-grown scripts/code), we need to point them and the maintainers of these solutions to the changes they need to make to their solutions. What about something like this

Added the /status subresource to VPA
action required: If you're installing VPA with different means than the `hack/vpa-up.sh` script, you need to add the ClusterRole `system:vpa-status-actor` and corresponding ClusterRoleBinding yourself. Otherwise VPA will no longer be able to update the recommendations!

Joachim: I don't think we're adding these release notes automatically to the github releases yet, but I guess this would be helpful when compiling the release notes? WDYT?

Release notes from PR descriptions aren't automatically added to Github releases. But they make writing release notes easier. Please add

@jbartosik jbartosik added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 25, 2023
Copy link
Collaborator

@jbartosik jbartosik left a comment

Choose a reason for hiding this comment

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

Also if I understand correctly the point here is that there will be user-visible API change.

Could you explain what will change? It's not clear to me from reading the isse

@jbartosik
Copy link
Collaborator

Please fix the commit message. I think you can't reference issues / PRs in commit messages. You can (and should and do) do that in PR mesages

@voelzmo
Copy link
Contributor

voelzmo commented Apr 25, 2023

Also if I understand correctly the point here is that there will be user-visible API change.

I don't think anything will change for users, except that currently metadata.generation is increased on every new recommendation, which isn't necessary. Another aspect is that the recommender has permissions to change all VPA .specs, which it doesn't need – permissions for changing /status is sufficient.

For operators, though, we need to ensure that they update CRD, RBAC rules and VPA version at the same time, otherwise this will just stop working.

@wu0407 wu0407 force-pushed the add-status-subresource branch 2 times, most recently from f27c02c to b8df346 Compare April 26, 2023 06:52
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Apr 26, 2023
@wu0407
Copy link
Contributor Author

wu0407 commented Apr 26, 2023

Thanks for the changes! The only thing missing from my side is a release note pointing people to the new, required permissions. There are lots of people using different means to deploy the VPA (e.g. Helm charts or home-grown scripts/code), we need to point them and the maintainers of these solutions to the changes they need to make to their solutions. What about something like this

Added the /status subresource to VPA
action required: If you're installing VPA with different means than the `hack/vpa-up.sh` script, you need to add the ClusterRole `system:vpa-status-actor` and corresponding ClusterRoleBinding yourself. Otherwise VPA will no longer be able to update the recommendations!

Joachim: I don't think we're adding these release notes automatically to the github releases yet, but I guess this would be helpful when compiling the release notes? WDYT?

Release notes from PR descriptions aren't automatically added to Github releases. But they make writing release notes easier. Please add

Added detail what to do in this pr and commit

@jbartosik jbartosik removed the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Apr 26, 2023
Add status field in subresource on crd yaml and add new ClusterRole system:vpa-actor to patch /status subresource.
The `metadata.generation` only increase on vpa spec update.
@wu0407 wu0407 force-pushed the add-status-subresource branch from b8df346 to 8e72470 Compare April 27, 2023 03:17
@voelzmo
Copy link
Contributor

voelzmo commented Apr 27, 2023

The release notes look good so far, they just need to mention that operators also need to update their CRD. Sorry I missed that in the first suggestion I had about the notes.

Copy link
Contributor

@mwielgus mwielgus 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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mwielgus, wu0407

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 2, 2023
@k8s-ci-robot k8s-ci-robot merged commit 850a0a8 into kubernetes:master May 2, 2023
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. area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vpa CRD not contain status subresource
5 participants