Baseboardmanagement controller implementation #3

pokearu · 2022-05-05T21:16:03Z

Description

This PR adds the reconciler implementation for the baseboardmanagement controller.
The controller ensures state for baseboardmanagement api type. On each reconcile loop,

Fetch the baseboardmanagement object, error if not found.
resolve the SecretReference to obtain connectivity information.
Initialize a bmc client and do a power status call. If connection error, update .Status.Conditions.
If BMC is not in desired state, set power state and update .Status.Power.

How Has This Been Tested?

I tested by generating a manifest and applying the controller to a cluster.

kind create cluster
make release-manifests
kubectl apply -f out/release/manifest.yaml

How are existing users impacted? What migration steps/scripts do we need?

No impact for existing users.

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

Signed-off-by: Aravind Ramalingam <[email protected]>

codecov-commenter · 2022-05-05T21:18:54Z

Codecov Report

Merging #3 (65acac8) into main (70769a2) will increase coverage by 54.46%.
The diff coverage is 64.13%.

@@            Coverage Diff             @@
##            main       #3       +/-   ##
==========================================
+ Coverage   0.00%   54.46%   +54.46%     
==========================================
  Files          3        4        +1     
  Lines         21      112       +91     
==========================================
+ Hits           0       61       +61     
- Misses        21       42       +21     
- Partials       0        9        +9

