Rolling update support for instance group manager

[ishashchuk]

Addresses 51

Supports beta functionality for rolling updates.

When rolling_update is selected as update_strategy, it requires "rolling_update_policy" block.
I'm storing rolling_update_policy block locally, and not applying it to computeBeta.InstanceGroupManager. There is no way to make an update call for this block on the InstanceGroupManager. It is only updated when the patch call for the instances is made. That's the reason I'm storing it locally and only applying it on the update calls that are changing the instance template. That allows us to change rolling_update_policy arguments post creating InstanceGroupManager and making sure that they will be applied at the next patch call.

Once I get feedback on this, I will add similar support to region_instance_group_manager

[apenney]

@ishashchuk I don't have any code feedback, I just wanted to thank you for tackling this (as we'll be early adopters of it, soon as it's merged).

[apenney]

@ishashchuk I don't have any code feedback, I just wanted to thank you for tackling this (as we'll be early adopters of it, soon as it's merged).

[amoiseiev]

@paddycarver friendly reminder :)

[amoiseiev]

@paddycarver friendly reminder :)

[ndmckinley]

I'm storing rolling_update_policy block locally, and not applying it to computeBeta.InstanceGroupManager. There is no way to make an update call for this block on the InstanceGroupManager. It is only updated when the patch call for the instances is made. That's the reason I'm storing it locally and only applying it on the update calls that are changing the instance template.

I like this... but I wonder, what happens if someone:

• creates the resource with RESTART
• changes it to ROLLING_UPDATE
• does an update
• changes it back to RESTART
• does another update

In that case, would this code act correctly? It seems like it wouldn't send a PATCH request to remove the strategy, but I might be off.

[ndmckinley]

Nit: Seems like it'd be neater as part of the if block below.

[ndmckinley]

same comment - include in the if statement if possible.

[ishashchuk]

I like this... but I wonder, what happens if someone:

• creates the resource with RESTART
• changes it to ROLLING_UPDATE
• does an update
• changes it back to RESTART
• does another update

In that case, would this code act correctly? It seems like it wouldn't send a PATCH request to remove the strategy, but I might be off.

@ndmckinley
There is no need to remove the strategy, since it only matters on PATCH requests. PATCH request is using a different API endpoint(clientComputeBeta.InstanceGroupManagers.Patch) and is only used when the resource is set to ROLLING_UPDATE. And we will always get the most up-to-date strategy from the config, because we are passing it explicitly on template change

When resource is set to RESTART, config.clientCompute.InstanceGroupManagers.RecreateInstances is used and that endpoint doesn't care about update_strategy attributes. It reboots all the machines at the same time.

[ishashchuk]

I like this... but I wonder, what happens if someone:

• creates the resource with RESTART
• changes it to ROLLING_UPDATE
• does an update
• changes it back to RESTART
• does another update

In that case, would this code act correctly? It seems like it wouldn't send a PATCH request to remove the strategy, but I might be off.

@ndmckinley
There is no need to remove the strategy, since it only matters on PATCH requests. PATCH request is using a different API endpoint(clientComputeBeta.InstanceGroupManagers.Patch) and is only used when the resource is set to ROLLING_UPDATE. And we will always get the most up-to-date strategy from the config, because we are passing it explicitly on template change

When resource is set to RESTART, config.clientCompute.InstanceGroupManagers.RecreateInstances is used and that endpoint doesn't care about update_strategy attributes. It reboots all the machines at the same time.

[ndmckinley]

Ah! That's quite nice, then, neat trick. I think (but am not 100% sure) that I'd like it to be set during Create as well as during Update, then, because in the event that someone creates a resource with Terraform then runs a ROLLING_UPDATE manually or using some other tool, I think the most sensible behavior is to default to what they specified in Terraform. Does that seem right to you?

[ndmckinley]

Ah! That's quite nice, then, neat trick. I think (but am not 100% sure) that I'd like it to be set during Create as well as during Update, then, because in the event that someone creates a resource with Terraform then runs a ROLLING_UPDATE manually or using some other tool, I think the most sensible behavior is to default to what they specified in Terraform. Does that seem right to you?

[ishashchuk]

@ndmckinley
I don't think there is much value in setting update_strategy on CREATE, because most of the tools are either expect the user to provide those values, or use their own defaults.

In UI, the values for ROLLING RESTART/REPLACE are pre-populated with defaults, which means that the values instantiated on terraform create won't be respected

Running gcloud beta compute instance-groups managed rolling-action without specifying strategy, use their own defaults too without respecting anything that was previously set on instance manager

[ishashchuk]

@ndmckinley
I don't think there is much value in setting update_strategy on CREATE, because most of the tools are either expect the user to provide those values, or use their own defaults.

In UI, the values for ROLLING RESTART/REPLACE are pre-populated with defaults, which means that the values instantiated on terraform create won't be respected

Running gcloud beta compute instance-groups managed rolling-action without specifying strategy, use their own defaults too without respecting anything that was previously set on instance manager

[ndmckinley]

Fair enough - can you add a comment to that effect? Just that and I'll be ready to merge.

[ndmckinley]

Fair enough - can you add a comment to that effect? Just that and I'll be ready to merge.

[ishashchuk]

Should be good to go

[ishashchuk]

Should be good to go

[ndmckinley]

Perfect. I'm running the tests now, and I expect to merge in an hour or so assuming no problems.

[ndmckinley]

Perfect. I'm running the tests now, and I expect to merge in an hour or so assuming no problems.