Skip to content

Commit

Permalink
openstackmachine: do not set transient error message and reason
Browse files Browse the repository at this point in the history 
Signed-off-by: Sean Schneeweiss <sean.schneeweiss@mercedes-benz.com>
  • Loading branch information
@seanschneeweiss
seanschneeweiss committed Sep 25, 2022
1 parent acab3dc commit 109b017
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 34 deletions.
11 changes: 11 additions & 0 deletions api/v1alpha6/openstackmachine_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package v1alpha6
import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
"sigs.k8s.io/cluster-api/errors"
)
Expand Down Expand Up @@ -174,6 +175,16 @@ func (r *OpenStackMachine) SetConditions(conditions clusterv1.Conditions) {
r.Status.Conditions = conditions
}

// SetFailureMessage sets the OpenStackMachine status failure message.
func (r *OpenStackMachine) SetFailureMessage(err error) {
r.Status.FailureMessage = pointer.StringPtr(err.Error())
}

// SetFailureReason sets the OpenStackMachine status failure reason.
func (r *OpenStackMachine) SetFailureReason(err errors.MachineStatusError) {
r.Status.FailureReason = &err
}

func init() {
SchemeBuilder.Register(&OpenStackMachine{}, &OpenStackMachineList{})
}
55 changes: 21 additions & 34 deletions controllers/openstackmachine_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ package controllers
import (
"context"
"encoding/base64"
"errors"
"fmt"
"reflect"
"time"

"github.com/go-logr/logr"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -261,17 +261,17 @@ func (r *OpenStackMachineReconciler) reconcileDelete(ctx context.Context, scope
if instanceStatus != nil {
instanceNS, err := instanceStatus.NetworkStatus()
if err != nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("error getting network status for OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
openStackMachine.SetFailureReason(capierrors.UpdateMachineError)
openStackMachine.SetFailureMessage(fmt.Errorf("get network status for OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
return ctrl.Result{}, nil
}

addresses := instanceNS.Addresses()
for _, address := range addresses {
if address.Type == corev1.NodeExternalIP {
if err = networkingService.DeleteFloatingIP(openStackMachine, address.Address); err != nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("error deleting Openstack floating IP: %v", err))
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.FloatingIPErrorReason, clusterv1.ConditionSeverityError, "Deleting floating IP failed: %v", err)
return ctrl.Result{}, nil
return ctrl.Result{}, fmt.Errorf("delete floating IP %q: %w", address.Address, err)
}
}
}
Expand All @@ -280,16 +280,14 @@ func (r *OpenStackMachineReconciler) reconcileDelete(ctx context.Context, scope

instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
if err != nil {
err = errors.Errorf("machine spec is invalid: %v", err)
handleUpdateMachineError(scope.Logger, openStackMachine, err)
err = fmt.Errorf("machine spec is invalid: %w", err)
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InvalidMachineSpecReason, clusterv1.ConditionSeverityError, err.Error())
return ctrl.Result{}, err
}

if err := computeService.DeleteInstance(openStackMachine, instanceSpec, instanceStatus); err != nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("error deleting OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err)
return ctrl.Result{}, nil
return ctrl.Result{}, fmt.Errorf("delete instance: %w", err)
}

controllerutil.RemoveFinalizer(openStackMachine, infrav1.MachineFinalizer)
Expand Down Expand Up @@ -346,14 +344,15 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope

instanceStatus, err := r.getOrCreate(scope.Logger, cluster, openStackCluster, machine, openStackMachine, computeService, userData)
if err != nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("OpenStack instance cannot be created: %v", err))
// Conditions set in getOrCreate
return ctrl.Result{}, err
}

// Set an error message if we couldn't find the instance.
if instanceStatus == nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.New("OpenStack instance cannot be found"))
err = errors.New("OpenStack instance not found")
openStackMachine.SetFailureReason(capierrors.UpdateMachineError)
openStackMachine.SetFailureMessage(err)
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, "")
return ctrl.Result{}, nil
}
Expand All @@ -368,8 +367,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope

instanceNS, err := instanceStatus.NetworkStatus()
if err != nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("Unable to get network status for OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
return ctrl.Result{}, nil
return ctrl.Result{}, fmt.Errorf("get network status: %w", err)
}

