diff --git a/pkg/actuators/machine/actuator.go b/pkg/actuators/machine/actuator.go index 41858f955b..816859ba5a 100644 --- a/pkg/actuators/machine/actuator.go +++ b/pkg/actuators/machine/actuator.go @@ -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" @@ -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 @@ -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) } @@ -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) @@ -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{} @@ -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 @@ -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 := "" @@ -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 } @@ -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 @@ -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 { @@ -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 @@ -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 @@ -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 { @@ -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) } @@ -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. @@ -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 } diff --git a/pkg/actuators/machine/actuator_test.go b/pkg/actuators/machine/actuator_test.go index 128334c140..013c7b2e33 100644 --- a/pkg/actuators/machine/actuator_test.go +++ b/pkg/actuators/machine/actuator_test.go @@ -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" @@ -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 { @@ -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) }, }, { @@ -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, @@ -740,7 +739,7 @@ func TestGetUserData(t *testing.T) { error: nil, }, { - secret: &apiv1.Secret{ + secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "notFound", Namespace: defaultNamespace, @@ -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, @@ -933,7 +932,7 @@ func TestCreate(t *testing.T) { }, }, }, - userDataSecret: &apiv1.Secret{ + userDataSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: userDataSecretName, Namespace: defaultNamespace, diff --git a/pkg/actuators/machine/utils.go b/pkg/actuators/machine/utils.go index 909bee3627..23e5f0432a 100644 --- a/pkg/actuators/machine/utils.go +++ b/pkg/actuators/machine/utils.go @@ -171,22 +171,6 @@ func getInstances(machine *machinev1.Machine, client awsclient.Client, instanceS return instances, nil } -func getVolume(client awsclient.Client, volumeID string) (*ec2.Volume, error) { - request := &ec2.DescribeVolumesInput{ - VolumeIds: []*string{&volumeID}, - } - result, err := client.DescribeVolumes(request) - if err != nil { - return &ec2.Volume{}, err - } - - if len(result.Volumes) != 1 { - return &ec2.Volume{}, fmt.Errorf("unable to get volume ID: %q", volumeID) - } - - return result.Volumes[0], nil -} - // terminateInstances terminates all provided instances with a single EC2 request. func terminateInstances(client awsclient.Client, instances []*ec2.Instance) ([]*ec2.InstanceStateChange, error) { instanceIDs := []*string{} @@ -252,102 +236,53 @@ func (a *Actuator) isMaster(machine *machinev1.Machine) (bool, error) { return false, nil } -// updateConditionCheck tests whether a condition should be updated from the -// old condition to the new condition. Returns true if the condition should -// be updated. -type updateConditionCheck func(oldReason, oldMessage, newReason, newMessage string) bool - -// updateConditionAlways returns true. The condition will always be updated. -func updateConditionAlways(_, _, _, _ string) bool { - return true -} - -// updateConditionNever return false. The condition will never be updated, -// unless there is a change in the status of the condition. -func updateConditionNever(_, _, _, _ string) bool { - return false -} - -// updateConditionIfReasonOrMessageChange returns true if there is a change -// in the reason or the message of the condition. -func updateConditionIfReasonOrMessageChange(oldReason, oldMessage, newReason, newMessage string) bool { - return oldReason != newReason || - oldMessage != newMessage -} - -func shouldUpdateCondition( - oldStatus corev1.ConditionStatus, oldReason, oldMessage string, - newStatus corev1.ConditionStatus, newReason, newMessage string, - updateConditionCheck updateConditionCheck, -) bool { - if oldStatus != newStatus { - return true - } - return updateConditionCheck(oldReason, oldMessage, newReason, newMessage) -} - // setAWSMachineProviderCondition sets the condition for the machine and // returns the new slice of conditions. // If the machine does not already have a condition with the specified type, -// a condition will be added to the slice if and only if the specified -// status is True. +// a condition will be added to the slice // If the machine does already have a condition with the specified type, // the condition will be updated if either of the following are true. -// 1) Requested status is different than existing status. -// 2) The updateConditionCheck function returns true. -func setAWSMachineProviderCondition( - conditions []providerconfigv1.AWSMachineProviderCondition, - conditionType providerconfigv1.AWSMachineProviderConditionType, - status corev1.ConditionStatus, - reason string, - message string, - updateConditionCheck updateConditionCheck, -) []providerconfigv1.AWSMachineProviderCondition { +func setAWSMachineProviderCondition(condition providerconfigv1.AWSMachineProviderCondition, conditions []providerconfigv1.AWSMachineProviderCondition) []providerconfigv1.AWSMachineProviderCondition { now := metav1.Now() - existingCondition := findAWSMachineProviderCondition(conditions, conditionType) - if existingCondition == nil { - if status == corev1.ConditionTrue { - conditions = append( - conditions, - providerconfigv1.AWSMachineProviderCondition{ - Type: conditionType, - Status: status, - Reason: reason, - Message: message, - LastTransitionTime: now, - LastProbeTime: now, - }, - ) - } + + if existingCondition := findProviderCondition(conditions, condition.Type); existingCondition == nil { + condition.LastProbeTime = now + condition.LastTransitionTime = now + conditions = append(conditions, condition) } else { - if shouldUpdateCondition( - existingCondition.Status, existingCondition.Reason, existingCondition.Message, - status, reason, message, - updateConditionCheck, - ) { - if existingCondition.Status != status { - existingCondition.LastTransitionTime = now - } - existingCondition.Status = status - existingCondition.Reason = reason - existingCondition.Message = message - existingCondition.LastProbeTime = now - } + updateExistingCondition(&condition, existingCondition) } + return conditions } -// findAWSMachineProviderCondition finds in the machine the condition that has the -// specified condition type. If none exists, then returns nil. -func findAWSMachineProviderCondition(conditions []providerconfigv1.AWSMachineProviderCondition, conditionType providerconfigv1.AWSMachineProviderConditionType) *providerconfigv1.AWSMachineProviderCondition { - for i, condition := range conditions { - if condition.Type == conditionType { +func findProviderCondition(conditions []providerconfigv1.AWSMachineProviderCondition, conditionType providerconfigv1.AWSMachineProviderConditionType) *providerconfigv1.AWSMachineProviderCondition { + for i := range conditions { + if conditions[i].Type == conditionType { return &conditions[i] } } return nil } +func updateExistingCondition(newCondition, existingCondition *providerconfigv1.AWSMachineProviderCondition) { + if !shouldUpdateCondition(newCondition, existingCondition) { + return + } + + if existingCondition.Status != newCondition.Status { + existingCondition.LastTransitionTime = metav1.Now() + } + existingCondition.Status = newCondition.Status + existingCondition.Reason = newCondition.Reason + existingCondition.Message = newCondition.Message + existingCondition.LastProbeTime = newCondition.LastProbeTime +} + +func shouldUpdateCondition(newCondition, existingCondition *providerconfigv1.AWSMachineProviderCondition) bool { + return newCondition.Reason != existingCondition.Reason || newCondition.Message != existingCondition.Message +} + // extractNodeAddresses maps the instance information from EC2 to an array of NodeAddresses func extractNodeAddresses(instance *ec2.Instance) ([]corev1.NodeAddress, error) { // Not clear if the order matters here, but we might as well indicate a sensible preference order @@ -413,3 +348,20 @@ func extractNodeAddresses(instance *ec2.Instance) ([]corev1.NodeAddress, error) return addresses, nil } + +func conditionSuccess() providerconfigv1.AWSMachineProviderCondition { + return providerconfigv1.AWSMachineProviderCondition{ + Type: providerconfigv1.MachineCreation, + Status: corev1.ConditionTrue, + Reason: providerconfigv1.MachineCreationSucceeded, + Message: "Machine successfully created", + } +} + +func conditionFailed() providerconfigv1.AWSMachineProviderCondition { + return providerconfigv1.AWSMachineProviderCondition{ + Type: providerconfigv1.MachineCreation, + Status: corev1.ConditionFalse, + Reason: providerconfigv1.MachineCreationFailed, + } +} diff --git a/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go b/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go index df9dd78e34..96072c738b 100644 --- a/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go +++ b/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go @@ -53,14 +53,24 @@ type AWSMachineProviderStatus struct { // AWSMachineProviderConditionType is a valid value for AWSMachineProviderCondition.Type type AWSMachineProviderConditionType string -// Valid conditions for an AWS machine instance +// Valid conditions for an AWS machine instance. const ( // MachineCreation indicates whether the machine has been created or not. If not, // it should include a reason and message for the failure. MachineCreation AWSMachineProviderConditionType = "MachineCreation" ) -// AWSMachineProviderCondition is a condition in a AWSMachineProviderStatus +// AWSMachineProviderConditionReason is reason for the condition's last transition. +type AWSMachineProviderConditionReason string + +const ( + // MachineCreationSucceeded indicates machine creation success. + MachineCreationSucceeded AWSMachineProviderConditionReason = "MachineCreationSucceeded" + // MachineCreationFailed indicates machine creation failure. + MachineCreationFailed AWSMachineProviderConditionReason = "MachineCreationFailed" +) + +// AWSMachineProviderCondition is a condition in a AWSMachineProviderStatus. type AWSMachineProviderCondition struct { // Type is the type of the condition. Type AWSMachineProviderConditionType `json:"type"` @@ -74,7 +84,7 @@ type AWSMachineProviderCondition struct { LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"` // Reason is a unique, one-word, CamelCase reason for the condition's last transition. // +optional - Reason string `json:"reason,omitempty"` + Reason AWSMachineProviderConditionReason `json:"reason,omitempty"` // Message is a human-readable message indicating details about last transition. // +optional Message string `json:"message,omitempty"`