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

🐛 openstackmachine: do not set transient error message and reason #1301

Merged
merged 1 commit into from
Nov 9, 2022
Merged

🐛 openstackmachine: do not set transient error message and reason #1301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions api/v1alpha6/openstackmachine_types.go
View file
Open in desktop
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,12 @@ func (r *OpenStackMachine) SetConditions(conditions clusterv1.Conditions) {
r.Status.Conditions = conditions
}

// SetFailure sets the OpenStackMachine status failure reason and failure message.
func (r *OpenStackMachine) SetFailure(failureReason errors.MachineStatusError, failureMessage error) {
r.Status.FailureReason = &failureReason
r.Status.FailureMessage = pointer.StringPtr(failureMessage.Error())
}

func init() {
SchemeBuilder.Register(&OpenStackMachine{}, &OpenStackMachineList{})
}
70 changes: 24 additions & 46 deletions controllers/openstackmachine_controller.go
View file
Open in desktop
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,27 +261,28 @@ 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.SetFailure(
capierrors.UpdateMachineError,
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)
}
}
}
}
}

if err := computeService.DeleteInstance(openStackMachine, instanceStatus, openStackMachine.Name, openStackMachine.Spec.RootVolume); 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 @@ -338,14 +339,14 @@ 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.SetFailure(capierrors.UpdateMachineError, err)
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, "")
return ctrl.Result{}, nil
}
Expand All @@ -360,26 +361,27 @@ 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()
openStackMachine.Status.Addresses = addresses

switch instanceStatus.State() {
case infrav1.InstanceStateActive:
scope.Logger.Info("Machine instance is ACTIVE", "instance-id", instanceStatus.ID())
scope.Logger.Info("Machine instance state is ACTIVE", "instance-id", instanceStatus.ID())
conditions.MarkTrue(openStackMachine, infrav1.InstanceReadyCondition)
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 state is ERROR", "instance-id", instanceStatus.ID())
err = fmt.Errorf("instance state %q is unexpected", instanceStatus.State())
openStackMachine.SetFailure(capierrors.UpdateMachineError, err)
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceStateErrorReason, clusterv1.ConditionSeverityError, "")
return ctrl.Result{}, nil
case infrav1.InstanceStateDeleted:
// we should avoid further actions for DELETED VM
scope.Logger.Info("Instance state is DELETED, no actions")
scope.Logger.Info("Machine instance state is DELETED, no actions")
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeletedReason, clusterv1.ConditionSeverityError, "")
return ctrl.Result{}, nil
default:
Expand All @@ -398,9 +400,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 @@ -409,26 +410,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 fp.PortID != "" {
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 @@ -445,30 +442,19 @@ func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *cl
}

if instanceStatus == nil {
instanceSpec := machineToInstanceSpec(openStackCluster, machine, openStackMachine, userData)
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)
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)
}
}

return instanceStatus, nil
}

func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, userData string) (*compute.InstanceSpec, error) {
if openStackMachine == nil {
return nil, fmt.Errorf("create Options need be specified to create instace")
}

func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, userData string) *compute.InstanceSpec {
instanceSpec := compute.InstanceSpec{
Name: openStackMachine.Name,
Image: openStackMachine.Spec.Image,
Expand Down Expand Up @@ -530,15 +516,7 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *
instanceSpec.Networks = openStackMachine.Spec.Networks
instanceSpec.Ports = openStackMachine.Spec.Ports

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("%s", string(err)), message.Error())
return &instanceSpec
}

func (r *OpenStackMachineReconciler) reconcileLoadBalancerMember(scope *scope.Scope, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, instanceNS *compute.InstanceNetworkStatus, clusterName string) error {
Expand Down Expand Up @@ -591,7 +569,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
14 changes: 1 addition & 13 deletions controllers/openstackmachine_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,13 @@ func Test_machineToInstanceSpec(t *testing.T) {
machine func() *clusterv1.Machine
openStackMachine func() *infrav1.OpenStackMachine
wantInstanceSpec func() *compute.InstanceSpec
wantErr bool
}{
{
name: "Defaults",
openStackCluster: getDefaultOpenStackCluster,
machine: getDefaultMachine,
openStackMachine: getDefaultOpenStackMachine,
wantInstanceSpec: getDefaultInstanceSpec,
wantErr: false,
},
{
name: "Control plane security group",
Expand All @@ -149,7 +147,6 @@ func Test_machineToInstanceSpec(t *testing.T) {
i.SecurityGroups = []infrav1.SecurityGroupParam{{UUID: controlPlaneSecurityGroupUUID}}
return i
},
wantErr: false,
},
{
name: "Worker security group",
Expand All @@ -165,7 +162,6 @@ func Test_machineToInstanceSpec(t *testing.T) {
i.SecurityGroups = []infrav1.SecurityGroupParam{{UUID: workerSecurityGroupUUID}}
return i
},
wantErr: false,
},
{
name: "Extra security group",
Expand All @@ -188,7 +184,6 @@ func Test_machineToInstanceSpec(t *testing.T) {
}
return i
},
wantErr: false,
},
{
name: "Tags",
Expand All @@ -208,18 +203,11 @@ func Test_machineToInstanceSpec(t *testing.T) {
i.Tags = []string{"machine-tag", "duplicate-tag", "cluster-tag"}
return i
},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := machineToInstanceSpec(tt.openStackCluster(), tt.machine(), tt.openStackMachine(), "user-data")
if tt.wantErr {
Expect(err).To(HaveOccurred())
} else {
Expect(err).NotTo(HaveOccurred())
}

got := machineToInstanceSpec(tt.openStackCluster(), tt.machine(), tt.openStackMachine(), "user-data")
Expect(got).To(Equal(tt.wantInstanceSpec()))
})
}
Expand Down