Impacted Files	Coverage Δ
controllers/bmcclient.go	`0.00% <0.00%> (ø)`
controllers/baseboardmanagement_controller.go	`67.03% <69.41%> (+67.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70769a2...65acac8. Read the comment docs.

config/rbac/role.yaml

controllers/baseboardmanagement_controller.go

micahhausler · 2022-05-05T21:30:53Z

controllers/baseboardmanagement_controller.go

+	}
+
+	// Initializing BMC Client connection
+	r.BMCClient.InitClient(bm.Spec.Connection.Host, "623", username, password)


Why is port hardcoded? Shouldn't that be configurable?

My bad. I was going to have a TODO to add this to the BaseboardManagement api.
Will update 👍

pkg/bmc/client/factory/factory.go

pkg/bmc/client/client.go

pkg/bmc/client/bmclib/bmclib.go

controllers/baseboardmanagement_controller.go

Signed-off-by: Aravind Ramalingam <[email protected]>

…nges Signed-off-by: Aravind Ramalingam <[email protected]>

Signed-off-by: Aravind Ramalingam <[email protected]>

…mcclient factory method Signed-off-by: Aravind Ramalingam <[email protected]>

chrisdoherty4

Looks OK. Just need to move the types around and get rid of some redundant code + some cleanup.

pkg/bmcclient/bmcclient.go

controllers/baseboardmanagement_controller.go

chrisdoherty4 · 2022-05-12T23:53:05Z

controllers/baseboardmanagement_controller.go

+	err = r.BMCClient.OpenConnection(ctx)
+	if err != nil {
+		logger.Error(err, "BMC connection failed", "host", bm.Spec.Connection.Host)
+		result, setConditionErr := r.setCondition(ctx, bm, bmcv1alpha1.ConnectionError, err.Error())


@pokearu Wants to address the conditions discussion in a separate PR so resolving for now.

controllers/baseboardmanagement_controller.go

Signed-off-by: Aravind Ramalingam <[email protected]>

chrisdoherty4

lgtm

micahhausler

Just a few minor questions/changes

controllers/baseboardmanagement_controller.go

micahhausler · 2022-05-13T22:07:53Z

controllers/baseboardmanagement_controller.go

+
+	// TODO (pokearu): Remove port hardcoding
+	// Initializing BMC Client
+	bmcClient, err := r.bmcClientFactory(ctx, bm.Spec.Connection.Host, "623", username, password)


Are you going to remove this hardcoding in this PR?

I was planning to do a quick follow up PR. Was going to add it to the BaseboardManagement api connection struct.

micahhausler · 2022-05-13T22:10:12Z

controllers/baseboardmanagement_controller.go

+	}
+	for _, reconiler := range fieldReconcilers {
+		if err := reconiler(ctx, bm, bmcClient); err != nil {
+			logger.Error(err, "Failed to reconcile BaseboardManagement", "host", bm.Spec.Connection.Host)


I think you still want to update the status on failure. You want to converge on desired state, not make it all-or-nothing

As discussed, I modified the reconciler to update status when reconciling fields 👍

micahhausler · 2022-05-13T22:16:44Z

controllers/baseboardmanagement_controller.go

+
+// patchStatus patches the specifies patch on the BaseboardManagement.
+func (r *BaseboardManagementReconciler) patchStatus(ctx context.Context, bm *bmcv1alpha1.BaseboardManagement, patch client.Patch) (ctrl.Result, error) {
+	err := r.client.Status().Patch(ctx, bm, patch)


@chrisdoherty4 @jacobweinstock there is no idempotency/locking in the whole reconcile, do we want/need that?

Nope. When you're only considering the object under reconciliation there's no races as the manager ensures you're the only reconciler reconciling that resource.

There is, of course, the possibility that an object is patched by something else but I don't think we have those semantics here.

main.go

Signed-off-by: Aravind Ramalingam <[email protected]>

micahhausler

LGTM

pokearu added 5 commits May 5, 2022 14:17

Adding client package for BMC client interactions

5d261a8

Signed-off-by: Aravind Ramalingam <[email protected]>

Injecting client into baseboardmanagement controller

f6c12ec

Signed-off-by: Aravind Ramalingam <[email protected]>

Updating Dockerfile to copy all code

3d691bb

Signed-off-by: Aravind Ramalingam <[email protected]>

Updating baseboardmanagement type validations

e4f533a

Signed-off-by: Aravind Ramalingam <[email protected]>

Adding reconciler logic for baseboardmanagement controller

dfe231e

Signed-off-by: Aravind Ramalingam <[email protected]>

pokearu force-pushed the baseboardmanagement-controller branch from d6f2eb1 to dfe231e Compare May 5, 2022 21:17

micahhausler suggested changes May 5, 2022

View reviewed changes

chrisdoherty4 reviewed May 5, 2022

View reviewed changes

pkg/bmc/client/bmclib/bmclib.go Outdated Show resolved Hide resolved

chrisdoherty4 reviewed May 5, 2022

View reviewed changes

controllers/baseboardmanagement_controller.go Outdated Show resolved Hide resolved

pokearu added 5 commits May 12, 2022 15:47

Adding new secrets viewer role for controller

876d116

Signed-off-by: Aravind Ramalingam <[email protected]>

Initializing logger as part BaseboardManagementReconciler and log cha…

e7c77bb

…nges Signed-off-by: Aravind Ramalingam <[email protected]>

Abstracting field reconcilers out of reconcile

f93e1ef

Signed-off-by: Aravind Ramalingam <[email protected]>

Refactoring bmc client package

2b70431

Signed-off-by: Aravind Ramalingam <[email protected]>

Adding flags to read namesapce, kubeconfig and apiserver. Injecting b…

4038d9e

…mcclient factory method Signed-off-by: Aravind Ramalingam <[email protected]>

pokearu force-pushed the baseboardmanagement-controller branch from 1fdfb67 to 4038d9e Compare May 12, 2022 22:47

chrisdoherty4 requested changes May 13, 2022

View reviewed changes

Refactoring BMCClient implementation and log cleanup

a34127a

Signed-off-by: Aravind Ramalingam <[email protected]>

chrisdoherty4 reviewed May 13, 2022

View reviewed changes

chrisdoherty4 approved these changes May 13, 2022

View reviewed changes

micahhausler suggested changes May 13, 2022

View reviewed changes

pokearu added 5 commits May 16, 2022 12:32

Renaming flag, zap opts and removing schema from Reconciler

3e3eed1

Signed-off-by: Aravind Ramalingam <[email protected]>

Modifying reconciler to update status during field reconciliation

43f7ed6

Signed-off-by: Aravind Ramalingam <[email protected]>

Adding gomock for generating interface mocks

cc8b689

Signed-off-by: Aravind Ramalingam <[email protected]>

Adding unit tests for baseboardmanagement controller Reconcile

eb72725

Signed-off-by: Aravind Ramalingam <[email protected]>

Fixing linter errors for error return

65acac8

Signed-off-by: Aravind Ramalingam <[email protected]>

pokearu force-pushed the baseboardmanagement-controller branch from 0c60ca4 to 65acac8 Compare May 17, 2022 17:44

micahhausler approved these changes May 17, 2022

View reviewed changes

micahhausler added the ready-to-merge Mergify: Ready for Merging label May 17, 2022

mergify bot merged commit 0732a9c into tinkerbell:main May 17, 2022

pokearu mentioned this pull request May 18, 2022

BaseboardManagement events and conditions #5

Merged

3 tasks

pokearu mentioned this pull request Jun 27, 2022

Added to CODEOWNERS #34

Merged

pokearu mentioned this pull request Aug 31, 2022

Fix default permissions to be able to read secrets cluster wide #62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Baseboardmanagement controller implementation #3

Baseboardmanagement controller implementation #3

pokearu commented May 5, 2022 •

edited

Loading

codecov-commenter commented May 5, 2022 •

edited

Loading

micahhausler May 5, 2022

pokearu May 9, 2022

chrisdoherty4 left a comment

chrisdoherty4 May 12, 2022

chrisdoherty4 left a comment

micahhausler left a comment

micahhausler May 13, 2022

pokearu May 16, 2022

micahhausler May 13, 2022

pokearu May 16, 2022

micahhausler May 13, 2022

chrisdoherty4 May 13, 2022

chrisdoherty4 May 13, 2022

micahhausler left a comment

Baseboardmanagement controller implementation #3

Baseboardmanagement controller implementation #3

Conversation

pokearu commented May 5, 2022 • edited Loading

Description

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

codecov-commenter commented May 5, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdoherty4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdoherty4 left a comment

Choose a reason for hiding this comment

micahhausler left a 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

micahhausler left a comment

Choose a reason for hiding this comment

pokearu commented May 5, 2022 •

edited

Loading

codecov-commenter commented May 5, 2022 •

edited

Loading