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

VPA - Adding status to Admission Controller #2763

Merged

Conversation

krzysied
Copy link
Contributor

@krzysied krzysied commented Jan 22, 2020

Adding status object (Lease) to Admission Controller.
Adding rbac entries for Leases manipulation.

ref #2738

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2020
@krzysied
Copy link
Contributor Author

/assign @bskiba

@krzysied krzysied force-pushed the vpa_admission_controller_status2 branch from c240ef1 to 7de0eb1 Compare January 22, 2020 17:38
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2020
Copy link
Member

@bskiba bskiba left a comment

Choose a reason for hiding this comment

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

Nits only

return
case <-time.After(updateInterval):
if err := su.client.UpdateStatus(); err != nil {
klog.Errorf("Admission Controller status update failed: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think Warning should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imho we want to have Updater logic to be dependent on this status, so error sounds reasonable.
Plus, there are retries implemented, so this situation shouldn't happen unless something is really off.

}

// Start starts status updates.
func (su *StatusUpdater) Start() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: isn't Run more popular for k8s controllers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to Run(<-chan struct{}) signature.

@@ -5,6 +5,7 @@ go 1.12
require (
github.com/beorn7/perks v1.0.1 // indirect
github.com/golang/mock v1.3.1
github.com/google/uuid v1.1.1
Copy link
Member

Choose a reason for hiding this comment

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

Did you run go mod vendor? Not sure if this should cause a change in vendor forlder

Copy link
Contributor Author

@krzysied krzysied Jan 27, 2020

Choose a reason for hiding this comment

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

I realized that uuid was used during implementation, but I removed it from the final version...
I'm removing go.mod/go.sum changes.

Also, it seems that I messed some vendor update in one of the previous commit :/ The change is unrelated to current PR, so I will fix it in separate PR (#2770).

@krzysied krzysied force-pushed the vpa_admission_controller_status2 branch from 7de0eb1 to 91b9553 Compare January 27, 2020 10:02
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 27, 2020
@bskiba
Copy link
Member

bskiba commented Jan 27, 2020

/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 Jan 27, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba

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 Jan 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit 25fa253 into kubernetes:master Jan 27, 2020
@krzysied krzysied deleted the vpa_admission_controller_status2 branch January 27, 2020 13:04
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

3 participants