New Resource: aws_emr_instance_fleet #1475

jmsantorum · 2017-08-22T19:18:30Z

I'm not sure it the acceptance tests are done in the best way.

It's my first PR on this project and any feedback is welcome

stekiri · 2017-11-28T21:12:52Z

aws/resource_aws_emr_instance_fleet.go

+				ForceNew: true,
+				Elem:     configurationSchema(),
+			},
+			"ebs_configuration": {


What do you think about reusing the schema for ebs_config that is already used in the instance_group? In that case, you could add the ebs_optimized variable on this hierarchy level.

makes sense! I tried to maintain the same hierarchy used in EMR but the schema used on instace_group is easier ;)

jmsantorum · 2017-11-30T22:29:11Z

Fixed some issues that I found & rebased

stekiri · 2017-12-01T21:03:10Z

aws/resource_aws_emr_instance_fleet.go

+				Type:     schema.TypeInt,
+				Optional: true,
+				ForceNew: true,
+				Default:  60,


I think the default should be 0. See Amazon doc (http://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-instance-fleet.html):

If you don't specify a defined duration, instances terminate as soon as the Spot price exceeds the maximum Spot price.

Right! The problem was a misunderstood reading the valid values for that parameter where it says (60, 120, ...). But it is not a mandatory parameter, so, default value setted to 0.

Thanks!

stekiri · 2017-12-04T13:29:34Z

When using instance fleets, it is possible to define a list of Subnets (RequestedEc2SubnetIds) or Availability Zones (RequestedEc2AvailabilityZones) to specify where to launch the cluster (see http://docs.aws.amazon.com/emr/latest/APIReference/API_Ec2InstanceAttributes.html). Can these two attributes be integrated in the ec2_attributes object?

stekiri · 2017-12-04T14:44:44Z

aws/resource_aws_emr_instance_fleet.go

+				Type:     schema.TypeString,
+				Optional: true,
+			},
+			//"configurations": {


Did it resolve in a loop issue or why did you comment out these lines?

yes! checking the documentation here we can define configurations inside configurations but when I defined it, it resolves in a loop issue trying to "compile" the schema. Any idea on how to fix this? Because I was looking in the code and I didn't find any alternative/solution.

The doc is not really clear, but it seems that no real recursion is required here. Limiting the nesting to one level should be enough. So, you could use a new schema (e.g. additionalConfigurationSchema) that only has a "classification" and "properties" attribute.

jmsantorum · 2017-12-05T16:50:36Z

Updated!

stekiri · 2017-12-05T22:01:09Z

aws/validators.go

@@ -916,6 +916,84 @@ func validateAwsEmrInstanceGroupRole(v interface{}, k string) (ws []string, erro
 	return
 }

+func validateAwsEmrSpotProvisioningTimeOutAction(v interface{}, k string) (ws []string, errors []error) {
+	validTypes := map[string]struct{}{
+		"SWITCH_TO_ON_DEMAND": {},


You could use the constant emr.SpotProvisioningTimeoutActionSwitchToOnDemand here and there are a couple of more places in this file and other files of this PR where you could use constants.

stekiri · 2017-12-05T22:05:44Z

aws/resource_aws_emr_instance_fleet.go

+	}
+}
+
+func ebsConfigurationSchema() *schema.Resource {


Shouldn't this schema be used by the instance group to avoid redundancy?

stekiri · 2017-12-05T22:11:35Z

You should document this feature also in the emr_cluster.html.md. It would be a pity if only the usage with the emr_instance_fleet resource was documented.

stekiri · 2017-12-08T15:12:38Z

website/docs/r/emr_instance_fleet.html.md

+
+* `timeout_action` - (Required) The action to take when `target_spot_capacity` has not been fulfilled when the 
+`timeout_durataion_minutes` has expired. Spot instances are not uprovisioned within the Spot provisioining timeout.
+Valid values are `TERMINATE_CLUSTER` and `SWITCH_TO_ON_DEMAND`. `SWITCH_TO_ON_DEMAND specifies that if no Spot 


Missing a backtick after the second SWITCH_TO_ON_DEMAND.

stekiri · 2017-12-08T15:12:43Z

website/docs/r/emr_instance_fleet.html.md

+Changing this forces a new resource to be created.
+
+* `instance_type_configs` - (Optional) The instance type configurations that define the EC2 instances in the instance fleet. 
+Array of `instance_type_config` blocks. 


Same here, no new line?

stekiri · 2017-12-08T15:12:45Z

website/docs/r/emr_instance_fleet.html.md

+* `cluster_id` - (Required) ID of the EMR Cluster to attach to. Changing this forces a new resource to be created.
+
+* `instance_fleet_type` - (Required) The node type that the instance fleet hosts. Valid values are `MASTER`, `CORE`, and `TASK`. 
+Changing this forces a new resource to be created.


No new line necessary?

stekiri · 2017-12-08T15:13:00Z

aws/resource_aws_emr_instance_fleet_test.go

+          ebs_optimized = true
+		  ebs_config = [
+			{
+              size = 10


The formatting of the resources seems to be muddled in some of the tests. It would be nice to fix this to improve readability.

stekiri · 2017-12-08T15:13:12Z

aws/resource_aws_emr_instance_fleet_test.go

+			{
+			  size = 100
+			  type = "gp2"
+              volumes_per_instance = 1


Just a bad alignment.

stekiri · 2017-12-08T15:13:39Z

aws/resource_aws_emr_cluster_test.go

+  	  	  ebs_configuration {
+  	  	    ebs_block_device_configs = [
+  	  	      {
+  	  	        volume_specification {


The EBS config definition used here is the old one. I'm surprised that this test still passes.

peay · 2018-07-03T17:15:50Z

@stekiri @radeksimko any chance this would get merged in, if we fix the conflicts?

krish7919 · 2018-07-09T13:03:12Z

Is this going to be merged anytime soon?

copumpkin · 2018-08-01T20:09:42Z

Another vote for merging (or otherwise explaining what needs to be done) cc @bflad

dthauvin · 2018-08-02T09:28:38Z

Another vote

peay · 2018-09-03T08:38:59Z

One more month has passed without any news that this could eventually get merged, this sends a worrying message. Trying one last round of mentions - @radeksimko @bflad what do you think?

peay · 2018-09-07T18:25:26Z

aws/resource_aws_emr_cluster.go

-				ebsBlockDeviceConfigs = append(ebsBlockDeviceConfigs, ebsBlockDeviceConfig)
-			}
-			ebsConfig.EbsBlockDeviceConfigs = ebsBlockDeviceConfigs
+		if v, ok := configAttributes["ebs_config"].(*schema.Set); ok && v.Len() == 1 {


Why silently ignore EBS configs with more than one volume with v.Len() == 1?

peay · 2018-09-09T14:54:02Z

aws/resource_aws_emr_instance_fleet.go

+		}
+
+		if rawConfig, ok := configAttributes["configurations"]; ok {
+			config.Configurations = expandConfigurations(rawConfig.([]interface{}))


Should the cast be rawConfig.(*schema.Set).List() instead?

marcelocorreia · 2018-11-06T00:58:09Z

is this one parked?

david-wells-1 · 2019-01-15T10:36:16Z

This is a really useful addition - any progress with its release?

david-wells-1 · 2019-01-31T11:11:25Z

@stekiri @radeksimko @bflad any chance this would get merged in?

dmitriy-lukyanchikov · 2019-02-12T08:00:57Z

emr instance fleet is really useful, we need it badly

plejik · 2019-02-28T10:00:50Z

If somebody has the opportunity to fix conflict here?

bflad · 2019-03-05T17:12:27Z

Hi @jmsantorum 👋 Can you please have this PR only handle the aws_emr_instance_fleet resource creation for now? We would like to get that in and address the other aws_emr_cluster and aws_emr_instance_group changes separately. Thanks.

jmsantorum · 2019-03-20T10:42:39Z

Hi @bflad 👋! Give me a couple of days. I'll update the PR with only the aws_emr_instance_fleet resource creation.

abhivijay89 · 2019-05-13T15:11:37Z

Any idea when this would get picked up?

sanoojm · 2019-05-23T11:02:43Z

@darrenhaken @copumpkin @jmsantorum I have all the conflicts fixed and I have synced with the latest master. I am using it in my preprod environment as well. How do I proceed to update this PR? @jmsantorum, can you give me access to push a new branch to your repo? If anyone knows a better way, please suggest.

jmsantorum · 2019-05-23T11:14:33Z

@sanoojm just added as collaborator to my repo

bflad · 2019-06-26T00:31:10Z

Closing this conflicted pull request as there has been no movement. Please feel free to submit a new one and we'll take a look. Thanks!

ghost · 2019-11-03T15:12:18Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Aug 28, 2017

jmsantorum force-pushed the issue-1146 branch from 3475ee3 to 2ee6156 Compare September 1, 2017 11:39

jmsantorum changed the title ~~[WIP] issue 1146 - Added instance fleet resource for AWS EMR~~ issue 1146 - Added instance fleet resource for AWS EMR Sep 1, 2017

jmsantorum force-pushed the issue-1146 branch from 2ee6156 to a239bd5 Compare September 1, 2017 11:58

radeksimko added the new-resource Introduces a new resource. label Oct 11, 2017

radeksimko added the size/XXL Managed by automation to categorize the size of a PR. label Nov 15, 2017

dthauvin mentioned this pull request Nov 21, 2017

Enable EMR cluster configuration through the Instance Fleet param #2391

Closed

stekiri reviewed Nov 28, 2017

View reviewed changes

jmsantorum force-pushed the issue-1146 branch from a239bd5 to a73d4aa Compare November 30, 2017 21:48

stekiri reviewed Dec 1, 2017

View reviewed changes

stekiri reviewed Dec 4, 2017

View reviewed changes

jmsantorum force-pushed the issue-1146 branch from d2bd180 to b280ba4 Compare December 5, 2017 16:43

stekiri reviewed Dec 5, 2017

View reviewed changes

jmsantorum force-pushed the issue-1146 branch from 13962dd to 5278ded Compare December 7, 2017 19:27

stekiri reviewed Dec 8, 2017

View reviewed changes

aws/resource_aws_emr_instance_fleet_test.go Outdated

{

size = 100

type = "gp2"

volumes_per_instance = 1

Copy link

stekiri Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a bad alignment.

stekiri reviewed Dec 8, 2017

View reviewed changes

jmsantorum force-pushed the issue-1146 branch 5 times, most recently from 6cf7469 to 8007872 Compare December 9, 2017 13:19

copumpkin mentioned this pull request Aug 1, 2018

EMR create a new resource aws_emr_instances_fleet #1146

Closed

peay reviewed Sep 7, 2018

View reviewed changes

peay reviewed Sep 9, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 5, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 20, 2019

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 20, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label May 13, 2019

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 7, 2019

aeschright requested a review from a team June 25, 2019 18:43

bflad closed this Jun 26, 2019

peay mentioned this pull request Sep 12, 2019

New Resource: aws_emr_instance_fleet #9263

Closed

ghost locked and limited conversation to collaborators Nov 3, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Nov 3, 2019

New Resource: aws_emr_instance_fleet #1475

New Resource: aws_emr_instance_fleet #1475

Conversation

jmsantorum commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmsantorum commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stekiri commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmsantorum commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stekiri commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peay commented Jul 3, 2018

krish7919 commented Jul 9, 2018

copumpkin commented Aug 1, 2018

dthauvin commented Aug 2, 2018

peay commented Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelocorreia commented Nov 6, 2018

david-wells-1 commented Jan 15, 2019

david-wells-1 commented Jan 31, 2019 • edited Loading

dmitriy-lukyanchikov commented Feb 12, 2019

plejik commented Feb 28, 2019

bflad commented Mar 5, 2019

jmsantorum commented Mar 20, 2019

abhivijay89 commented May 13, 2019

sanoojm commented May 23, 2019

jmsantorum commented May 23, 2019

bflad commented Jun 26, 2019

ghost commented Nov 3, 2019

peay commented Sep 3, 2018 •

edited

Loading

david-wells-1 commented Jan 31, 2019 •

edited

Loading