Skip to content

Commit

Permalink
Merge pull request #305 from alexander-demichev/refactorconditions
Browse files Browse the repository at this point in the history 
Refactor conditions logic
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Mar 18, 2020
2 parents 923caeb + 674a7c4 commit bf2f68a
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 130 deletions.
43 changes: 19 additions & 24 deletions pkg/actuators/machine/actuator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/aws/aws-sdk-go/service/ec2"
machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
machinecontroller "github.com/openshift/machine-api-operator/pkg/controller/machine"
mapierrors "github.com/openshift/machine-api-operator/pkg/controller/machine"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
apimachineryerrors "k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -43,12 +42,6 @@ const (
ec2InstanceIDNotFoundCode = "InvalidInstanceID.NotFound"
requeueAfterSeconds = 20
requeueAfterFatalSeconds = 180

// MachineCreationSucceeded indicates success for machine creation
MachineCreationSucceeded = "MachineCreationSucceeded"

// MachineCreationFailed indicates that machine creation failed
MachineCreationFailed = "MachineCreationFailed"
)

// Actuator is the AWS-specific actuator for the Cluster API machine controller
Expand Down Expand Up @@ -109,7 +102,9 @@ func (a *Actuator) Create(context context.Context, machine *machinev1.Machine) e
instance, err := a.CreateMachine(machine)
if err != nil {
klog.Errorf("%s: error creating machine: %v", machine.Name, err)
updateConditionError := a.setMachineProviderConditions(machine, providerconfigv1.MachineCreation, MachineCreationFailed, err.Error())
conditionFailed := conditionFailed()
conditionFailed.Message = err.Error()
updateConditionError := a.setMachineProviderConditions(machine, conditionFailed)
if updateConditionError != nil {
klog.Errorf("%s: error updating machine conditions: %v", machine.Name, updateConditionError)
}
Expand All @@ -130,7 +125,7 @@ func (a *Actuator) Create(context context.Context, machine *machinev1.Machine) e
return a.handleMachineError(machine, errors.Wrap(err, "failed to set machine cloud provider specifics"), createEventAction)
}

