-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement multiple version in instance group manager #1499
Changes from 1 commit
0f15d93
ab6a795
07dc7a2
bae2703
e1d8f97
427c45a
1060031
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,10 +33,40 @@ func resourceComputeInstanceGroupManager() *schema.Resource { | |
|
||
"instance_template": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
Optional: true, | ||
DiffSuppressFunc: compareSelfLinkRelativePaths, | ||
}, | ||
|
||
"version": &schema.Schema{ | ||
Type: schema.TypeList, | ||
Optional: true, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"name": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
}, | ||
|
||
"instance_template": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
DiffSuppressFunc: compareSelfLinkRelativePaths, | ||
}, | ||
|
||
"target_size_fixed": &schema.Schema{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two comments here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was not able to set But I don't see how I can make it work for target_size (which is a list of 1 element) inside version (which is a list too).
My feeling is that I need to find the right indice to edit the correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, you're right- that isn't possible at the moment. |
||
Type: schema.TypeInt, | ||
Optional: true, | ||
}, | ||
|
||
"target_size_percent": &schema.Schema{ | ||
Type: schema.TypeInt, | ||
Optional: true, | ||
ValidateFunc: validation.IntBetween(0, 100), | ||
}, | ||
}, | ||
}, | ||
}, | ||
|
||
"name": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
|
@@ -254,6 +284,7 @@ func resourceComputeInstanceGroupManagerCreate(d *schema.ResourceData, meta inte | |
NamedPorts: getNamedPortsBeta(d.Get("named_port").([]interface{})), | ||
TargetPools: convertStringSet(d.Get("target_pools").(*schema.Set)), | ||
AutoHealingPolicies: expandAutoHealingPolicies(d.Get("auto_healing_policies").([]interface{})), | ||
Versions: expandVersions(d.Get("version").([]interface{})), | ||
// Force send TargetSize to allow a value of 0. | ||
ForceSendFields: []string{"TargetSize"}, | ||
} | ||
|
@@ -290,6 +321,23 @@ func flattenNamedPortsBeta(namedPorts []*computeBeta.NamedPort) []map[string]int | |
|
||
} | ||
|
||
func flattenVersions(versions []*computeBeta.InstanceGroupManagerVersion) []map[string]interface{} { | ||
result := make([]map[string]interface{}, 0, len(versions)) | ||
for _, version := range versions { | ||
versionMap := make(map[string]interface{}) | ||
versionMap["name"] = version.Name | ||
versionMap["instance_template"] = ConvertSelfLinkToV1(version.InstanceTemplate) | ||
if value := version.TargetSize.Percent; value > 0 { | ||
versionMap["target_size_percent"] = version.TargetSize.Percent | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you want to make this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, my bad! |
||
} else { | ||
versionMap["target_size_fixed"] = version.TargetSize.Fixed | ||
} | ||
result = append(result, versionMap) | ||
} | ||
|
||
return result | ||
} | ||
|
||
func getManager(d *schema.ResourceData, meta interface{}) (*computeBeta.InstanceGroupManager, error) { | ||
config := meta.(*Config) | ||
|
||
|
@@ -352,6 +400,7 @@ func resourceComputeInstanceGroupManagerRead(d *schema.ResourceData, meta interf | |
|
||
d.Set("base_instance_name", manager.BaseInstanceName) | ||
d.Set("instance_template", ConvertSelfLinkToV1(manager.InstanceTemplate)) | ||
d.Set("version", flattenVersions(manager.Versions)) | ||
d.Set("name", manager.Name) | ||
d.Set("zone", GetResourceNameFromSelfLink(manager.Zone)) | ||
d.Set("description", manager.Description) | ||
|
@@ -505,6 +554,74 @@ func resourceComputeInstanceGroupManagerUpdate(d *schema.ResourceData, meta inte | |
d.SetPartial("instance_template") | ||
} | ||
|
||
// If version changes then update | ||
if d.HasChange("version") { | ||
manager := &computeBeta.InstanceGroupManager{ | ||
Versions: expandVersions(d.Get("version").([]interface{})), | ||
} | ||
|
||
op, err := config.clientComputeBeta.InstanceGroupManagers.Patch(project, zone, d.Id(), manager).Do() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still not totally sure this will work. For rolling updates, wouldn't we want the UpdatePolicy in the same patch call that we're actually making the update in? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It works because the first Patch (the one triggered if "version" field changes) updates versions and their parameters (target size and instance template) but does not trigger the update (as it is considered as an opportunistic update). The second Patch (called through performUpdate method) will trigger a proactive update. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like |
||
if err != nil { | ||
return fmt.Errorf("Error updating managed group instances: %s", err) | ||
} | ||
|
||
err = computeSharedOperationWait(config.clientCompute, op, project, "Updating managed group instances") | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if d.Get("update_strategy").(string) == "RESTART" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same code as in the Also, can you add a comment or something that makes it clear that yes, this does need to be called? From https://cloud.google.com/compute/docs/instance-groups/updating-managed-instance-groups#starting_a_canary_update it looks like all that needs to be done is that first Patch call updating the version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just made the test again, I am not sure why it is not working, I see in Cloud Console that update parameters are well updated, I see the correct target number of instances in versions, but it looks like the update part is not triggered... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Template Current size Target size There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which test isn't working? At this commit, all the TestAccRegionInstanceGroupManager tests seem to be passing for me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests are not failing but I wasn't really satisfied with all this code to perform an update. However, I just realized that API updates were treated as opportunistic updates by default (and I expected a proactive update). That's why this code portion is needed. I just refactored it and commented to make it clear ! |
||
managedInstances, err := config.clientComputeBeta.InstanceGroupManagers.ListManagedInstances(project, zone, d.Id()).Do() | ||
if err != nil { | ||
return fmt.Errorf("Error getting instance group managers instances: %s", err) | ||
} | ||
|
||
managedInstanceCount := len(managedInstances.ManagedInstances) | ||
instances := make([]string, managedInstanceCount) | ||
for i, v := range managedInstances.ManagedInstances { | ||
instances[i] = v.Instance | ||
} | ||
|
||
recreateInstances := &computeBeta.InstanceGroupManagersRecreateInstancesRequest{ | ||
Instances: instances, | ||
} | ||
|
||
op, err = config.clientComputeBeta.InstanceGroupManagers.RecreateInstances(project, zone, d.Id(), recreateInstances).Do() | ||
if err != nil { | ||
return fmt.Errorf("Error restarting instance group managers instances: %s", err) | ||
} | ||
|
||
// Wait for the operation to complete | ||
err = computeSharedOperationWaitTime(config.clientCompute, op, project, managedInstanceCount*4, "Restarting InstanceGroupManagers instances") | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
if d.Get("update_strategy").(string) == "ROLLING_UPDATE" { | ||
// UpdatePolicy is set for InstanceGroupManager on update only, because it is only relevant for `Patch` calls. | ||
// Other tools(gcloud and UI) capable of executing the same `ROLLING UPDATE` call | ||
// expect those values to be provided by user as part of the call | ||
// or provide their own defaults without respecting what was previously set on UpdateManager. | ||
// To follow the same logic, we provide policy values on relevant update change only. | ||
manager := &computeBeta.InstanceGroupManager{ | ||
UpdatePolicy: expandUpdatePolicy(d.Get("rolling_update_policy").([]interface{})), | ||
} | ||
|
||
op, err = config.clientComputeBeta.InstanceGroupManagers.Patch(project, zone, d.Id(), manager).Do() | ||
if err != nil { | ||
return fmt.Errorf("Error updating managed group instances: %s", err) | ||
} | ||
|
||
err = computeSharedOperationWait(config.clientCompute, op, project, "Updating managed group instances") | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
d.SetPartial("version") | ||
} | ||
|
||
// If named_port changes then update: | ||
if d.HasChange("named_port") { | ||
|
||
|
@@ -647,6 +764,28 @@ func expandAutoHealingPolicies(configured []interface{}) []*computeBeta.Instance | |
return autoHealingPolicies | ||
} | ||
|
||
func expandVersions(configured []interface{}) []*computeBeta.InstanceGroupManagerVersion { | ||
versions := make([]*computeBeta.InstanceGroupManagerVersion, 0, len(configured)) | ||
for _, raw := range configured { | ||
data := raw.(map[string]interface{}) | ||
version := computeBeta.InstanceGroupManagerVersion{ | ||
Name: data["name"].(string), | ||
InstanceTemplate: data["instance_template"].(string), | ||
} | ||
if v := data["target_size_percent"]; v.(int) > 0 { | ||
version.TargetSize = &computeBeta.FixedOrPercent{ | ||
Percent: int64(v.(int)), | ||
} | ||
} else { | ||
version.TargetSize = &computeBeta.FixedOrPercent{ | ||
Fixed: int64(data["target_size_fixed"].(int)), | ||
} | ||
} | ||
versions = append(versions, &version) | ||
} | ||
return versions | ||
} | ||
|
||
func expandUpdatePolicy(configured []interface{}) *computeBeta.InstanceGroupManagerUpdatePolicy { | ||
updatePolicy := &computeBeta.InstanceGroupManagerUpdatePolicy{} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this and
instance_template
within it are also going to need to beComputed
. When I run tests that don't set a version, it seems to want to fill one in:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made
version
optional and computed, but I cannot makeinstance_template
Computed and Required, shouldn't we leave it Required ? (it is required within the version)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, that should be fine then. I'll run the test again and check it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran tests and still hit the perpetual diff issue. Making version Optional & Computed is apparently not enough.
Here is the error:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to make tests pass by removing "Computed" flag, and now I see only one test failing (the one I wrote), because of the ImportStateVerify flag in my TestStep.
Do I need to add something to make version fields importable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some debugging on my end and was able to get it working- the d.Set call for versions was failing, which is why the import test failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thank you, I should have catch this one, this is all my bad! Sorry for that!