-
Notifications
You must be signed in to change notification settings - Fork 4k
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 a multi-dimensional pod autoscaler #7550
base: master
Are you sure you want to change the base?
Conversation
Welcome @davidspek! |
Hi @davidspek. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: davidspek The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @jbartosik since you reviewed the original PR. |
891012a
to
7321b16
Compare
@adrianmoisey Do you think you could have a look at this? |
I'm happy to take a look, but I can't approve it. I do have 2 initial concerns:
I don't speak on behalf of sig-autoscaling though. |
Regarding the overlap with VPA, there is definitely some overlap, but I believe part of that is just scripts which can be deduplicated in the future. @James-QiuHaoran can likely provider better comment here as he implemented the original PR. In terms of maintenance, that will always be a difficult question to answer at the start of something new. I revived this PR since there was an approved |
I think this is worth bring up in the sig-autoscaling meeting, just to see how others feel about it |
Thanks a lot David! I'm having trouble loading all the files in this PR :( Wonder if it is possible to split out the vendor files into a different PR? I'd really love to dig into the logic of how the vertical and horizontal autoscaling recommendations are generated and actuated since the original AEP skims over this part a bit. +1 to Adrian, we should discuss this in the SIG meeting - if anything to raise awareness and gauge additional interest of folks who might want to use this or help out! Some random thoughts on maintenance:
|
Even temporarily while we look at the core logic, then we can bring vendored files back in afterwards. |
This would be very helpful!
I'm also going to focus on this
I was wondering about this too. Putting it into this repo may make it "official", so calling it out as experimental (at least for now) may help One thing I did notice so far is that this doesn't seem to have e2e tests. I think it makes sense to try get decent test coverage for a new tool, especially if we're low on maintainers. |
Thanks for the replies. I think we're all on the same line here. My intention was also to get this contribution into the mix to be able to properly gauge interest, which is difficult without it actually being a thing that is released and people can play with. At the same time, it most definitely should be released as an alpha or beta without any guarantees with very clear messaging on this. I also don't think this should be burdened on the maintainers of the VPA, and should be released independently. However, if this proves to be popular it might make sense to do a joined refactor with the VPA so more code can be reused and forces can be joined. Personally, I could see a world where the MPA becomes more popular than VPA, since it also does horizontal scaling which is the most popular form of autoscaling. In terms of end-to-end testing, I agree that is something which definitely should be added. However, as I fear my time for this will probably be quite lacking and writing good tests takes some time, I hope it would be possible to have this also be separated from the initial contribution to hopefully get the ball rolling a bit. I've also removed the vendored files so it should be easier to review the PR now. |
err = api_util.CreateOrUpdateMpaCheckpoint(writer.vpaCheckpointClient.VerticalPodAutoscalerCheckpoints(mpa.ID.Namespace), &vpaCheckpoint) | ||
if err != nil { | ||
klog.Errorf("Cannot save MPA %s/%s checkpoint for %s. Reason: %+v", | ||
mpa.ID.Namespace, vpaCheckpoint.Spec.VPAObjectName, vpaCheckpoint.Spec.ContainerName, err) | ||
} else { | ||
klog.V(3).Infof("Saved MPA %s/%s checkpoint for %s", | ||
mpa.ID.Namespace, vpaCheckpoint.Spec.VPAObjectName, vpaCheckpoint.Spec.ContainerName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on the VPA's CRDs to be installed in the cluster.
Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually looking at this myself as well and something I tried to verify but was running into an issue with KinD where I could register the CRD nicely. The MPA was already working in that it scaled up the pod, but maybe it was working incorrectly. Do you think it makes sense to have a separate checkpoint CRD for the MPA? I'm also not sure if other code that is getting imported from the VPA might require the VPA checkpoint to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a project depend on another project's CRDs may create an awkward dependency between the two projects.
I'm not sure on the best way forward, may be for now it's worth noting that the MPA depends on having the VPA CRDs installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking it should be possible to import the type of the VPA checkpoint as the MPA checkpoint and generate it that way. I'll give that a quick try in a moment. If at some point the MPA and VPA start converging more it might make sense to just have the one resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now added the mpacheckpoint
CRD and should have the code migrated to that. This should allow somewhat cleaner separation, at least from a deployment standpoint.
a579fdb
to
a4feb37
Compare
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
// GetHumanizeMemory returns the value of the HumanizeMemory flag. | ||
func GetHumanizeMemory() bool { | ||
return *humanizeMemory | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what the purpose of this change is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed that for having accessible in https://github.com/kubernetes/autoscaler/pull/7550/files#diff-27dae50f34ff6f13aa0bcf2556de814c2d2e7aa2dbc277e1ee8339f60917622bR227-R230.
Revival of: #5999
What type of PR is this?
/kind feature
What this PR does / why we need it:
Multidimensional pod autoscaling: https://github.com/kubernetes/autoscaler/blob/master/multidimensional-pod-autoscaler/AEP.md
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: