Skip to content

Commit

Permalink
refactoring to reduce cyclomatic complexity in machine controller
Browse files Browse the repository at this point in the history
  • Loading branch information
@AshleyDumaine
AshleyDumaine committed Mar 8, 2024
1 parent 158b033 commit c9e04b2
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 114 deletions.
2 changes: 1 addition & 1 deletion .golangci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ linters-settings:
local-prefixes: github.com/crossplane/provider-template

cyclop:
max-complexity: 17
max-complexity: 15

maligned:
# print struct with more effective memory layout or not, false by default
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions cloud/services/loadbalancers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ func AddNodeToNB(
ctx context.Context,
logger logr.Logger,
machineScope *scope.MachineScope,
clusterScope *scope.ClusterScope,
) error {
// Update the NB backend with the new instance if it's a control plane node
if !kutil.IsControlPlaneMachine(machineScope.Machine) {
Expand All @@ -132,8 +131,8 @@ func AddNodeToNB(
}

lbPort := defaultLBPort
if clusterScope.LinodeCluster.Spec.Network.LoadBalancerPort != 0 {
lbPort = clusterScope.LinodeCluster.Spec.Network.LoadBalancerPort
if machineScope.LinodeCluster.Spec.Network.LoadBalancerPort != 0 {
lbPort = machineScope.LinodeCluster.Spec.Network.LoadBalancerPort
}
if machineScope.LinodeCluster.Spec.Network.NodeBalancerConfigID == nil {
err := errors.New("nil NodeBalancer Config ID")
Expand Down
6 changes: 3 additions & 3 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ spec:
CredentialsRef is a reference to a Secret that contains the credentials
to use for provisioning this machine. If not supplied then these
credentials will be used in-order:
1. Machine
2. Cluster
2. Controller
1. LinodeMachine
2. Owner LinodeCluster
3. Controller
properties:
name:
description: name is unique within a namespace to reference a
Expand Down
6 changes: 3 additions & 3 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ spec:
CredentialsRef is a reference to a Secret that contains the credentials
to use for provisioning this machine. If not supplied then these
credentials will be used in-order:
1. Machine
2. Cluster
2. Controller
1. LinodeMachine
2. Owner LinodeCluster
3. Controller
properties:
name:
description: name is unique within a namespace to reference
Expand Down
114 changes: 11 additions & 103 deletions controller/linodemachine_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,76 +103,31 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques
linodeMachine := &infrav1alpha1.LinodeMachine{}
if err := r.Client.Get(ctx, req.NamespacedName, linodeMachine); err != nil {
if err = client.IgnoreNotFound(err); err != nil {
log.Error(err, "Failed to fetch Linode machine")
log.Error(err, "Failed to fetch LinodeMachine")
}

return ctrl.Result{}, err
}

machine, err := kutil.GetOwnerMachine(ctx, r.Client, linodeMachine.ObjectMeta)
switch {
case err != nil:
if err = client.IgnoreNotFound(err); err != nil {
log.Error(err, "Failed to fetch owner machine")
}

machine, err := r.getOwnerMachine(ctx, *linodeMachine, log)
if err != nil || machine == nil {
return ctrl.Result{}, err
case machine == nil:
log.Info("Machine Controller has not yet set OwnerRef, skipping reconciliation")

return ctrl.Result{}, nil
case skippedMachinePhases[machine.Status.Phase]:

return ctrl.Result{}, nil
default:
match := false
for i := range linodeMachine.OwnerReferences {
if match = linodeMachine.OwnerReferences[i].UID == machine.UID; match {
break
}
}

if !match {
log.Info("Failed to find the referenced owner machine, skipping reconciliation", "references", linodeMachine.OwnerReferences, "machine", machine.ObjectMeta)

return ctrl.Result{}, nil
}
}
log = log.WithValues("LinodeMachine: ", machine.Name)

log = log.WithValues("Linode machine: ", machine.Name)

cluster, err := kutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta)
switch {
case err != nil:
if err = client.IgnoreNotFound(err); err != nil {
log.Error(err, "Failed to fetch cluster by label")
}

return ctrl.Result{}, err
case cluster == nil:
err = errors.New("missing cluster")

log.Error(err, "Missing cluster")

return ctrl.Result{}, err
case cluster.Spec.InfrastructureRef == nil:
err = errors.New("missing infrastructure reference")

log.Error(err, "Missing infrastructure reference")

cluster, err := r.getClusterFromMetadata(ctx, *machine, log)
if err != nil || cluster == nil {
return ctrl.Result{}, err
}

linodeCluster := &infrav1alpha1.LinodeCluster{}

machineScope, err := scope.NewMachineScope(
ctx,
r.LinodeApiKey,
scope.MachineScopeParams{
Client: r.Client,
Cluster: cluster,
Machine: machine,
LinodeCluster: linodeCluster,
LinodeCluster: &infrav1alpha1.LinodeCluster{},
LinodeMachine: linodeMachine,
},
)
Expand All @@ -182,29 +137,13 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques
return ctrl.Result{}, fmt.Errorf("failed to create machine scope: %w", err)
}

clusterScope, err := scope.NewClusterScope(
ctx,
r.LinodeApiKey,
scope.ClusterScopeParams{
Client: r.Client,
Cluster: cluster,
LinodeCluster: linodeCluster,
},
)
if err != nil {
log.Error(err, "Failed to create cluster scope")

return ctrl.Result{}, fmt.Errorf("failed to create cluster scope: %w", err)
}

return r.reconcile(ctx, log, machineScope, clusterScope)
return r.reconcile(ctx, log, machineScope)
}

func (r *LinodeMachineReconciler) reconcile(
ctx context.Context,
logger logr.Logger,
machineScope *scope.MachineScope,
clusterScope *scope.ClusterScope,
) (res ctrl.Result, err error) {
res = ctrl.Result{}

Expand Down Expand Up @@ -290,7 +229,7 @@ func (r *LinodeMachineReconciler) reconcile(

return
}
linodeInstance, err = r.reconcileCreate(ctx, logger, machineScope, clusterScope)
linodeInstance, err = r.reconcileCreate(ctx, logger, machineScope)

return
}
Expand All @@ -299,7 +238,6 @@ func (r *LinodeMachineReconciler) reconcileCreate(
ctx context.Context,
logger logr.Logger,
machineScope *scope.MachineScope,
clusterScope *scope.ClusterScope,
) (*linodego.Instance, error) {
logger.Info("creating machine")

Expand Down Expand Up @@ -332,17 +270,6 @@ func (r *LinodeMachineReconciler) reconcileCreate(
return nil, err
}

if machineScope.LinodeCluster.Spec.VPCRef != nil {
iface, err := r.getVPCInterfaceConfig(ctx, machineScope, createOpts.Interfaces, logger)
if err != nil {
logger.Error(err, "Failed to get VPC interface config")

return nil, err
}

createOpts.Interfaces = append(createOpts.Interfaces, *iface)
}

linodeInstance, err = machineScope.LinodeClient.CreateInstance(ctx, *createOpts)
if err != nil {
logger.Error(err, "Failed to create Linode machine instance")
Expand Down Expand Up @@ -370,31 +297,12 @@ func (r *LinodeMachineReconciler) reconcileCreate(
return nil, err
}

if linodeInstance == nil {
err = errors.New("missing instance")

logger.Error(err, "Panic! Failed to create isntance")

return nil, err
}

machineScope.LinodeMachine.Status.Ready = true
machineScope.LinodeMachine.Spec.InstanceID = &linodeInstance.ID
machineScope.LinodeMachine.Spec.ProviderID = util.Pointer(fmt.Sprintf("linode://%d", linodeInstance.ID))
machineScope.LinodeMachine.Status.Addresses = buildInstanceAddrs(linodeInstance)

machineScope.LinodeMachine.Status.Addresses = []clusterv1.MachineAddress{}
for _, addr := range linodeInstance.IPv4 {
addrType := clusterv1.MachineExternalIP
if addr.IsPrivate() {
addrType = clusterv1.MachineInternalIP
}
machineScope.LinodeMachine.Status.Addresses = append(machineScope.LinodeMachine.Status.Addresses, clusterv1.MachineAddress{
Type: addrType,
Address: addr.String(),
})
}

if err = services.AddNodeToNB(ctx, logger, machineScope, clusterScope); err != nil {
if err = services.AddNodeToNB(ctx, logger, machineScope); err != nil {
logger.Error(err, "Failed to add instance to Node Balancer backend")

return linodeInstance, err
Expand Down
86 changes: 85 additions & 1 deletion controller/linodemachine_controller_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ import (
// The decoded user_data must not exceed 16384 bytes per the Linode API
const maxBootstrapDataBytes = 16384

func (*LinodeMachineReconciler) newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, tags []string, logger logr.Logger) (*linodego.InstanceCreateOptions, error) {
func (r *LinodeMachineReconciler) newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, tags []string, logger logr.Logger) (*linodego.InstanceCreateOptions, error) {
var err error

createConfig := linodeMachineSpecToInstanceCreateConfig(machineScope.LinodeMachine.Spec)
Expand Down Expand Up @@ -94,9 +94,93 @@ func (*LinodeMachineReconciler) newCreateConfig(ctx context.Context, machineScop
createConfig.RootPass = uuid.NewString()
}

if machineScope.LinodeCluster.Spec.VPCRef != nil {
iface, err := r.getVPCInterfaceConfig(ctx, machineScope, createConfig.Interfaces, logger)
if err != nil {
logger.Error(err, "Failed to get VPC interface config")

return nil, err
}
createConfig.Interfaces = append(createConfig.Interfaces, *iface)
}

return createConfig, nil
}

func buildInstanceAddrs(linodeInstance *linodego.Instance) []clusterv1.MachineAddress {
addrs := []clusterv1.MachineAddress{}
for _, addr := range linodeInstance.IPv4 {
addrType := clusterv1.MachineExternalIP
if addr.IsPrivate() {
addrType = clusterv1.MachineInternalIP
}
addrs = append(addrs, clusterv1.MachineAddress{
Type: addrType,
Address: addr.String(),
})
}

return addrs
}

func (r *LinodeMachineReconciler) getOwnerMachine(ctx context.Context, linodeMachine infrav1alpha1.LinodeMachine, log logr.Logger) (*clusterv1.Machine, error) {
machine, err := kutil.GetOwnerMachine(ctx, r.Client, linodeMachine.ObjectMeta)
switch {
case err != nil:
if err = client.IgnoreNotFound(err); err != nil {
log.Error(err, "Failed to fetch owner machine")
}

return nil, err
case machine == nil:
log.Info("Machine Controller has not yet set OwnerRef, skipping reconciliation")

return nil, err
case skippedMachinePhases[machine.Status.Phase]:

return nil, err
default:
match := false
for i := range linodeMachine.OwnerReferences {
if match = linodeMachine.OwnerReferences[i].UID == machine.UID; match {
break
}
}
if !match {
log.Info("Failed to find the referenced owner machine, skipping reconciliation", "references", linodeMachine.OwnerReferences, "machine", machine.ObjectMeta)

return nil, err
}
}

return machine, nil
}

func (r *LinodeMachineReconciler) getClusterFromMetadata(ctx context.Context, machine clusterv1.Machine, log logr.Logger) (*clusterv1.Cluster, error) {
cluster, err := kutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta)
switch {
case err != nil:
if err = client.IgnoreNotFound(err); err != nil {
log.Error(err, "Failed to fetch cluster by label")
}

return nil, err
case cluster == nil:
err = errors.New("missing cluster")
log.Error(err, "Missing cluster")

return nil, err
case cluster.Spec.InfrastructureRef == nil:
err = errors.New("missing infrastructure reference")

log.Error(err, "Missing infrastructure reference")

return nil, err
}

return cluster, nil
}

func (r *LinodeMachineReconciler) linodeClusterToLinodeMachines(logger logr.Logger) handler.MapFunc {
logger = logger.WithName("LinodeMachineReconciler").WithName("linodeClusterToLinodeMachines")

Expand Down

0 comments on commit c9e04b2

Please sign in to comment.