Merge pull request #97 from spangenberg/availability-zone

Incorporate availability zone into placement decision
openshift · Nov 7, 2018 · 947111b · 947111b
2 parents 24d07d0 + 98b836b
commit 947111b
Show file tree

Hide file tree

Showing 4 changed files with 204 additions and 6 deletions.
diff --git a/pkg/cloud/aws/actuators/machine/actuator.go b/pkg/cloud/aws/actuators/machine/actuator.go
@@ -237,21 +237,25 @@ func getSecurityGroupsIDs(securityGroups []providerconfigv1.AWSResourceReference
 	return securityGroupIDs, nil
 }
 
-func getSubnetIDs(subnet providerconfigv1.AWSResourceReference, client awsclient.Client) ([]*string, error) {
+func getSubnetIDs(subnet providerconfigv1.AWSResourceReference, availabilityZone string, client awsclient.Client) ([]*string, error) {
 	var subnetIDs []*string
 	// ID has priority
 	if subnet.ID != nil {
 		subnetIDs = append(subnetIDs, subnet.ID)
-		subnetIDs = append(subnetIDs, subnet.ID)
 	} else {
+		var filters []providerconfigv1.Filter
+		if availabilityZone != "" {
+			filters = append(filters, providerconfigv1.Filter{Name: "availabilityZone", Values: []string{availabilityZone}})
+		}
+		filters = append(filters, subnet.Filters...)
 		glog.Info("Describing subnets based on filters")
 		describeSubnetRequest := ec2.DescribeSubnetsInput{
-			Filters: buildEC2Filters(subnet.Filters),
+			Filters: buildEC2Filters(filters),
 		}
 		describeSubnetResult, err := client.DescribeSubnets(&describeSubnetRequest)
 		if err != nil {
-			glog.Errorf("error describing security groups: %v", err)
-			return nil, fmt.Errorf("error describing security groups: %v", err)
+			glog.Errorf("error describing subnetes: %v", err)
+			return nil, fmt.Errorf("error describing subnets: %v", err)
 		}
 		for _, n := range describeSubnetResult.Subnets {
 			subnetID := *n.SubnetId
@@ -337,7 +341,7 @@ func (a *Actuator) CreateMachine(cluster *clusterv1.Cluster, machine *clusterv1.
 	if err != nil {
 		return nil, fmt.Errorf("error getting security groups IDs: %v,", err)
 	}
-	subnetIDs, err := getSubnetIDs(machineProviderConfig.Subnet, client)
+	subnetIDs, err := getSubnetIDs(machineProviderConfig.Subnet, machineProviderConfig.Placement.AvailabilityZone, client)
 	if err != nil {
 		return nil, fmt.Errorf("error getting subnet IDs: %v,", err)
 	}
@@ -403,6 +407,13 @@ func (a *Actuator) CreateMachine(cluster *clusterv1.Cluster, machine *clusterv1.
 		}
 	}
 
+	var placement *ec2.Placement
+	if machineProviderConfig.Placement.AvailabilityZone != "" && machineProviderConfig.Subnet.ID == nil {
+		placement = &ec2.Placement{
+			AvailabilityZone: aws.String(machineProviderConfig.Placement.AvailabilityZone),
+		}
+	}
+
 	inputConfig := ec2.RunInstancesInput{
 		ImageId:      amiID,
 		InstanceType: aws.String(machineProviderConfig.InstanceType),
@@ -414,6 +425,7 @@ func (a *Actuator) CreateMachine(cluster *clusterv1.Cluster, machine *clusterv1.
 		TagSpecifications:  []*ec2.TagSpecification{tagInstance, tagVolume},
 		NetworkInterfaces:  networkInterfaces,
 		UserData:           &userDataEnc,
+		Placement:          placement,
 	}
 
 	runResult, err := client.RunInstances(&inputConfig)

diff --git a/pkg/cloud/aws/actuators/machine/actuator_test.go b/pkg/cloud/aws/actuators/machine/actuator_test.go
@@ -412,3 +412,132 @@ func TestBuildEC2Filters(t *testing.T) {
 		t.Errorf("failed to buildEC2Filters. Expected: %+v, got: %+v", expected, got)
 	}
 }
+
+func TestAvailabiltyZone(t *testing.T) {
+	cases := []struct {
+		name             string
+		availabilityZone string
+		subnet           string
+	}{
+		{
+			name:             "availability zone only",
+			availabilityZone: "us-east-1a",
+		},
+		{
+			name:   "subnet only",
+			subnet: "subnet-b46032ec",
+		},
+		{
+			name:             "availability zone and subnet",
+			availabilityZone: "us-east-1a",
+			subnet:           "subnet-b46032ec",
+		},
+	}
+
+	codec, err := providerconfigv1.NewCodec()
+	if err != nil {
+		t.Fatalf("unable to build codec: %v", err)
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			machine, cluster, awsCredentialsSecret, userDataSecret, err := testMachineAPIResources(clusterID)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			machinePc := &providerconfigv1.AWSMachineProviderConfig{}
+			if err = codec.DecodeProviderConfig(&machine.Spec.ProviderConfig, machinePc); err != nil {
+				t.Fatal(err)
+			}
+
+			machinePc.Placement.AvailabilityZone = tc.availabilityZone
+			if tc.subnet == "" {
+				machinePc.Subnet.ID = nil
+			} else {
+				machinePc.Subnet.ID = aws.String(tc.subnet)
+			}
+
+			config, err := codec.EncodeProviderConfig(machinePc)
+			if err != nil {
+				t.Fatal(err)
+			}
+			machine.Spec.ProviderConfig = *config
+
+			fakeKubeClient := kubernetesfake.NewSimpleClientset(awsCredentialsSecret, userDataSecret)
+
+			fakeClient := fake.NewFakeClient(machine)
+
+			mockCtrl := gomock.NewController(t)
+			mockAWSClient := mockaws.NewMockClient(mockCtrl)
+
+			params := ActuatorParams{
+				Client:     fakeClient,
+				KubeClient: fakeKubeClient,
+				AwsClientBuilder: func(kubeClient kubernetes.Interface, secretName, namespace, region string) (awsclient.Client, error) {
+					return mockAWSClient, nil
+				},
+				Codec: codec,
+			}
+
+			actuator, err := NewActuator(params)
+			if err != nil {
+				t.Fatalf("Could not create AWS machine actuator: %v", err)
+			}
+
+			mockRunInstancesForPlacement(mockAWSClient, tc.availabilityZone, tc.subnet)
+			mockDescribeInstances(mockAWSClient, false)
+			mockTerminateInstances(mockAWSClient)
+			mockRegisterInstancesWithLoadBalancer(mockAWSClient, false)
+			mockDescribeSubnets(mockAWSClient)
+
+			actuator.Create(cluster, machine)
+		})
+	}
+}
+
+func mockDescribeSubnets(mockAWSClient *mockaws.MockClient) {
+	mockAWSClient.EXPECT().DescribeSubnets(gomock.Any()).Return(&ec2.DescribeSubnetsOutput{}, nil)
+}
+
+func mockRunInstancesForPlacement(mockAWSClient *mockaws.MockClient, availabilityZone, subnet string) {
+	var placement *ec2.Placement
+	if availabilityZone != "" && subnet == "" {
+		placement = &ec2.Placement{AvailabilityZone: aws.String(availabilityZone)}
+	}
+
+	mockAWSClient.EXPECT().RunInstances(Placement(placement)).Return(
+		&ec2.Reservation{
+			Instances: []*ec2.Instance{
+				{
+					ImageId:    aws.String("ami-a9acbbd6"),
+					InstanceId: aws.String("i-02fcb933c5da7085c"),
+					State: &ec2.InstanceState{
+						Name: aws.String("Running"),
+						Code: aws.Int64(16),
+					},
+					LaunchTime: aws.Time(time.Now()),
+				},
+			},
+		}, nil)
+}
+
+type placementMatcher struct {
+	placement *ec2.Placement
+}
+
+func (m placementMatcher) Matches(input interface{}) bool {
+	runInstancesInput, ok := input.(*ec2.RunInstancesInput)
+	if !ok {
+		return false
+	}
+	if runInstancesInput.Placement == m.placement {
+		return true
+	}
+	return false
+}
+
+func (m placementMatcher) String() string {
+	return fmt.Sprintf("is placement: %#v", m.placement)
+}
+
+func Placement(placement *ec2.Placement) gomock.Matcher { return placementMatcher{placement} }
diff --git a/test/machines/awsclient.go b/test/machines/awsclient.go
@@ -88,3 +88,25 @@ func (client *awsClientWrapper) GetTags(machine *clusterv1alpha1.Machine) (map[s
 	}
 	return tags, nil
 }
+
+func (client *awsClientWrapper) GetSubnet(machine *clusterv1alpha1.Machine) (string, error) {
+	instance, err := machineutils.GetRunningInstance(machine, client.client)
+	if err != nil {
+		return "", err
+	}
+	if instance.SubnetId == nil {
+		return "", err
+	}
+	return *instance.SubnetId, nil
+}
+
+func (client *awsClientWrapper) GetAvailabilityZone(machine *clusterv1alpha1.Machine) (string, error) {
+	instance, err := machineutils.GetRunningInstance(machine, client.client)
+	if err != nil {
+		return "", err
+	}
+	if instance.Placement == nil {
+		return "", err
+	}
+	return *instance.Placement.AvailabilityZone, nil
+}
diff --git a/test/machines/machines_test.go b/test/machines/machines_test.go
@@ -20,6 +20,9 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1alpha1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+
 	awsclient "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/client"
 )
 
@@ -132,6 +135,38 @@ var _ = framework.SigKubeDescribe("Machines", func() {
 			f.CreateMachineAndWait(testMachine, acw)
 			machinesToDelete.AddMachine(testMachine, f, acw)
 
+			var subnetID string
+			{
+				describeSubnetsInput := &ec2.DescribeSubnetsInput{
+					Filters: []*ec2.Filter{
+						{
+							Name: aws.String("tag:Name"),
+							Values: []*string{
+								aws.String(fmt.Sprintf("%s-*", clusterID)),
+							},
+						},
+						{
+							Name: aws.String("availabilityZone"),
+							Values: []*string{
+								aws.String("us-east-1a"),
+							},
+						},
+					},
+				}
+				describeSubnetsResult, err := awsClient.DescribeSubnets(describeSubnetsInput)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(len(describeSubnetsResult.Subnets)).
+					To(Equal(1), "Test criteria not met. Only one Subnet should match the given Tag.")
+				subnetID = *describeSubnetsResult.Subnets[0].SubnetId
+			}
+			subnet, err := acw.GetSubnet(testMachine)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(subnet).To(Equal(subnetID))
+
+			availabilityZone, err := acw.GetAvailabilityZone(testMachine)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(availabilityZone).To(Equal("us-east-1a"))
+
 			securityGroups, err := acw.GetSecurityGroups(testMachine)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(securityGroups).To(Equal([]string{fmt.Sprintf("%s-default", clusterID)}))