feat(autoscale): add autoscaling support to application on per proc type basis#1018
feat(autoscale): add autoscaling support to application on per proc type basis#1018helgi merged 1 commit intodeis:masterfrom
Conversation
|
@kmala, @bacongobbler and @mboersma are potential reviewers of this pull request based on my analysis of |
Current coverage is 87.01% (diff: 81.01%)@@ master #1018 diff @@
==========================================
Files 41 41
Lines 3473 3542 +69
Methods 0 0
Messages 0 0
Branches 579 600 +21
==========================================
+ Hits 3027 3082 +55
- Misses 291 301 +10
- Partials 155 159 +4
|
| return | ||
|
|
||
| # if nothing changed copy the settings from previous | ||
| if new is None and data is not None: |
There was a problem hiding this comment.
since the default is {} i don't think this check passes.
There was a problem hiding this comment.
True - pushing change
helgi
force-pushed
the
hpa
branch
2 times, most recently
from
0de4562 to
c402aef
Compare
| changes = [] | ||
| old_autoscale = getattr(previous_settings, 'autoscale', {}) | ||
| diff = dict_diff(self.autoscale, old_autoscale) | ||
| # try to be as succinct as possible |
There was a problem hiding this comment.
the following lines take a while to grok. any chance they can be split up so they're more understandable?
There was a problem hiding this comment.
That's a copy pasta of our Config / Release summary (and other things) :| I had talked to @bacongobbler about doing a more unified way of doing changelog summaries (if possible) across multiple models. Will refactor the below during that.
|
g2g, except for #1018 (comment). I wouldn't consider that blocking for this PR though |
| raise UnprocessableEntity('{} does not exist under {}'.format(proc, 'autoscale')) # noqa | ||
| del data[proc] | ||
| else: | ||
| data[proc] = scale |
There was a problem hiding this comment.
this depends a lot on how the cli sends the data....can user edit the hpa configuration and if so what does the cli ux looks like?
There was a problem hiding this comment.
deis autoscale:set cmd --min=3 --max=5 --cpu-percent=60 -a test-app
There was a problem hiding this comment.
So they can update / edit an existing rule by applying a full rule again with the same proc type
There was a problem hiding this comment.
i am interested about edit....if he wants to edit max to 10 after he set it using the above command, what should he do?
There was a problem hiding this comment.
Runs the exact same command as above but changes the max
There was a problem hiding this comment.
yes...but i think handling them is better asking user to enter the whole thing again...right now there are only 3 parameters but in future it might grow into more right?
can we override create method https://docs.djangoproject.com/en/1.10/ref/models/instances/#django.db.models.Model to copy previous into current if its get called before the serializers but am not sure if it works.
There was a problem hiding this comment.
Inclined to say we solve that problem when we get to it unless we have a clear cut way towards that right now. @slack ?
There was a problem hiding this comment.
@kmala how do healthchecks behave in this same situation? They are far more complicated tbh but still
There was a problem hiding this comment.
it has only one mandatory argument(port/command) and remaining are optional...even there to edit you have to enter the mandatory argument again
There was a problem hiding this comment.
going by the kubectl autoscale help (I have to review HPA again to double check if it is identical) then max is the only required element, as such, but if we made that even optional to allow editing only min or only cpu % then we'd have to do something smart on the Controller side and have the CLI understand that as well
|
I don't have a strong opinion. The "override/set single value" UX is nice as long as the implementation doesn't become unruly. Seems that there are two options, driven mostly by "what happens when you first create this thing without some values": What does k8s do if the values are not set? I assume it takes some default value? If we could look up those defaults and store them off on the app/proc-type after creating the HPA, that would probably be preferable to a masked set of values. That could confuse the operator. The other option would be for Workflow to hold HPA defaults for unspecified values. Benefit there is that you would get the same thing every time, no matter the k8s version. I think there is a strong argument there as well. |
|
And to be fair. I'm okay with the "give me all of the values every time" even though it is annoying... I think there is going to be a bit of time before we declare autoscaling stable... |
helgi
force-pushed
the
hpa
branch
3 times, most recently
from
cf7e86b to
39ad346
Compare
| self._scheduler.hpa.update( | ||
| self.id, name, proc_type, target, **autoscale | ||
| ) | ||
| except KubeHTTPException as e: |
There was a problem hiding this comment.
shouldn't we re-throw/raise the exception if its not 404.
There was a problem hiding this comment.
Yeah and looking at it I should split this into 2 separate things
|
https://ci.deis.io/job/workflow-test-pr/6082/console finished fine even if it didn't get reported back here |
helgi
force-pushed
the
hpa
branch
3 times, most recently
from
8b70f3d to
813b2a2
Compare
|
e2e went green https://ci.deis.io/job/workflow-test-pr/6212/console |
7 participants
Uh oh!
There was an error while loading. Please reload this page.