err = a.setStatus(machine, instance)
err = a.setStatus(machine, instance, conditionSuccess())
if err != nil {
return a.handleMachineError(machine, errors.Wrap(err, "failed to set machine status"), createEventAction)

Expand Down Expand Up @@ -218,7 +213,7 @@ func (a *Actuator) setMachineStatus(machine *machinev1.Machine, awsStatus *provi
}

// updateMachineProviderConditions updates conditions set within machine provider status.
func (a *Actuator) setMachineProviderConditions(machine *machinev1.Machine, conditionType providerconfigv1.AWSMachineProviderConditionType, reason string, msg string) error {
func (a *Actuator) setMachineProviderConditions(machine *machinev1.Machine, condition providerconfigv1.AWSMachineProviderCondition) error {
klog.Infof("%s: updating machine conditions", machine.Name)

awsStatus := &providerconfigv1.AWSMachineProviderStatus{}
Expand All @@ -227,7 +222,7 @@ func (a *Actuator) setMachineProviderConditions(machine *machinev1.Machine, cond
return err
}

awsStatus.Conditions = setAWSMachineProviderCondition(awsStatus.Conditions, conditionType, corev1.ConditionTrue, reason, msg, updateConditionIfReasonOrMessageChange)
awsStatus.Conditions = setAWSMachineProviderCondition(condition, awsStatus.Conditions)

if err := a.setMachineStatus(machine, awsStatus, nil); err != nil {
return err
Expand All @@ -240,7 +235,7 @@ func (a *Actuator) setMachineProviderConditions(machine *machinev1.Machine, cond
func (a *Actuator) CreateMachine(machine *machinev1.Machine) (*ec2.Instance, error) {
machineProviderConfig, err := providerConfigFromMachine(machine, a.codec)
if err != nil {
return nil, a.handleMachineError(machine, mapierrors.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), createEventAction)
return nil, a.handleMachineError(machine, machinecontroller.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), createEventAction)
}

credentialsSecretName := ""
Expand Down Expand Up @@ -298,13 +293,13 @@ func (a *Actuator) getUserData(machine *machinev1.Machine, machineProviderConfig
err := a.client.Get(context.Background(), client.ObjectKey{Namespace: machine.Namespace, Name: machineProviderConfig.UserDataSecret.Name}, &userDataSecret)
if err != nil {
if apimachineryerrors.IsNotFound(err) {
return nil, a.handleMachineError(machine, mapierrors.InvalidMachineConfiguration("user data secret %s: %v not found", machineProviderConfig.UserDataSecret.Name, err), createEventAction)
return nil, a.handleMachineError(machine, machinecontroller.InvalidMachineConfiguration("user data secret %s: %v not found", machineProviderConfig.UserDataSecret.Name, err), createEventAction)
}
return nil, a.handleMachineError(machine, mapierrors.CreateMachine("error getting user data secret %s: %v", machineProviderConfig.UserDataSecret.Name, err), createEventAction)
return nil, a.handleMachineError(machine, machinecontroller.CreateMachine("error getting user data secret %s: %v", machineProviderConfig.UserDataSecret.Name, err), createEventAction)
}
userData, exists := userDataSecret.Data[userDataSecretKey]
if !exists {
return nil, a.handleMachineError(machine, mapierrors.InvalidMachineConfiguration("%s: Secret %v/%v does not have %q field set. Thus, no user data applied when creating an instance.", machine.Name, machine.Namespace, machineProviderConfig.UserDataSecret.Name, userDataSecretKey), createEventAction)
return nil, a.handleMachineError(machine, machinecontroller.InvalidMachineConfiguration("%s: Secret %v/%v does not have %q field set. Thus, no user data applied when creating an instance.", machine.Name, machine.Namespace, machineProviderConfig.UserDataSecret.Name, userDataSecretKey), createEventAction)
}
return userData, nil
}
Expand All @@ -323,7 +318,7 @@ func (a *Actuator) Delete(context context.Context, machine *machinev1.Machine) e
func (a *Actuator) DeleteMachine(machine *machinev1.Machine) error {
machineProviderConfig, err := providerConfigFromMachine(machine, a.codec)
if err != nil {
return a.handleMachineError(machine, mapierrors.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), deleteEventAction)
return a.handleMachineError(machine, machinecontroller.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), deleteEventAction)
}

region := machineProviderConfig.Placement.Region
Expand Down Expand Up @@ -351,7 +346,7 @@ func (a *Actuator) DeleteMachine(machine *machinev1.Machine) error {

terminatingInstances, err := terminateInstances(client, existingInstances)
if err != nil {
return a.handleMachineError(machine, mapierrors.DeleteMachine(err.Error()), noEventAction)
return a.handleMachineError(machine, machinecontroller.DeleteMachine(err.Error()), noEventAction)
}

if len(terminatingInstances) == 1 {
Expand All @@ -377,7 +372,7 @@ func (a *Actuator) Update(context context.Context, machine *machinev1.Machine) e

machineProviderConfig, err := providerConfigFromMachine(machine, a.codec)
if err != nil {
return a.handleMachineError(machine, mapierrors.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), updateEventAction)
return a.handleMachineError(machine, machinecontroller.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", err), updateEventAction)
}

region := machineProviderConfig.Placement.Region
Expand Down Expand Up @@ -409,10 +404,10 @@ func (a *Actuator) Update(context context.Context, machine *machinev1.Machine) e

klog.Warningf("%s: attempted to update machine but no instances found", machine.Name)

a.handleMachineError(machine, mapierrors.UpdateMachine("no instance found, reason unknown"), updateEventAction)
a.handleMachineError(machine, machinecontroller.UpdateMachine("no instance found, reason unknown"), updateEventAction)

// Update status to clear out machine details.
if err := a.setStatus(machine, nil); err != nil {
if err := a.setStatus(machine, nil, conditionSuccess()); err != nil {
return err
}
// This is an unrecoverable error condition. We should delay to
Expand All @@ -431,7 +426,7 @@ func (a *Actuator) Update(context context.Context, machine *machinev1.Machine) e

err = a.updateLoadBalancers(client, machineProviderConfig, newestInstance, machine.Name)
if err != nil {
a.handleMachineError(machine, mapierrors.CreateMachine("Error updating load balancers: %v", err), updateEventAction)
a.handleMachineError(machine, machinecontroller.CreateMachine("Error updating load balancers: %v", err), updateEventAction)
return err
}
} else {
Expand All @@ -453,7 +448,7 @@ func (a *Actuator) Update(context context.Context, machine *machinev1.Machine) e
}

// We do not support making changes to pre-existing instances, just update status.
err = a.setStatus(machine, newestInstance)
err = a.setStatus(machine, newestInstance, conditionSuccess())
if err != nil {
return a.handleMachineError(machine, errors.Wrap(err, "failed to set machine status"), updateEventAction)
}
Expand Down Expand Up @@ -572,7 +567,7 @@ func (a *Actuator) updateLoadBalancers(client awsclient.Client, providerConfig *
}

// setStatus calculates the new machine status, checks if anything has changed, and updates if so.
func (a *Actuator) setStatus(machine *machinev1.Machine, instance *ec2.Instance) error {
func (a *Actuator) setStatus(machine *machinev1.Machine, instance *ec2.Instance, condition providerconfigv1.AWSMachineProviderCondition) error {
klog.Infof("%s: Updating status", machine.Name)

// Starting with a fresh status as we assume full control of it here.
Expand Down Expand Up @@ -603,7 +598,7 @@ func (a *Actuator) setStatus(machine *machinev1.Machine, instance *ec2.Instance)
}
klog.Infof("%s: finished calculating AWS status", machine.Name)

awsStatus.Conditions = setAWSMachineProviderCondition(awsStatus.Conditions, providerconfigv1.MachineCreation, corev1.ConditionTrue, MachineCreationSucceeded, "machine successfully created", updateConditionIfReasonOrMessageChange)
awsStatus.Conditions = setAWSMachineProviderCondition(condition, awsStatus.Conditions)
if err := a.setMachineStatus(machine, awsStatus, networkAddresses); err != nil {
return err
}
Expand Down
15 changes: 7 additions & 8 deletions pkg/actuators/machine/actuator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
machineapierrors "github.com/openshift/machine-api-operator/pkg/controller/machine"
"github.com/stretchr/testify/assert"
apiv1 "k8s.io/api/core/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -267,7 +266,7 @@ func TestActuator(t *testing.T) {
t.Fatalf("Unable to get machine status: %v", err)
}

assert.Equal(t, machineStatus.Conditions[0].Reason, MachineCreationSucceeded)
assert.Equal(t, machineStatus.Conditions[0].Reason, providerconfigv1.MachineCreationSucceeded)

// Get the machine
if exists, err := actuator.Exists(context.TODO(), machine); err != nil || !exists {
Expand Down Expand Up @@ -307,7 +306,7 @@ func TestActuator(t *testing.T) {
t.Fatalf("Unable to get machine status: %v", err)
}

assert.Equal(t, machineStatus.Conditions[0].Reason, MachineCreationFailed)
assert.Equal(t, machineStatus.Conditions[0].Reason, providerconfigv1.MachineCreationFailed)
},
},
{
Expand Down Expand Up @@ -724,11 +723,11 @@ func TestGetUserData(t *testing.T) {
}

testCases := []struct {
secret *apiv1.Secret
secret *corev1.Secret
error error
}{
{
secret: &apiv1.Secret{
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: userDataSecretName,
Namespace: defaultNamespace,
Expand All @@ -740,7 +739,7 @@ func TestGetUserData(t *testing.T) {
error: nil,
},
{
secret: &apiv1.Secret{
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "notFound",
Namespace: defaultNamespace,
Expand All @@ -752,7 +751,7 @@ func TestGetUserData(t *testing.T) {
error: &machineapierrors.MachineError{},
},
{
secret: &apiv1.Secret{
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: userDataSecretName,
Namespace: defaultNamespace,
Expand Down Expand Up @@ -933,7 +932,7 @@ func TestCreate(t *testing.T) {
},
},
},
userDataSecret: &apiv1.Secret{
userDataSecret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: userDataSecretName,
Namespace: defaultNamespace,
Expand Down
Loading

0 comments on commit bf2f68a

Please sign in to comment.