From e74f4f96463735baea95869b4afaa36d878ee041 Mon Sep 17 00:00:00 2001 From: Chuck Ha Date: Thu, 1 Nov 2018 14:27:56 -0400 Subject: [PATCH] Fixes machine delete (#353) This separates out updating of the machine and updating of the status. Adds really basic tests for the machine actuator. Signed-off-by: Chuck Ha --- pkg/cloud/aws/actuators/machine/actuator.go | 38 +++-- .../aws/actuators/machine/actuator_test.go | 160 ++++++++++++++++++ 2 files changed, 186 insertions(+), 12 deletions(-) create mode 100644 pkg/cloud/aws/actuators/machine/actuator_test.go diff --git a/pkg/cloud/aws/actuators/machine/actuator.go b/pkg/cloud/aws/actuators/machine/actuator.go index 48bdd66df9..958e320f1f 100644 --- a/pkg/cloud/aws/actuators/machine/actuator.go +++ b/pkg/cloud/aws/actuators/machine/actuator.go @@ -133,7 +133,14 @@ func (a *Actuator) Create(cluster *clusterv1.Cluster, machine *clusterv1.Machine return errors.Wrap(err, "failed to reconcile LB attachment") } - if err := a.storeMachineStatus(machine, status, true); err != nil { + if err := a.updateMachine(machine); err != nil { + glog.Errorf("failed to update machine %q in namespace %q trying to set annotation: %v", machine.Name, machine.Namespace, err) + return &controllerError.RequeueAfterError{ + RequeueAfter: time.Minute, + } + } + + if err := a.storeMachineStatus(machine, status); err != nil { glog.Errorf("failed to store provider status for machine %q in namespace %q: %v", machine.Name, machine.Namespace, err) } @@ -169,8 +176,10 @@ func (a *Actuator) Delete(cluster *clusterv1.Cluster, machine *clusterv1.Machine return errors.Wrap(err, "failed to get cluster provider config") } + // status.InstanceID is nil, so don't do this if status.InstanceID == nil { // Instance was never created + glog.Infof("Machine %v does not exist", machine.Name) return nil } @@ -183,6 +192,7 @@ func (a *Actuator) Delete(cluster *clusterv1.Cluster, machine *clusterv1.Machine if instance == nil { // The machine hasn't been created yet + glog.Info("Instance is nil and therefore does not exist") return nil } @@ -192,6 +202,7 @@ func (a *Actuator) Delete(cluster *clusterv1.Cluster, machine *clusterv1.Machine // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-lifecycle.html switch instance.State { case v1alpha1.InstanceStateShuttingDown, v1alpha1.InstanceStateTerminated: + glog.Infof("instance %q is shutting down or already terminated", machine.Name) return nil default: if err := ec2svc.TerminateInstance(aws.StringValue(status.InstanceID)); err != nil { @@ -199,6 +210,7 @@ func (a *Actuator) Delete(cluster *clusterv1.Cluster, machine *clusterv1.Machine } } + glog.Info("shutdown signal was sent. Shutting down machine.") return nil } @@ -260,11 +272,11 @@ func (a *Actuator) Update(cluster *clusterv1.Cluster, machine *clusterv1.Machine } if securityGroupsChanged || tagsChanged { - if err := a.storeMachineStatus(machine, status, true); err != nil { + if err := a.updateMachine(machine); err != nil { glog.Errorf("failed to store provider status for machine %q in namespace %q: %v", machine.Name, machine.Namespace, err) } } else { - if err := a.storeMachineStatus(machine, status, false); err != nil { + if err := a.storeMachineStatus(machine, status); err != nil { glog.Errorf("failed to store provider status for machine %q in namespace %q: %v", machine.Name, machine.Namespace, err) } } @@ -318,7 +330,15 @@ func (a *Actuator) Exists(cluster *clusterv1.Cluster, machine *clusterv1.Machine return true, nil } -func (a *Actuator) storeMachineStatus(machine *clusterv1.Machine, status *v1alpha1.AWSMachineProviderStatus, updateResource bool) error { +func (a *Actuator) updateMachine(machine *clusterv1.Machine) error { + machinesClient := a.machinesGetter.Machines(machine.Namespace) + if _, err := machinesClient.Update(machine); err != nil { + return fmt.Errorf("failed to update machine: %v", err) + } + return nil +} + +func (a *Actuator) storeMachineStatus(machine *clusterv1.Machine, status *v1alpha1.AWSMachineProviderStatus) error { machinesClient := a.machinesGetter.Machines(machine.Namespace) ext, err := v1alpha1.EncodeMachineStatus(status) @@ -328,14 +348,8 @@ func (a *Actuator) storeMachineStatus(machine *clusterv1.Machine, status *v1alph machine.Status.ProviderStatus = ext - if updateResource { - if _, err := machinesClient.Update(machine); err != nil { - return fmt.Errorf("failed to update machine for machine %q in namespace %q: %v", machine.Name, machine.Namespace, err) - } - } else { - if _, err := machinesClient.UpdateStatus(machine); err != nil { - return fmt.Errorf("failed to update machine status for machine %q in namespace %q: %v", machine.Name, machine.Namespace, err) - } + if _, err := machinesClient.UpdateStatus(machine); err != nil { + return fmt.Errorf("failed to update machine status for machine %q in namespace %q: %v", machine.Name, machine.Namespace, err) } return nil diff --git a/pkg/cloud/aws/actuators/machine/actuator_test.go b/pkg/cloud/aws/actuators/machine/actuator_test.go new file mode 100644 index 0000000000..f9bc6bad12 --- /dev/null +++ b/pkg/cloud/aws/actuators/machine/actuator_test.go @@ -0,0 +1,160 @@ +// Copyright © 2018 The Kubernetes Authors. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package machine_test + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/session" + "github.com/golang/mock/gomock" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsprovider/v1alpha1" + "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/actuators/machine" + "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/actuators/machine/mock_machineiface" + service "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/services" + "sigs.k8s.io/cluster-api-provider-aws/pkg/cloudtest" + clusterv1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1" + client "sigs.k8s.io/cluster-api/pkg/client/clientset_generated/clientset/typed/cluster/v1alpha1" +) + +type ec2svc struct { + instance *v1alpha1.Instance +} + +func (m *ec2svc) InstanceIfExists(instanceID *string) (*v1alpha1.Instance, error) { return nil, nil } +func (m *ec2svc) CreateInstance(machine *clusterv1.Machine, config *v1alpha1.AWSMachineProviderConfig, clusterStatus *v1alpha1.AWSClusterProviderStatus, cluster *clusterv1.Cluster) (*v1alpha1.Instance, error) { + return nil, nil +} +func (m *ec2svc) TerminateInstance(instanceID string) error { return nil } +func (m *ec2svc) DeleteBastion(instanceID string, status *v1alpha1.AWSClusterProviderStatus) error { + return nil +} +func (m *ec2svc) CreateOrGetMachine(machine *clusterv1.Machine, status *v1alpha1.AWSMachineProviderStatus, config *v1alpha1.AWSMachineProviderConfig, clusterStatus *v1alpha1.AWSClusterProviderStatus, cluster *clusterv1.Cluster) (*v1alpha1.Instance, error) { + return m.instance, nil +} +func (m *ec2svc) UpdateInstanceSecurityGroups(instanceID string, securityGroups []string) error { + return nil +} +func (m *ec2svc) UpdateResourceTags(resourceID *string, create map[string]string, remove map[string]string) error { + return nil +} +func (m *ec2svc) ReconcileNetwork(clusterName string, network *v1alpha1.Network) error { return nil } +func (m *ec2svc) ReconcileBastion(clusterName, keyName string, status *v1alpha1.AWSClusterProviderStatus) error { + return nil +} +func (m *ec2svc) DeleteNetwork(clusterName string, network *v1alpha1.Network) error { return nil } + +type machinesvc struct { + mock *mock_machineiface.MockMachineInterface +} + +func (m *machinesvc) Machines(namespace string) client.MachineInterface { + return m.mock +} + +type getter struct { + ec2 service.EC2Interface +} + +func (g *getter) Session(*v1alpha1.AWSClusterProviderConfig) *session.Session { + return nil +} +func (g *getter) EC2(*session.Session) service.EC2Interface { + return g.ec2 +} +func (g *getter) ELB(*session.Session) service.ELBInterface { + return nil +} + +func TestCreate(t *testing.T) { + testcases := []struct { + name string + machine *clusterv1.Machine + service *ec2svc + machineExpects func(*mock_machineiface.MockMachineInterface) + }{ + { + name: "ensure machine and status are both updated", + machine: &clusterv1.Machine{ + Spec: clusterv1.MachineSpec{ + ProviderConfig: clusterv1.ProviderConfig{ + Value: cloudtest.RuntimeRawExtension(t, v1alpha1.AWSMachineProviderConfig{}), + }, + }, + Status: clusterv1.MachineStatus{}, + }, + service: &ec2svc{ + instance: &v1alpha1.Instance{ + ID: "hello world", + }, + }, + machineExpects: func(mock *mock_machineiface.MockMachineInterface) { + mock.EXPECT().Update(&clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "cluster-api-provider-aws": "true", + }, + }, + Spec: clusterv1.MachineSpec{ + ProviderConfig: clusterv1.ProviderConfig{ + Value: cloudtest.RuntimeRawExtension(t, v1alpha1.AWSMachineProviderConfig{}), + }, + }, + }).Return(nil, nil) + + mock.EXPECT().UpdateStatus(&clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "cluster-api-provider-aws": "true", + }, + }, + Spec: clusterv1.MachineSpec{ + ProviderConfig: clusterv1.ProviderConfig{ + Value: cloudtest.RuntimeRawExtension(t, v1alpha1.AWSMachineProviderConfig{}), + }, + }, + Status: clusterv1.MachineStatus{ + ProviderStatus: cloudtest.RuntimeRawExtension(t, v1alpha1.AWSMachineProviderStatus{ + InstanceID: aws.String("hello world"), + InstanceState: aws.String(""), + }), + }, + }).Return(nil, nil) + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mockMachine := mock_machineiface.NewMockMachineInterface(mockCtrl) + tc.machineExpects(mockMachine) + + a := machine.NewActuator(machine.ActuatorParams{ + ServicesGetter: &getter{ + ec2: tc.service, + }, + MachinesGetter: &machinesvc{ + mock: mockMachine, + }, + }) + err := a.Create(&clusterv1.Cluster{}, tc.machine) + if err != nil { + t.Fatalf("did not expect an error: %v", err) + } + }) + } +}