Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor conditions logic #305

Merged
merged 2 commits into from
Mar 18, 2020
Merged

Refactor conditions logic #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc73da2
Refactor conditions logic
Mar 17, 2020
674a7c4
Remove double imports
Mar 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 19 additions & 24 deletions pkg/actuators/machine/actuator.go
View file
Open in desktop
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
Open in desktop
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