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

🧹 [cleanup] - refactoring to reduce cyclomatic complexity in machine controller, add missing autogenerated changes #174

Merged
merged 1 commit into from
Mar 8, 2024
Merged

🧹 [cleanup] - refactoring to reduce cyclomatic complexity in machine controller, add missing autogenerated changes #174

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
2 changes: 1 addition & 1 deletion .golangci.yml
View file
Open in desktop
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
Open in desktop

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
124 changes: 13 additions & 111 deletions controller/linodemachine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,76 +103,30 @@ 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,
LinodeMachine: linodeMachine,
},
)
Expand All @@ -182,29 +136,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 +228,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 +237,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 @@ -327,74 +264,39 @@ func (r *LinodeMachineReconciler) reconcileCreate(
// get the bootstrap data for the Linode instance and set it for create config
createOpts, err := r.newCreateConfig(ctx, machineScope, tags, logger)
if err != nil {
logger.Error(err, "Failed to create Linode machine create config")
logger.Error(err, "Failed to create Linode machine InstanceCreateOptions")

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 {
if err != nil || linodeInstance == nil {
logger.Error(err, "Failed to create Linode machine instance")

return nil, err
}

if err = r.configureDisksControlPlane(ctx, logger, machineScope, linodeInstance.ID); err != nil {
logger.Error(err, "Failed to configure instance disks")

return nil, err
}

if err = machineScope.LinodeClient.BootInstance(ctx, linodeInstance.ID, 0); err != nil {
logger.Error(err, "Failed to boot instance")

return nil, err
}

default:
err = errors.New("multiple instances")

logger.Error(err, "Panic! Multiple instances found. This might be a concurrency issue in the controller!!!", "tags", tags)

return nil, err
}

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

logger.Error(err, "Panic! Failed to create isntance")
logger.Error(err, "multiple instances found", "tags", tags)

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
Loading
Loading