FleetAutoscaler can be targeted at Non Existent Fleets #416
Conversation
markmandel
added
kind/cleanup
|
/cc @victor-prodan - I made some decisions here, just to get a sacrificial draft in place -- please definitely let me know if you see issues. |
|
Build Succeeded 👏 Build Id: 47c6b1ee-3227-4561-87cd-8eed8d1e98a5 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
| if fas.Spec.Policy.Type == BufferPolicyType { | ||
| causes = fas.Spec.Policy.Buffer.ValidateAutoScalingBufferPolicy(causes) | ||
| } | ||
| return causes | ||
| } | ||
|
|
||
| //ValidateAutoScalingSettings validates the FleetAutoscaler Buffer policy settings | ||
| //Validate validates the FleetAutoscaler Buffer policy settings | ||
| func (b *BufferPolicy) ValidateAutoScalingBufferPolicy(causes []metav1.StatusCause) []metav1.StatusCause { |
There was a problem hiding this comment.
I think you meant to change the name of the function too.
There was a problem hiding this comment.
Actually no - but that comment shouldn't be changed. Weird the linter didn't pick that up?
There was a problem hiding this comment.
Fixed comment. Intellij got a little aggressive with an automatic refactor. Thanks for the pickup!
There was a problem hiding this comment.
Or actually, are you saying this should be something like ValidateBufferPolicy ? (That would make more sense)
There was a problem hiding this comment.
Gentle bump on this - I think this is the last issue - and would love to get this in before RC release tomorrow.
There was a problem hiding this comment.
I just wanted the name to match. ValidateBufferPolicy works for me.
There was a problem hiding this comment.
SGTM! Will make the change. Thanks for the quick response.
markmandel
force-pushed
the
bug/decouple-fa
branch
from
ab3eed5
to
b172344
Compare
|
Build Succeeded 👏 Build Id: 2f5d47b2-c287-4d35-a29d-85d24dcd9742 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
force-pushed
the
bug/decouple-fa
branch
from
b172344
to
75f01bd
Compare
|
Build Succeeded 👏 Build Id: b7c9dbb4-d385-493b-aac3-a3f2ac06c5d4 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
force-pushed
the
bug/decouple-fa
branch
from
75f01bd
to
79c0a68
Compare
|
Build Succeeded 👏 Build Id: d927e64f-01e2-4197-bdb5-05db27a0e54a The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
This removes some previous restrictions with the FleetAutoscaler,
as well as cleaning up a couple of items.
- FleetAutoscalers can now have a Fleet target that doesn't exit.
- If the target doesn't exist, this will be recorded as a warning event
of type `FailedGetFleet`, as it allows a human readable message, as
well as give us data on how often this occurs, and it is readable by
stat/alerting packages.
- FleetAutoscalers can now have their Fleet target edited. This
means that FleetAutoscalers can be transitioned from one Fleet
to another. This may be useful in scenarios like red-green deployments, or
if you wish to temporarily disable an Autoscaler, but not delete it.
- `FleetAutoscaler.Status.ScalingLimited` has an added
Event, as it allows a human readable message, as well as give us data on how
often this occurs, and it is readable by stat/alerting packages.
Closes googleforgames#406
markmandel
force-pushed
the
bug/decouple-fa
branch
from
79c0a68
to
cd2b7c8
Compare
|
Build Succeeded 👏 Build Id: f5425a52-ddbc-4039-96c6-9e2ca84b861d The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
This removes some previous restrictions with the FleetAutoscaler,
as well as cleaning up a couple of items.
FailedGetFleet, rather thanScalingLimitedstatus item, as it allows a human readable message, as well as give us data on how often this occurs, and it is readable by stat/alerting packages.FleetAutoscaler.Status.ScalingLimitedwas also replaced by aScalingLimitedEvent, as it allows a human readable message, as well as give us data on how often this occurs, and it is readable by stat/alerting packages.Closes #406