Repository: keikoproj/upgrade-manager
Pull request #43 (2020-01-13 22:06:51)

[WIP] Add support for LaunchTemplates
Hello!
Fixes https://github.com/keikoproj/upgrade-manager/issues/42

Adds support for LaunchTemplates and MixedInstancesPolicy.

I did manual functional tests, works as expected.

shrinandj: Looks good to me. But, please add a "Testing Done" to the PR here (ideally to the original commit message itself) where you add the details of what all tests were done. 

e.g.
- Added unit tests. They pass.
- Verified that upgrade-manager completed successfully for LaunchConfig based upgrade.
- Verified that upgrade-manager completed successfully for LaunchTemplate based upgrade.
- Replaced a launchConfig with launchTemplate. Verified that upgrade completed for such a launchTemplate.
shrinandj: Great work BTW!! :-)
uthark: @eytan-avisror Could you please take another look?
eytan-avisror: @uthark Thanks, it looks good. did you get a chance to run this code and do a basic test?
uthark: Yes, I tested actual rolling update in our dev infra.
On Jan 15, 2020, 17:55 -0800, Eytan Avisror <notifications@github.com>, wrote:
> @uthark Thanks, it looks good. did you get a chance to run this code and do a basic test?
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.


Patch:
diff --git a/controllers/launch_definition.go b/controllers/launch_definition.go
new file mode 100644
index 0000000..f68e5b9
--- /dev/null
+++ b/controllers/launch_definition.go
@@ -0,0 +1,28 @@
+package controllers
+
+import (
+	"github.com/aws/aws-sdk-go/service/autoscaling"
+)
+
+// launchDefinition describes how instances are launched in ASG.
+// Supports LaunchConfiguration and LaunchTemplate.
+type launchDefinition struct {
+	// launchConfigurationName is name of LaunchConfiguration used by ASG.
+	// +optional
+	launchConfigurationName *string
+	// launchTemplate is Launch template definition used for ASG.
+	// +optional
+	launchTemplate *autoscaling.LaunchTemplateSpecification
+}
+
+func NewLaunchDefinition(asg *autoscaling.Group) *launchDefinition {
+	template := asg.LaunchTemplate
+	if template == nil && asg.MixedInstancesPolicy != nil {
+		template = asg.MixedInstancesPolicy.LaunchTemplate.LaunchTemplateSpecification
+	}
+	l := &launchDefinition{
+		launchConfigurationName: asg.LaunchConfigurationName,
+		launchTemplate:          template,
+	}
+	return l
+}
diff --git a/controllers/rollingupgrade_controller.go b/controllers/rollingupgrade_controller.go
index 9f79ef8..f3a4983 100644
--- a/controllers/rollingupgrade_controller.go
+++ b/controllers/rollingupgrade_controller.go
@@ -40,8 +40,6 @@ import (
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 	"github.com/go-logr/logr"
-	upgrademgrv1alpha1 "github.com/keikoproj/upgrade-manager/api/v1alpha1"
-	log "github.com/keikoproj/upgrade-manager/pkg/log"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	v1errors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -51,6 +49,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	upgrademgrv1alpha1 "github.com/keikoproj/upgrade-manager/api/v1alpha1"
+	log "github.com/keikoproj/upgrade-manager/pkg/log"
 )
 
 const (
@@ -446,7 +447,8 @@ func (r *RollingUpgradeReconciler) runRestack(ctx *context.Context, ruObj *upgra
 	// set the state of instances in the ASG to new in the cluster store
 	r.ClusterState.initializeAsg(*asg.AutoScalingGroupName, asg.Instances)
 
-	currentLaunchConfigName := aws.StringValue(asg.LaunchConfigurationName)
+	launchDefinition := NewLaunchDefinition(asg)
+
 	processedInstances := 0
 
 	inProgress, err := r.getInProgressInstances(asg.Instances)
@@ -479,7 +481,7 @@ func (r *RollingUpgradeReconciler) runRestack(ctx *context.Context, ruObj *upgra
 		}
 
 		// update the instances
-		err := r.UpdateInstances(ctx, ruObj, instances, currentLaunchConfigName, KubeCtlCall)
+		err := r.UpdateInstances(ctx, ruObj, instances, launchDefinition, KubeCtlCall)
 		processedInstances += len(instances)
 		if err != nil {
 			return processedInstances, err
@@ -754,7 +756,7 @@ func NewUpdateInstancesError(instanceUpdateErrors []error) *UpdateInstancesError
 func (r *RollingUpgradeReconciler) UpdateInstances(ctx *context.Context,
 	ruObj *upgrademgrv1alpha1.RollingUpgrade,
 	instances []*autoscaling.Instance,
-	currentLaunchConfigName string,
+	launchDefinition *launchDefinition,
 	KubeCtlCall string) error {
 
 	totalNodes := len(instances)
@@ -765,7 +767,7 @@ func (r *RollingUpgradeReconciler) UpdateInstances(ctx *context.Context,
 	ch := make(chan error)
 
 	for _, instance := range instances {
-		go r.UpdateInstance(ctx, ruObj, instance, currentLaunchConfigName, KubeCtlCall, ch)
+		go r.UpdateInstance(ctx, ruObj, instance, launchDefinition, KubeCtlCall, ch)
 	}
 
 	// wait for upgrades to complete
@@ -796,24 +798,19 @@ Loop:
 func (r *RollingUpgradeReconciler) UpdateInstance(ctx *context.Context,
 	ruObj *upgrademgrv1alpha1.RollingUpgrade,
 	i *autoscaling.Instance,
-	currentLaunchConfigName string,
+	launchDefinition *launchDefinition,
 	KubeCtlCall string,
 	ch chan error) {
 
-	targetLaunchConfigName := aws.StringValue(i.LaunchConfigurationName)
-	targetInstanceID := aws.StringValue(i.InstanceId)
-
 	// If the running node has the same launchconfig as the asg,
 	// there is no need to refresh it.
-	if targetLaunchConfigName != "" {
-		// if LaunchConfig is blank, the instance is not inline with the active LaunchConfig
-		if targetLaunchConfigName == currentLaunchConfigName {
-			log.Printf("Ignoring %s since it has the correct launch-config", targetInstanceID)
-			ruObj.Status.NodesProcessed = ruObj.Status.NodesProcessed + 1
-			r.Update(*ctx, ruObj)
-			ch <- nil
-			return
-		}
+	targetInstanceID := aws.StringValue(i.InstanceId)
+	if !requiresRefresh(i, launchDefinition) {
+		log.Printf("Ignoring %s since it has the correct launch-config", targetInstanceID)
+		ruObj.Status.NodesProcessed = ruObj.Status.NodesProcessed + 1
+		r.Update(*ctx, ruObj)
+		ch <- nil
+		return
 	}
 
 	nodeName := r.getNodeName(i, r.NodeList, ruObj)
@@ -875,3 +872,24 @@ func (r *RollingUpgradeReconciler) UpdateInstance(ctx *context.Context,
 	ch <- nil
 	return
 }
+
+func requiresRefresh(ec2Instance *autoscaling.Instance, definition *launchDefinition) bool {
+	if definition.launchConfigurationName != nil {
+		if *(definition.launchConfigurationName) != aws.StringValue(ec2Instance.LaunchConfigurationName) {
+			return true
+		}
+	} else if definition.launchTemplate != nil && ec2Instance.LaunchTemplate != nil {
+		instanceLaunchTemplate := ec2Instance.LaunchTemplate
+		targetLaunchTemplate := definition.launchTemplate
+		if aws.StringValue(instanceLaunchTemplate.LaunchTemplateId) != aws.StringValue(targetLaunchTemplate.LaunchTemplateId) {
+			return true
+		}
+		if aws.StringValue(instanceLaunchTemplate.LaunchTemplateName) != aws.StringValue(targetLaunchTemplate.LaunchTemplateName) {
+			return true
+		}
+		if aws.String(*instanceLaunchTemplate.Version) != aws.String(*targetLaunchTemplate.Version) {
+			return true
+		}
+	}
+	return false
+}


Test patch:
diff --git a/controllers/rollingupgrade_controller_test.go b/controllers/rollingupgrade_controller_test.go
index cb26e38..8392236 100644
--- a/controllers/rollingupgrade_controller_test.go
+++ b/controllers/rollingupgrade_controller_test.go
@@ -9,7 +9,7 @@ import (
 	"testing"
 	"time"
 
-	log "github.com/keikoproj/upgrade-manager/pkg/log"
+	"github.com/keikoproj/upgrade-manager/pkg/log"
 
 	"k8s.io/apimachinery/pkg/util/intstr"
 
@@ -27,26 +27,20 @@ import (
 	"golang.org/x/net/context"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
 	v1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	upgrademgrv1alpha1 "github.com/keikoproj/upgrade-manager/api/v1alpha1"
 )
 
 var c client.Client
 
-var expectedRequest = reconcile.Request{NamespacedName: types.NamespacedName{Name: "foo", Namespace: "default"}}
-var depKey = types.NamespacedName{Name: "foo-deployment", Namespace: "default"}
 var defaultMsgPrefix = "ru-foo"
 
-const timeout = time.Second * 5
-
 func TestMain(m *testing.M) {
 	testEnv = &envtest.Environment{
 		CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")},
@@ -1276,8 +1270,9 @@ func TestUpdateInstances(t *testing.T) {
 
 	ctx := context.TODO()
 
+	lcName := "A"
 	err = rcRollingUpgrade.UpdateInstances(&ctx,
-		ruObj, mockAsg.Instances, "A", "exit 0;")
+		ruObj, mockAsg.Instances, &launchDefinition{launchConfigurationName: &lcName}, "exit 0;")
 	g.Expect(err).ShouldNot(gomega.HaveOccurred())
 }
 
@@ -1330,8 +1325,9 @@ func TestUpdateInstancesError(t *testing.T) {
 
 	ctx := context.TODO()
 
+	lcName := "A"
 	err = rcRollingUpgrade.UpdateInstances(&ctx,
-		ruObj, mockAsg.Instances, "A", "exit 0;")
+		ruObj, mockAsg.Instances, &launchDefinition{launchConfigurationName: &lcName}, "exit 0;")
 	g.Expect(err).Should(gomega.HaveOccurred())
 	g.Expect(err).Should(gomega.BeAssignableToTypeOf(&UpdateInstancesError{}))
 	if updateInstancesError, ok := err.(*UpdateInstancesError); ok {
@@ -1391,8 +1387,9 @@ func TestUpdateInstancesPartialError(t *testing.T) {
 
 	ctx := context.TODO()
 
+	lcName := "A"
 	err = rcRollingUpgrade.UpdateInstances(&ctx,
-		ruObj, mockAsg.Instances, "A", "exit 0;")
+		ruObj, mockAsg.Instances, &launchDefinition{launchConfigurationName: &lcName}, "exit 0;")
 	g.Expect(err).Should(gomega.HaveOccurred())
 	g.Expect(err).Should(gomega.BeAssignableToTypeOf(&UpdateInstancesError{}))
 	if updateInstancesError, ok := err.(*UpdateInstancesError); ok {
@@ -1418,8 +1415,9 @@ func TestUpdateInstancesWithZeroInstances(t *testing.T) {
 
 	ctx := context.TODO()
 
+	lcName := "A"
 	err = rcRollingUpgrade.UpdateInstances(&ctx,
-		nil, nil, "A", "exit 0;")
+		nil, nil, &launchDefinition{launchConfigurationName: &lcName}, "exit 0;")
 	g.Expect(err).ShouldNot(gomega.HaveOccurred())
 }
 
@@ -2122,3 +2120,113 @@ func TestRunRestackWithNodesLessThanMaxUnavailable(t *testing.T) {
 	g.Expect(err).To(gomega.BeNil())
 	g.Expect(nodesProcessed).To(gomega.Equal(1))
 }
+
+func TestRequiresRefreshHandlesLaunchConfiguration(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+
+	mockID := "some-id"
+	someLaunchConfig := "some-launch-config-v1"
+	az := "az-1"
+	mockInstance := autoscaling.Instance{InstanceId: &mockID, LaunchConfigurationName: &someLaunchConfig, AvailabilityZone: &az}
+
+	newLaunchConfig := "some-launch-config-v2"
+	definition := launchDefinition{
+		launchConfigurationName: &newLaunchConfig,
+	}
+
+	result := requiresRefresh(&mockInstance, &definition)
+	g.Expect(result).To(gomega.Equal(true))
+}
+
+func TestRequiresRefreshHandlesLaunchTemplateNameVersionUpdate(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+
+	mockID := "some-id"
+	oldLaunchTemplate := &autoscaling.LaunchTemplateSpecification{
+		LaunchTemplateName: aws.String("launch-template"),
+		Version:            aws.String("1"),
+	}
+	az := "az-1"
+	mockInstance := autoscaling.Instance{InstanceId: &mockID, LaunchTemplate: oldLaunchTemplate, AvailabilityZone: &az}
+
+	newLaunchTemplate := &autoscaling.LaunchTemplateSpecification{
+		LaunchTemplateName: aws.String("launch-template"),
+		Version:            aws.String("2"),
+	}
+	definition := launchDefinition{
+		launchTemplate: newLaunchTemplate,
+	}
+
+	result := requiresRefresh(&mockInstance, &definition)
+	g.Expect(result).To(gomega.Equal(true))
+}
+
+func TestRequiresRefreshHandlesLaunchTemplateIDVersionUpdate(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+
+	mockID := "some-id"
+	oldLaunchTemplate := &autoscaling.LaunchTemplateSpecification{
+		LaunchTemplateId: aws.String("launch-template-id-v1"),
+		Version:            aws.String("1"),
+	}
+	az := "az-1"
+	mockInstance := autoscaling.Instance{InstanceId: &mockID, LaunchTemplate: oldLaunchTemplate, AvailabilityZone: &az}
+
+	newLaunchTemplate := &autoscaling.LaunchTemplateSpecification{
+		LaunchTemplateId: aws.String("launch-template-id-v1"),
+		Version:            aws.String("2"),
+	}
+	definition := launchDefinition{
+		launchTemplate: newLaunchTemplate,
+	}
+
+	result := requiresRefresh(&mockInstance, &definition)
+	g.Expect(result).To(gomega.Equal(true))
+}
+
+func TestRequiresRefreshHandlesLaunchTemplateNameUpdate(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+
+	mockID := "some-id"
+	oldLaunchTemplate := &autoscaling.LaunchTemplateSpecification{
+		LaunchTemplateName: aws.String("launch-template"),
+		Version:            aws.String("1"),
+	}
+	az := "az-1"
+	mockInstance := autoscaling.Instance{InstanceId: &mockID, LaunchTemplate: oldLaunchTemplate, AvailabilityZone: &az}
+
+	newLaunchTemplate := &autoscaling.LaunchTemplateSpecification{
+		LaunchTemplateName: aws.String("launch-template-v2"),
+		Version:            aws.String("1"),
+	}
+	definition := launchDefinition{
+		launchTemplate: newLaunchTemplate,
+	}
+
+	result := requiresRefresh(&mockInstance, &definition)
+	g.Expect(result).To(gomega.Equal(true))
+}
+
+
+func TestRequiresRefreshHandlesLaunchTemplateIDUpdate(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+
+	mockID := "some-id"
+	oldLaunchTemplate := &autoscaling.LaunchTemplateSpecification{
+		LaunchTemplateId: aws.String("launch-template-id-v1"),
+		Version:            aws.String("1"),
+	}
+	az := "az-1"
+	mockInstance := autoscaling.Instance{InstanceId: &mockID, LaunchTemplate: oldLaunchTemplate, AvailabilityZone: &az}
+
+	newLaunchTemplate := &autoscaling.LaunchTemplateSpecification{
+		LaunchTemplateId: aws.String("launch-template-id-v2"),
+		Version:            aws.String("1"),
+	}
+	definition := launchDefinition{
+		launchTemplate: newLaunchTemplate,
+	}
+
+	result := requiresRefresh(&mockInstance, &definition)
+	g.Expect(result).To(gomega.Equal(true))
+}