Repository: keikoproj/instance-manager
Pull request #57 (2019-10-15 15:42:25)

Support for MetricsCollection
Fixing  #53 Developed and tested support for metrics collection with metric option -all which would simply add all of the supported metrics.
Testing done:
Added TestGetNodeAutoScalingGroupMetrics in ekscloudformation_test.go that covers
case 1 when metricsCollection section is not specified at all
case 2 for metricsCollection:
                    - groupMinSize
                    - groupMaxSize
case 3 for metricsCollection:
                    - all
I had used sigs.k8s.io/controller-tools **v0.2.1** as this sigs.k8s.io/controller-tools/cmd/controller-gen **@v0.2.0-beta.2** was throwing error while I tried setting up, so there are many changes in go.sum accordingly, should I commit them too?

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/keikoproj/instance-manager?pullRequest=57) <br/>Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our [Contributor License Agreement](https://cla-assistant.io/keikoproj/instance-manager?pullRequest=57) before we can accept your contribution.<br/>**0** out of **2** committers have signed the CLA.<br/><br/>:x: rjohn3<br/>:x: RiyaJohn<br/><hr/>**rjohn3** seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please [add the email address used for this commit to your account](https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user).<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/keikoproj/instance-manager?pullRequest=57) it.</sub>
codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/57?src=pr&el=h1) Report
> Merging [#57](https://codecov.io/gh/keikoproj/instance-manager/pull/57?src=pr&el=desc) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/fb2f0893bf5aa00c97f550ce28d505907e29be6c?src=pr&el=desc) will **increase** coverage by `0.28%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/instance-manager/pull/57/graphs/tree.svg?width=650&token=IJbjmSBliL&height=150&src=pr)](https://codecov.io/gh/keikoproj/instance-manager/pull/57?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   72.15%   72.43%   +0.28%     
==========================================
  Files           6        6              
  Lines         783      791       +8     
==========================================
+ Hits          565      573       +8     
  Misses        160      160              
  Partials       58       58
```


| [Impacted Files](https://codecov.io/gh/keikoproj/instance-manager/pull/57?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...rovisioners/ekscloudformation/ekscloudformation.go](https://codecov.io/gh/keikoproj/instance-manager/pull/57/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrc2Nsb3VkZm9ybWF0aW9uL2Vrc2Nsb3VkZm9ybWF0aW9uLmdv) | `81.44% <100%> (+0.47%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/57?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/57?src=pr&el=footer). Last update [fb2f089...e76e00f](https://codecov.io/gh/keikoproj/instance-manager/pull/57?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

RiyaJohn: By mistake, my initial commit got a wrong author name rjohn3. Can the cla block be skipped for rjohn3? Else I will have to clone repo afresh again and commit changes?
eytan-avisror: Thanks @RiyaJohn it looks great.
I just had one comment above that I think we can improve the resolving of `all` and avoid go-templating if possible.

Can you also add a `testing done` section and elaborate a bit on the scenarios you tested?

Regarding the CLA - afraid it is not possible the skip this check, can you sign in with the other user and try to sign it? If you look at the last comment of #55 I think he had the same issue.

Regarding the change for `sigs.k8s.io/controller-tools` - we can possibly do this in a separate PR - we should probably run a full e2e test before merging it too.

You can run a full BDD test on your changes if you'd like (see https://github.com/keikoproj/instance-manager/blob/master/.github/DEVELOPER.md#running-bdd-tests)

Thanks again for the PR
RiyaJohn: > Thanks @RiyaJohn it looks great.
> I just had one comment above that I think we can improve the resolving of `all` and avoid go-templating if possible.
> 
> Can you also add a `testing done` section to the PR description and elaborate a bit on the scenarios you tested?
> 
> Regarding the CLA - afraid it is not possible the skip this check, can you sign in with the other user and try to sign it? If you look at the last comment of #55 I think he had the same issue.
> 
> Regarding the change for `sigs.k8s.io/controller-tools` - we can possibly do this in a separate PR - we should probably run a full e2e test before merging it too.
> 
> You can run a full BDD test on your changes if you'd like (see https://github.com/keikoproj/instance-manager/blob/master/.github/DEVELOPER.md#running-bdd-tests)
> 
> Thanks again for the PR

Added testing done section to PR. Fixed the cla issue, Thanks to the comment in #55, it helped.

Patch:
diff --git a/.github/DEVELOPER.md b/.github/DEVELOPER.md
index 173f23c..3e812c2 100644
--- a/.github/DEVELOPER.md
+++ b/.github/DEVELOPER.md
@@ -27,7 +27,7 @@ create
 Using the `Makefile` you can use `make run` to run instance-manager locally on your machine, and it will try to reconcile InstanceGroups in the cluster.
 Make sure the context you are in is correct and that you scaled down the actual instance-manager controller pod.
 
-1. You need to create 2 files for controller config and cfn and copy the data section from #config/crd/bases/instance-manager-configmap.yaml respectively
+1. You need to create 2 files(controller.conf and cloudformation.template) for controller config and cfn and copy controller.conf and cloudformation.template section from the data section from #config/crd/bases/instance-manager-configmap.yaml respectively
 2. Make sure you have AWS Credentials and region exported so that your local instance-manager controller can execute a cloud formation template
 
 
diff --git a/api/v1alpha1/instancegroup_types.go b/api/v1alpha1/instancegroup_types.go
index d25029c..f9dbbeb 100644
--- a/api/v1alpha1/instancegroup_types.go
+++ b/api/v1alpha1/instancegroup_types.go
@@ -174,6 +174,7 @@ type EKSCFConfiguration struct {
 	SpotPrice          string              `json:"spotPrice,omitempty"`
 	Tags               []map[string]string `json:"tags,omitempty"`
 	ManagedPolicies    []string            `json:"managedPolicies,omitempty"`
+	MetricsCollection  []string            `json:"metricsCollection,omitempty"`
 }
 
 // InstanceGroupStatus defines the schema of resource Status
@@ -426,6 +427,14 @@ func (conf *EKSCFConfiguration) SetTags(tags []map[string]string) {
 	conf.Tags = tags
 }
 
+func (conf *EKSCFConfiguration) GetMetricsCollection() []string {
+	return conf.MetricsCollection
+}
+
+func (conf *EKSCFConfiguration) SetMetricsCollection(metricsCollection []string) {
+	conf.MetricsCollection = metricsCollection
+}
+
 func (ig *InstanceGroup) GetState() ReconcileState {
 	return ReconcileState(ig.Status.CurrentState)
 }
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
index 232626d..873c28b 100644
--- a/api/v1alpha1/zz_generated.deepcopy.go
+++ b/api/v1alpha1/zz_generated.deepcopy.go
@@ -86,6 +86,11 @@ func (in *EKSCFConfiguration) DeepCopyInto(out *EKSCFConfiguration) {
 		*out = make([]string, len(*in))
 		copy(*out, *in)
 	}
+	if in.MetricsCollection != nil {
+		in, out := &in.MetricsCollection, &out.MetricsCollection
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EKSCFConfiguration.
diff --git a/config/crd/bases/instance-manager-configmap.yaml b/config/crd/bases/instance-manager-configmap.yaml
index fec05d5..10c15ff 100644
--- a/config/crd/bases/instance-manager-configmap.yaml
+++ b/config/crd/bases/instance-manager-configmap.yaml
@@ -111,6 +111,9 @@ data:
       ManagedPolicyARNs:
         Description: List of Managed IAM Policy ARNs to be attached to instance groups
         Type: List<String>
+      NodeAutoScalingGroupMetrics:
+        Description: The Metrics that an Amazon EC2 ASG sends to Amazon CloudWatch.
+        Type: List<String>
     Metadata:
       AWS::CloudFormation::Interface:
         ParameterGroups:
@@ -133,6 +136,7 @@ data:
               - KeyName
               - BootstrapArguments
               - ManagedPolicyARNs
+              - NodeAutoScalingGroupMetrics
           -
             Label:
               default: "Worker Network Configuration"
@@ -179,6 +183,17 @@ data:
           - Key: KubernetesCluster
             Value: !Ref ClusterName
             PropagateAtLaunch: 'true'
+          {{if .Spec.EKSCFSpec.EKSCFConfiguration.MetricsCollection}}
+          MetricsCollection:
+          {{$metrics := index .Spec.EKSCFSpec.EKSCFConfiguration.MetricsCollection 0}}
+          {{if (eq $metrics "all")}}
+          - Granularity: "1Minute"
+          {{else}}
+          - Metrics:
+              !Ref NodeAutoScalingGroupMetrics
+            Granularity: "1Minute"
+          {{end}}
+          {{end}}
         {{$strategy := .Spec.AwsUpgradeStrategy.Type | ToLower}}
         {{if (eq $strategy "rollingupdate")}}
         UpdatePolicy:
diff --git a/config/samples/instancemgr_v1alpha1_instancegroup_with_metrics_collection1.yaml b/config/samples/instancemgr_v1alpha1_instancegroup_with_metrics_collection1.yaml
new file mode 100644
index 0000000..974c50d
--- /dev/null
+++ b/config/samples/instancemgr_v1alpha1_instancegroup_with_metrics_collection1.yaml
@@ -0,0 +1,27 @@
+apiVersion: instancemgr.keikoproj.io/v1alpha1
+kind: InstanceGroup
+metadata:
+  labels:
+    controller-tools.k8s.io: "1.0"
+  name: hello-world
+spec:
+  provisioner: eks-cf
+  strategy:
+      type: rollingUpdate
+  eks-cf:
+    maxSize: 3
+    minSize: 1
+    configuration:
+      keyPairName: mykey
+      image: ami-0923e4b35a30a5f53
+      instanceType: m5.large
+      volSize: 20
+      securityGroups:
+      - sg-0de3bb02a40b915e6
+      tags:
+        - key: mykey
+          value: myvalue
+      metricsCollection:
+        - groupMinSize
+        - groupMaxSize
+        - groupDesiredCapacity
\ No newline at end of file
diff --git a/config/samples/instancemgr_v1alpha1_instancegroup_with_metrics_collection2.yaml b/config/samples/instancemgr_v1alpha1_instancegroup_with_metrics_collection2.yaml
new file mode 100644
index 0000000..25e36d3
--- /dev/null
+++ b/config/samples/instancemgr_v1alpha1_instancegroup_with_metrics_collection2.yaml
@@ -0,0 +1,25 @@
+apiVersion: instancemgr.keikoproj.io/v1alpha1
+kind: InstanceGroup
+metadata:
+  labels:
+    controller-tools.k8s.io: "1.0"
+  name: hello-world
+spec:
+  provisioner: eks-cf
+  strategy:
+      type: rollingUpdate
+  eks-cf:
+    maxSize: 3
+    minSize: 1
+    configuration:
+      keyPairName: mykey
+      image: ami-0923e4b35a30a5f53
+      instanceType: m5.large
+      volSize: 20
+      securityGroups:
+      - sg-0de3bb02a40b915e6
+      tags:
+        - key: mykey
+          value: myvalue
+      metricsCollection:
+        - all
\ No newline at end of file
diff --git a/controllers/provisioners/ekscloudformation/ekscloudformation.go b/controllers/provisioners/ekscloudformation/ekscloudformation.go
index 84a56f4..4fd480f 100644
--- a/controllers/provisioners/ekscloudformation/ekscloudformation.go
+++ b/controllers/provisioners/ekscloudformation/ekscloudformation.go
@@ -475,6 +475,7 @@ func (ctx *EksCfInstanceGroupContext) processParameters() error {
 		"NodeGroupName":               ctx.AwsWorker.StackName,
 		"VpcId":                       ctx.VpcID,
 		"ManagedPolicyARNs":           getManagedPolicyARNs(specConfig.ManagedPolicies),
+		"NodeAutoScalingGroupMetrics": getNodeAutoScalingGroupMetrics(specConfig.GetMetricsCollection()),
 	}
 
 	var parameters []*cloudformation.Parameter
@@ -510,3 +511,14 @@ func getManagedPolicyARNs(pNames []string) string {
 
 	return common.ConcatonateList(managedPolicyARNs, ",")
 }
+
+func getNodeAutoScalingGroupMetrics(metrics []string) string {
+	if len(metrics) == 0 || len(metrics) == 1 && metrics[0] == "all" {
+		return ""
+	}
+	var resp []string
+	for _, metric := range metrics {
+		resp = append(resp, strings.Title(metric))
+	}
+	return common.ConcatonateList(resp, ",")
+}
diff --git a/docs/04_instance-manager.yaml b/docs/04_instance-manager.yaml
index 04b793f..703ee71 100644
--- a/docs/04_instance-manager.yaml
+++ b/docs/04_instance-manager.yaml
@@ -443,6 +443,10 @@ spec:
                       items:
                         type: string
                       type: array
+                    metricsCollection:
+                      items:
+                        type: string
+                      type: array
                     securityGroups:
                       items:
                         type: string
@@ -775,6 +779,9 @@ data:
       ManagedPolicyARNs:
         Description: List of Managed IAM Policy ARNs to be attached to instance groups
         Type: List<String>
+      NodeAutoScalingGroupMetrics:
+        Description: The Metrics that an Amazon EC2 ASG sends to Amazon CloudWatch.
+        Type: List<String>
     Metadata:
       AWS::CloudFormation::Interface:
         ParameterGroups:
@@ -797,6 +804,7 @@ data:
               - KeyName
               - BootstrapArguments
               - ManagedPolicyARNs
+              - NodeAutoScalingGroupMetrics
           -
             Label:
               default: "Worker Network Configuration"
@@ -843,6 +851,17 @@ data:
           - Key: KubernetesCluster
             Value: !Ref ClusterName
             PropagateAtLaunch: 'true'
+          {{if .Spec.EKSCFSpec.EKSCFConfiguration.MetricsCollection}}
+          MetricsCollection:
+          {{$metrics := index .Spec.EKSCFSpec.EKSCFConfiguration.MetricsCollection 0}}
+          {{if (eq $metrics "all")}}
+          - Granularity: "1Minute"
+          {{else}}
+          - Metrics:
+            !Ref NodeAutoScalingGroupMetrics
+            Granularity: "1Minute"
+          {{end}}
+          {{end}}
         {{$strategy := .Spec.AwsUpgradeStrategy.Type | ToLower}}
         {{if (eq $strategy "rollingupdate")}}
         UpdatePolicy:


Test patch:
diff --git a/controllers/provisioners/ekscloudformation/ekscloudformation_test.go b/controllers/provisioners/ekscloudformation/ekscloudformation_test.go
index c7ea365..eaf6ee1 100644
--- a/controllers/provisioners/ekscloudformation/ekscloudformation_test.go
+++ b/controllers/provisioners/ekscloudformation/ekscloudformation_test.go
@@ -1240,3 +1240,35 @@ func TestGetManagedPolicyARNs(t *testing.T) {
 		}
 	}
 }
+
+func TestGetNodeAutoScalingGroupMetrics(t *testing.T) {
+	tt := []struct {
+		testCase string
+		input    []string
+		expected string
+	}{
+		{
+			testCase: "NodeGroup with no metrics collection",
+			input:    []string{},
+			expected: "",
+		},
+		{
+			testCase: "NodeGroup metricsCollection with few metrics",
+			input:    []string{"groupMinSize", "groupMaxSize"},
+			expected: "GroupMinSize,GroupMaxSize",
+		},
+		{
+			testCase: "NodeGroup metricsCollection with all",
+			input:    []string{"all"},
+			expected: "",
+		},
+	}
+
+	for _, tc := range tt {
+		resp := getNodeAutoScalingGroupMetrics(tc.input)
+		if !reflect.DeepEqual(resp, tc.expected) {
+			t.Fatalf("Test Case [%s] Failed Expected [%s] Got [%s]\n", tc.testCase, tc.expected, resp)
+		}
+	}
+}
+