Repository navigation
Conversation
This reverts commit b3a741d.
This reverts commit 1ce3bc5.
This reverts commit b3a741d.
fix(controller): revert release.check_image_access for now
This reverts commit b3a741d.
This reverts commit 1ce3bc5.
This reverts commit b3a741d.
This is basically just manually re-applying the reverted changes from teamhephy#86
This functionality now lives in KubeHTTPClient, so it is unnecessary to mutate the reported k8s API version in the deployment resource.
Refactoring for new KubeHTTPClient.version() behavior
Upgrading to apps/v1 requires changes to the way that Scale action is called, and as we haven't understood this change yet, we will depend on the older version of Deployment api to remain available until we do. includes these squashed commits: * WIP: this commit is probably wrong * WIP: this seems more likely to work
* WIP: hardcode extensions/v1beta1 here for now There is no "App.api_version" attribute and I'm not sure where this information should come from * fixup: line was too long for the style guide
kingdonb
force-pushed
the
kb-fix-autoscale-take-3
branch
from
August 6, 2019 20:14
4cf2e01 to
ce6f29b
Compare
Member
Author
|
I rebased this with some |
Member
Author
|
We can probably also take this to a new branch and do some commit squashing to improve the history, for the changelog. |
Contributor
|
Tested this on an EKS cluster running 1.13, verified as working. |
Cryptophobia
self-requested a review
August 8, 2019 14:35
Member
Cryptophobia
approved these changes
Aug 8, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a WIP
We've been testing changes to make the Autoscale functionality via HPA work again.
Right now it looks like this image (
quay.io/kingdonb/controller:kb-fix-autoscale-take-5) is successfully permitting a manual scale as well as an HPA to be created, like:The concession we had to make is that we don't know what has changed with manual scaling in
apps/v1deployment, so we have left it atextensions/v1beta1API for deployments.The
autoscaler/v1HorizontalPodAutoscaler will only find the Deployment as a match if it is also provided with ascaleTargetRef.apiVersionselector that matches. I added that here, and HPA now succeeds, assuming you have metrics-server properly configured so the HPA can get pod metrics to make scale decisions.I also tested the manual
deis ps:scale -a testing-hephy cmd=1behavior, which was broken by the last attempt we made to upgrade toautoscaler/v1. We should make any reasonable efforts to upgrade the deployment toapps/v1, but that is not necessary in order to fix the autoscale feature (without unfortunately also breaking manual scaling.) This PR seems to do both 👍!The other frankly unacceptable concession I had to make was in
rootfs/api/models/app.pywhere the HPA is created, I have hardcoded'apiVersion': 'extensions/v1beta1'as I'm not sure how the App resource is supposed to find out what the Deployment's api_version ought to be.We discussed maybe adding an API version service that you can use from any module to determine what API versions are in use for what objects, but there is more discussion that needs to happen before it's clear what that piece ought to look like.
Also, since upgrading a resource's API version means necessarily destroying the old resource, we are going to need a bigger plan when it comes to upgrading deployments to
apps/v1, since that step appears to be unavoidable, we might need to write code so that a cluster can recognize when these deployment upgrades are needed, and to handle them in a reasonable way. I'm not sure how this can be done without potentially causing apps to have downtime.(It should be perfectly safe to delete and recreate HPAs though, so for the upgrade guide, maybe we give that a little mention and this can go to press relatively as-is in the next release.)