addresses := instanceNS.Addresses()
Expand All @@ -382,7 +380,10 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
openStackMachine.Status.Ready = true
case infrav1.InstanceStateError:
// Error is unexpected, thus we report error and never retry
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("OpenStack instance state %q is unexpected", instanceStatus.State()))
scope.Logger.Info("Machine instance is ERROR", "instance-id", instanceStatus.ID())
err = fmt.Errorf("instance state %q is unexpected", instanceStatus.State())
openStackMachine.SetFailureReason(capierrors.UpdateMachineError)
openStackMachine.SetFailureMessage(err)
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceStateErrorReason, clusterv1.ConditionSeverityError, "")
return ctrl.Result{}, nil
case infrav1.InstanceStateDeleted:
Expand All @@ -406,9 +407,8 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
if openStackCluster.Spec.APIServerLoadBalancer.Enabled {
err = r.reconcileLoadBalancerMember(scope, openStackCluster, machine, openStackMachine, instanceNS, clusterName)
if err != nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("LoadBalancerMember cannot be reconciled: %v", err))
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.LoadBalancerMemberErrorReason, clusterv1.ConditionSeverityError, "Reconciling load balancer member failed: %v", err)
return ctrl.Result{}, nil
return ctrl.Result{}, fmt.Errorf("reconcile load balancer member: %w", err)
}
} else if !openStackCluster.Spec.DisableAPIServerFloatingIP {
floatingIPAddress := openStackCluster.Spec.ControlPlaneEndpoint.Host
Expand All @@ -417,26 +417,22 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
}
fp, err := networkingService.GetOrCreateFloatingIP(openStackMachine, openStackCluster, clusterName, floatingIPAddress)
if err != nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("Floating IP cannot be got or created: %v", err))
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.FloatingIPErrorReason, clusterv1.ConditionSeverityError, "Floating IP cannot be obtained or created: %v", err)
return ctrl.Result{}, nil
return ctrl.Result{}, fmt.Errorf("get or create floating IP %q: %w", floatingIPAddress, err)
}
port, err := computeService.GetManagementPort(openStackCluster, instanceStatus)
if err != nil {
err = errors.Errorf("getting management port for control plane machine %s: %v", machine.Name, err)
handleUpdateMachineError(scope.Logger, openStackMachine, err)
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.FloatingIPErrorReason, clusterv1.ConditionSeverityError, "Obtaining management port for control plane machine failed: %v", err)
return ctrl.Result{}, nil
return ctrl.Result{}, fmt.Errorf("get management port for control plane machine: %w", err)
}

if len(fp.PortID) != 0 {
scope.Logger.Info("Floating IP already associated to a port:", "id", fp.ID, "fixed ip", fp.FixedIP, "portID", port.ID)
} else {
err = networkingService.AssociateFloatingIP(openStackMachine, fp, port.ID)
if err != nil {
handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("Floating IP cannot be associated: %v", err))
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.FloatingIPErrorReason, clusterv1.ConditionSeverityError, "Associating floating IP failed: %v", err)
return ctrl.Result{}, nil
return ctrl.Result{}, fmt.Errorf("associate floating IP %q with port %q: %w", fp.FloatingIP, port.ID, err)
}
}
}
Expand All @@ -456,16 +452,15 @@ func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *cl
logger.Info("Machine not exist, Creating Machine", "Machine", openStackMachine.Name)
instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, userData)
if err != nil {
err = errors.Errorf("machine spec is invalid: %v", err)
handleUpdateMachineError(logger, openStackMachine, err)
err = fmt.Errorf("machine spec is invalid: %w", err)
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InvalidMachineSpecReason, clusterv1.ConditionSeverityError, err.Error())
return nil, err
}

instanceStatus, err = computeService.CreateInstance(openStackMachine, openStackCluster, instanceSpec, cluster.Name)
if err != nil {
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceCreateFailedReason, clusterv1.ConditionSeverityError, err.Error())
return nil, errors.Errorf("error creating Openstack instance: %v", err)
return nil, fmt.Errorf("create OpenStack instance: %w", err)
}
}

Expand Down Expand Up @@ -541,14 +536,6 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *
return &instanceSpec, nil
}

func handleUpdateMachineError(logger logr.Logger, openstackMachine *infrav1.OpenStackMachine, message error) {
err := capierrors.UpdateMachineError
openstackMachine.Status.FailureReason = &err
openstackMachine.Status.FailureMessage = pointer.StringPtr(message.Error())
// TODO remove if this error is logged redundantly
logger.Error(fmt.Errorf(string(err)), message.Error())
}

func (r *OpenStackMachineReconciler) reconcileLoadBalancerMember(scope *scope.Scope, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, instanceNS *compute.InstanceNetworkStatus, clusterName string) error {
ip := instanceNS.IP(openStackCluster.Status.Network.Name)
loadbalancerService, err := loadbalancer.NewService(scope)
Expand Down Expand Up @@ -599,7 +586,7 @@ func (r *OpenStackMachineReconciler) getBootstrapData(ctx context.Context, machi
secret := &corev1.Secret{}
key := types.NamespacedName{Namespace: machine.Namespace, Name: *machine.Spec.Bootstrap.DataSecretName}
if err := r.Client.Get(ctx, key, secret); err != nil {
return "", errors.Wrapf(err, "failed to retrieve bootstrap data secret for Openstack Machine %s/%s", machine.Namespace, openStackMachine.Name)
return "", fmt.Errorf("failed to retrieve bootstrap data secret for Openstack Machine %s/%s: %w", machine.Namespace, openStackMachine.Name, err)
}

value, ok := secret.Data["value"]
Expand Down

0 comments on commit 109b017

Please sign in to comment.