From bc73da27a5331d7a182700c5ea95d1296a85b719 Mon Sep 17 00:00:00 2001 From: Alexander Demichev Date: Tue, 17 Mar 2020 12:32:15 +0100 Subject: [PATCH] Refactor conditions logic --- pkg/actuators/machine/actuator.go | 25 ++-- pkg/actuators/machine/actuator_test.go | 4 +- pkg/actuators/machine/utils.go | 139 ++++++------------ .../v1beta1/awsmachineproviderconfig_types.go | 12 +- 4 files changed, 71 insertions(+), 109 deletions(-) diff --git a/pkg/actuators/machine/actuator.go b/pkg/actuators/machine/actuator.go index 41858f955b..ace4375f25 100644 --- a/pkg/actuators/machine/actuator.go +++ b/pkg/actuators/machine/actuator.go @@ -33,6 +33,7 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/tools/record" "k8s.io/klog" + "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1beta1" providerconfigv1 "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1beta1" awsclient "sigs.k8s.io/cluster-api-provider-aws/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client" @@ -43,12 +44,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 +104,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 +127,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 +215,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 +224,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 @@ -412,7 +409,7 @@ func (a *Actuator) Update(context context.Context, machine *machinev1.Machine) e a.handleMachineError(machine, mapierrors.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 @@ -453,7 +450,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 +569,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 v1beta1.AWSMachineProviderCondition) error { klog.Infof("%s: Updating status", machine.Name) // Starting with a fresh status as we assume full control of it here. @@ -603,7 +600,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..a5c4b985c2 100644 --- a/pkg/actuators/machine/actuator_test.go +++ b/pkg/actuators/machine/actuator_test.go @@ -267,7 +267,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 +307,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) }, }, { diff --git a/pkg/actuators/machine/utils.go b/pkg/actuators/machine/utils.go index 909bee3627..72723202c5 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,40 +236,6 @@ 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, @@ -293,61 +243,49 @@ func shouldUpdateCondition( // status is True. // 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 { + if condition.Status == corev1.ConditionTrue { + 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 +351,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.ConditionTrue, + Reason: providerconfigv1.MachineCreationFailed, + } +} diff --git a/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go b/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go index ecdd389e5f..c5ce98b5b6 100644 --- a/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go +++ b/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go @@ -60,6 +60,16 @@ const ( MachineCreation AWSMachineProviderConditionType = "MachineCreation" ) +// 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 fail + MachineCreationFailed AWSMachineProviderConditionReason = "MachineCreationFailed" +) + // AWSMachineProviderCondition is a condition in a AWSMachineProviderStatus type AWSMachineProviderCondition struct { // Type is the type of the condition. @@ -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"`