Skip to content

Commit

Permalink
Refactor conditions logic
Browse files Browse the repository at this point in the history
  • Loading branch information
Alexander Demichev committed Mar 17, 2020
1 parent 44499c1 commit bc73da2
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 109 deletions.
25 changes: 11 additions & 14 deletions pkg/actuators/machine/actuator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)

Expand Down Expand Up @@ -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{}
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/actuators/machine/actuator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
},
},
{
Expand Down
139 changes: 47 additions & 92 deletions pkg/actuators/machine/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -252,102 +236,56 @@ 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.
// 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
Expand Down Expand Up @@ -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,
}
}
12 changes: 11 additions & 1 deletion pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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"`
Expand Down

0 comments on commit bc73da2

Please sign in to comment.