Refactor Tinkerbell provider #1830

moadqassem · 2024-07-25T15:06:31Z

What this PR does / why we need it:
In a previous PR here, we started a major refactoring for the tinkerbell provider adding new features and removing deprecated ones. This PR however, address a lot of changes for the original one as the current author is on a long vacation and we need this changes in ASAP. For more details on the functionality and what points this PR amends please take a look at the previous PR and its review.
Which issue(s) this PR fixes:

Fixes #

What type of PR is this?

Special notes for your reviewer:

Does this PR introduce a user-facing change? Then add your Release Note here:

Refactor the tinkerbell provider

Documentation:

None

Signed-off-by: moadqassem <moad.qassem@gmail.com>

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/hardware.go

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/template.go

xrstf · 2024-07-25T16:43:58Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/template.go

+func createGrowPartitionAction(destDisk string) Action {
+	return Action{
+		Name:    "grow-partition",
+		Image:   "quay.io/tinkerbell/actions/cexec:latest",


This could also be pinned to c5bde803d9f6c90f1a9d5e06930d856d1481854c, which is the same as "latest" at the moment. Or is there a reason not to pin this image?

Why? There is always enhancements that er com into latest and all our tinkerbell actions are using latest,

You pinned every single other image in every single other action. Except this one. Surely there had to be a reason. :) When I look at the code, I might now have accidentally broke it by thinking "oh, someone forgot to pin this, let me just fix that". So a comment why this one action is not pinned to a version would be appreciated.

I guess you have overlooked something, while the tags are pinned for the all the images, due the the image repo and path are different 😉

quay.io/tinkerbell/actions/cexec:latest

quay.io/tinkerbell-actions/writefile:v1.0.0

Anyway if you really would like to have this in, I have no problem with that 😉. Will pin it down

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/utils.go

xrstf · 2024-07-25T16:47:33Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/driver.go

+	ClusterName string
+	OSImageURL  string
+	TinkClient  ctrlruntimeclient.Client
+	//KubeClient     ctrlruntimeclient.Client


Is this a leftover?

Probably yeah, it is from the original PR. I just wanted to know what's the idea of this field. But this was at the beginning of my implementation, but since I have refactored quite a lot I can simply take it out.

xrstf · 2024-07-25T16:49:59Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/driver.go

+		ClusterName: tinkSpec.ClusterName.Value,
+		TinkClient:  tinkClient,
+		HardwareRef: tinkSpec.HardwareRef,
+		//KubeClient:     k8sClient,


Another leftover

Probably yeah, it is from the original PR. I just wanted to know what's the idea of this field. But this was at the beginning of my implementation, but since I have refactored quite a lot I can simply take it out.

xrstf · 2024-07-25T16:52:11Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/driver.go

+	}
+
+	if !allowProvision {
+		log.Infow("server is not allowed to be provisioned; either hardware allowPXE and allowWorkflow is set tp false", "hardware", hardware.Name)


xrstf · 2024-07-25T16:56:31Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/driver.go

+
+	if !allowProvision {
+		log.Infow("server is not allowed to be provisioned; either hardware allowPXE and allowWorkflow is set tp false", "hardware", hardware.Name)
+		return nil, nil


Returning nil, nil here will make the baremetal provider return a baremetalServer struct with nil in it, which will then later explode because its GetName() functions are not capable of handling nils.

If a server cannot be provisioned, it should IMHO be an error that can bubble up and not hidden in the MC log where few people would ever look (especially not the user who doesn't have access to MC logs in KKP systems).

Good catch! I agree this makes sense

xrstf · 2024-07-25T17:00:05Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/workflow.go

-		result = append(result, e)
+	workflow := &tinkv1alpha1.Workflow{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      workflowName + "-workflow",


Please somehow make this single line re-usable. At the moment it's both an implementation detail, but also used by the Tinkerbell driver directly (i.e. it has knowledge it should not have).

Also, is it intended that "-workflow" is appended to the workflow name, or is the parameter for this function misnamed? In the driver, the server's name is used for this parameter.

Yes we can but honestly I took over this PR only yesterday and I cannot fix everything in one shot 😅 I can address that in up coming PRs

xrstf · 2024-07-25T17:00:42Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/driver.go

-	if hw.Hardware.Network == nil {
-		return fmt.Errorf("tinkerbell hardware network configs can not be empty")
+	// Delete the associated Workflow.
+	workflowName := targetHardware.Name + "-workflow" // Assuming workflow names are derived from hardware names


(this is the spot I mean regarding leaking implementation logic)

xrstf · 2024-07-25T17:01:30Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/driver.go

 	}

-	if hw.Hardware.Metadata == "" {
-		return fmt.Errorf("tinkerbell hardware metadata can not be empty")
+	// Reset the hardware ID in the tinkerbell cluster.


ID and state, so it's more obvious that from this point on, the machine is also not Staged anymore.

xrstf · 2024-07-25T17:01:55Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/driver.go

-	if err := d.hardwareClient.Delete(ctx, string(uid)); err != nil {
-		if resourceNotFoundErr(err) {
-			return nil
+func GetConfig(driverConfig tinktypes.TinkerbellPluginSpec, aa func(configVar providerconfigtypes.ConfigVarString, envVarName string) (string, error)) (*tinktypes.Config, error) {


What is aa?

No idea tbh! part of the previous implementation(The PR that I rebased my changes on top, the one mentioned in the description). But I can take a look

Ok found out, will rename after its functionality

xrstf · 2024-07-25T17:05:02Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/driver.go

+	if driverConfig.Auth.Kubeconfig.Value != "" {
+		val, err := base64.StdEncoding.DecodeString(driverConfig.Auth.Kubeconfig.Value)
+		if err != nil {
+			// An error here means that this is not a valid base64 string
+			// We can be more explicit here with the error for visibility. Webhook will return this error if we hit this scenario.
+			return nil, fmt.Errorf("failed to decode base64 encoded kubeconfig. Expected value is a base64 encoded Kubeconfig in JSON or YAML format: %w", err)
+		}
+		config.Kubeconfig = string(val)
+	} else {


Do we really want to support this? Inlining kubeconfigs has never been a pleasant thing in KKP and I thought it's better practice to reference Secrets instead.

I kinda agree with you tbh, and it is less error prune. Ok I will adjust that to be read from a secret instead of Inlining it. But can I do it in a follow up PR instead? I kinda need this image now 😂

I guess it's a sideeffect from using providerconfigtypes.ConfigVarString for the field, but I would be totally okay with commenting that inline strings are not supported for this field.

xrstf · 2024-07-25T17:07:25Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/types/hardware.go

+func (h *Hardware) GetID() string {
+	if h.Spec.Metadata != nil &&
+		h.Spec.Metadata.Instance != nil &&
+		h.Spec.Metadata.Instance.ID != "" {


&& h.Spec.Metadata.Instance.ID != "" is redundant and can be removed

How is this redundant ? You can have a hardware object without an id.

Yeah, but if the ID is an empty string, you can just return it. Because the default would be to return an empty string anyway. You are adding a bit of unnecessary logic to this function. The function doesn't need to concern itself with how the ID field looks like, it should just return it if instance metadata exists, regardless of its value.

xrstf · 2024-07-25T17:09:29Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/types/hardware.go

+		return string(h.Status.State)
+	}
+
+	return "Unknown"


To make comparisons easier, couldn't this be one of the constants? Now this function would need to be understood as "returns a proper State type [no matter that it's currently just a string], or the random string 'Unknown'".

True can be addressed as well

xrstf · 2024-07-25T17:10:20Z

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/types/types.go

+)
+
+const (
+	Staged         string = "Staged"


A dedicated type HardwareState string would help identify what these constants are used for.

Honestly I added that then I took out as I actually see no real value other than the hassle of keep casting the value when comparing it with the API field. Especially the API doesn't know this HardwareState or isn't considering as an Enum.

Signed-off-by: moadqassem <moad.qassem@gmail.com>

xrstf · 2024-07-25T20:28:26Z

/approve
/lgtm

kubermatic-bot · 2024-07-25T20:28:31Z

LGTM label has been added.

Git tree hash: be437d850b7526466e76ce80553cc8ea6adcb773

kubermatic-bot · 2024-07-25T20:28:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xrstf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xrstf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

moadqassem mentioned this pull request Jul 25, 2024

Update Tinkerbell Plugin to Support Kubernetes CRDs #1815

Closed

kubermatic-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 25, 2024

moadqassem requested a review from xrstf July 25, 2024 15:07

mohamed-rafraf and others added 8 commits July 25, 2024 17:18

update baremetal provider

5c49ded

fix lint issues

8748861

refactor tinkerbell clients

ec100c0

Signed-off-by: moadqassem <moad.qassem@gmail.com>

add netmask to cidr utility

43a3b4f

Signed-off-by: moadqassem <moad.qassem@gmail.com>

refactor tinkerbell bare metal plugin

3dc6aff

add bare metal machine states

0f988a3

refactor the bare metal provider

5a4b8d5

remove hegelurl

ec017c7

moadqassem force-pushed the refactor-tinkerbell-provider branch from af54b25 to ec017c7 Compare July 25, 2024 15:19

xrstf reviewed Jul 25, 2024

View reviewed changes

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/hardware.go Outdated Show resolved Hide resolved

xrstf reviewed Jul 25, 2024

View reviewed changes

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/template.go Show resolved Hide resolved

xrstf reviewed Jul 25, 2024

View reviewed changes

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/template.go Outdated Show resolved Hide resolved

xrstf reviewed Jul 25, 2024

View reviewed changes

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/template.go Outdated Show resolved Hide resolved

xrstf reviewed Jul 25, 2024

View reviewed changes

pkg/cloudprovider/provider/baremetal/plugins/tinkerbell/client/utils.go Show resolved Hide resolved

xrstf reviewed Jul 25, 2024

View reviewed changes

moadqassem added 4 commits July 25, 2024 19:43

addressing PR review

bb8d79d

Signed-off-by: moadqassem <moad.qassem@gmail.com>

linting

fbe7d6a

Signed-off-by: moadqassem <moad.qassem@gmail.com>

remove the metadata client as it is not needed

549cdca

Signed-off-by: moadqassem <moad.qassem@gmail.com>

linting

4b80b0f

Signed-off-by: moadqassem <moad.qassem@gmail.com>

moadqassem force-pushed the refactor-tinkerbell-provider branch from d36b200 to 4b80b0f Compare July 25, 2024 18:04

kubermatic-bot assigned xrstf Jul 25, 2024

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2024

kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2024

kubermatic-bot merged commit d89c4fb into kubermatic:main Jul 25, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Tinkerbell provider #1830

Refactor Tinkerbell provider #1830

moadqassem commented Jul 25, 2024 •

edited

Loading

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

moadqassem Jul 25, 2024 •

edited

Loading

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

xrstf Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024

xrstf Jul 25, 2024

moadqassem Jul 25, 2024 •

edited

Loading

xrstf commented Jul 25, 2024

kubermatic-bot commented Jul 25, 2024

kubermatic-bot commented Jul 25, 2024

Refactor Tinkerbell provider #1830

Refactor Tinkerbell provider #1830

Conversation

moadqassem commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moadqassem Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moadqassem Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

xrstf commented Jul 25, 2024

kubermatic-bot commented Jul 25, 2024

kubermatic-bot commented Jul 25, 2024

moadqassem commented Jul 25, 2024 •

edited

Loading

moadqassem Jul 25, 2024 •

edited

Loading

moadqassem Jul 25, 2024 •

edited

Loading