feat(rollback): avoid scaling down during rollback #3185
Conversation
Should partly address spinnaker/spinnaker#4895 by avoiding resizing the restore server group down. This does not address concurrent actions issues, so it is probably still possible to have race conditions result in bad things. So fundamentally this changes: * computes the max of the current restore server group size and the rollback server group size * only generates a resize stage if that computed size is bigger than the current size This also opens the possibility to skip the snapshot/restoreMin tasks if we are not going to resize (which would pin the min of the restore server group).
|
cc @luispollo |
| @@ -56,6 +56,13 @@ interface ResizeStrategy { | |||
| Integer max | |||
| Integer desired | |||
| Integer min | |||
|
|
|||
| // from https://codereview.stackexchange.com/questions/8801/returning-groovy-class-fields-as-a-map | |||
| public Map asMap() { | |||
There was a problem hiding this comment.
I think you can just call .properties() to get the value as a map?
There was a problem hiding this comment.
or I guess that has some groovy magic-junk in it?
There was a problem hiding this comment.
I get this though
MissingMethodException: No signature of method: Capacity.properties() is applicable for argument types: () values: []
There was a problem hiding this comment.
If you're still interested, it's a property, not a method:
> import com.netflix.spinnaker.orca.kato.pipeline.support.ResizeStrategy.Capacity
>
> Capacity c = new Capacity(3,2,1)
> print(c.properties)
[class:class com.netflix.spinnaker.orca.kato.pipeline.support.ResizeStrategy$Capacity, max:3, min:1, desired:2]
|
cc @gal-yardeni |
| targetLocation : restoreServerGroup.getLocation(), | ||
| account : restoreServerGroup.credentials, | ||
| cloudProvider : restoreServerGroup.cloudProvider, | ||
| pinMinimumCapacity : true, |
There was a problem hiding this comment.
wy do you need this if you already set min=desired
There was a problem hiding this comment.
it's not needed, I thought I would just leave it like that to express the intent anyway
| Map resizeServerGroupContext = new HashMap(parentStage.context) + [ | ||
| action : ResizeStrategy.ResizeAction.scale_exact.toString(), | ||
| capacity : newRestoreCapacity.asMap(), | ||
| asgName : restoreServerGroupName, |
There was a problem hiding this comment.
add serverGroupName?
There was a problem hiding this comment.
I don't like it, but I will do it
| ] | ||
|
|
||
| return newStage(parentStage.execution, resizeServerGroupStage.type, | ||
| "Resize Restore Server Group to (min: ${newRestoreCapacity.min}, max: ${newRestoreCapacity.max}, desired: ${newRestoreCapacity.desired})", |
There was a problem hiding this comment.
nit: Resize Restore is pretty confusing wording.. maybe Resize {serverGroupaName}...?
| fromContext.location | ||
| ).orElse(null) | ||
| } catch(Exception e) { | ||
| log.error('Skipping resize stage because there was an error looking up {}', serverGroupName, e) |
There was a problem hiding this comment.
i think this error would be better in line 157/162.
that way you can differentiate between failure in asg lookup vs there legitimately not being an ASG
There was a problem hiding this comment.
chatted offline, will change the way we log when we get an empty optional
| newRestoreCapacity.min = newRestoreCapacity.desired | ||
|
|
||
| ResizeStrategy.Capacity currentCapacity = restoreServerGroup.capacity | ||
| if (currentCapacity == newRestoreCapacity) { |
There was a problem hiding this comment.
Shouldn't this be comparing with .equals() instead?
There was a problem hiding this comment.
it is Groovy so it does some magic behind the scenes that ultimately does call .equals()
There was a problem hiding this comment.
Actually I think it's because of the way that @Canonical annotation is implemented (they use property values for hashCode() so .equals() ends up being the same as ==). But that's TMI. 😄
| return stages | ||
| } | ||
|
|
||
| @Nullable TargetServerGroup lookupServerGroup(Stage parentStage, String serverGroupName) { |
There was a problem hiding this comment.
nit: Wouldn't mind seeing @Nonnull etc added to the method args, too.
There was a problem hiding this comment.
do we have an explicit convention around this or tooling that actually takes advantage of that?
Otherwise it seems overkill, for me the implicit contract is that things that are not explicitly @Nullable must be @Nonnull
|
@dreynaud Thanks a lot for tagging me for review. Still very early days for me so as you could see, not a lot of insightful comments, but for whatever it's worth, your changes made sense to me. |
|
Thanks for taking the time to look things over! 🙏
|
Should partly address spinnaker/spinnaker#4895 by avoiding resizing the restore server group down.
This does not address concurrent actions issues, so it is probably still possible to have race conditions result in bad things.
So fundamentally this changes:
This also opens the possibility to skip the snapshot/restoreMin tasks if we are not going to resize (which would pin the min of the restore server group).