Add Kubernetes back-end client #82

chrisdoherty4 · 2022-04-15T15:29:42Z

Description

Builds on #95 (rebased after PR was created)

This feature builds on #81 and realizes the Hegel portion of the Kubernetes back-end proposal.

Having generalized the client used to acquire metadata, this PR implements a Kubernetes client that leverages an internal client caching mechanism so as to reduce load on the Kubernetes API Server.

Additionally, I put some effort into reworking the Hegel Dockerfile so it could leverage layer caching for Go module dependencies and added an image build command to the Makefile.

The Watch API for the client is unimplemented and scheduled for a follow up. This is so we can get off the ground for machine provisioning. The Watch API is used for subscription capabilities so doesn't feature in metadata retrieval.

How Has This Been Tested?

Built and ran the image with the Kubernetes back-end enabled talking to a KinD cluster containing the necessary CRDs and a Hardware resource.

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

None. Users enable the Kubernetes back-end by setting the DATA_MODEL_VERSION=kubernetes plus some additional env vars.

Future work

Implement the Watch API for consistency with other back-ends.

hardware/kubernetes.go

micahhausler

A few questions, and some cleanup.

hardware/client.go

hardware/kubernetes.go

hardware/kubernetes_test.go

.gitattributes

hardware/kubernetes_client.go

codecov · 2022-04-18T13:53:03Z

Codecov Report

Merging #82 (9fa3d0c) into main (50983c3) will decrease coverage by 7.64%.
The diff coverage is 42.44%.

❗ Current head 9fa3d0c differs from pull request most recent head f6fccce. Consider uploading reports for the commit f6fccce to get more accurate results

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   50.00%   42.35%   -7.65%     
==========================================
  Files           3        6       +3     
  Lines         374      595     +221     
==========================================
+ Hits          187      252      +65     
- Misses        155      311     +156     
  Partials       32       32

Impacted Files	Coverage Δ
hardware/client.go	`0.00% <0.00%> (ø)`
grpc/server.go	`56.96% <20.00%> (+10.75%)`	⬆️
http/handlers.go	`44.56% <36.36%> (-1.00%)`	⬇️
hardware/kubernetes_client.go	`46.15% <46.15%> (ø)`
http/server.go	`88.88% <88.88%> (ø)`
hardware/export.go	`0.00% <0.00%> (ø)`

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 50983c3...f6fccce. Read the comment docs.

displague

I took an initial look to get a sense of what's in flux and left a few questions.

.golangci.yml

go.mod

hardware/kubernetes_client.go

hardware/kubernetes_client_test.go

hardware/kubernetes_client.go

micahhausler

LGTM modulo minor nits, and pending merge of tinkerbell/tink#573

Makefile

hardware/kubernetes_client.go

go.mod

The Kubernetes back-end client enables Hegel to talk to a Kubernetes API Server to retrieve Hardware metadata from CRDs available in the Tink project. This realizes the Hegel portion of proposal 0026. Signed-off-by: Chris Doherty <[email protected]>

hardware/kubernetes_client.go

jacobweinstock · 2022-05-06T23:34:46Z

hardware/kubernetes_client.go

+	}
+
+	managerCtx, cancel := context.WithCancel(context.Background())
+	go func() {


hmmm...removing this goroutine from this function might be something to consider. The name suggests that it is a constructor and having a constructor start a goroutine seems a bit unusual, could be just me though.

refs: 1, 2

Generally I agree. The way the app is designed though makes this difficult to do. The NewClient() call is a factory function returning an interface so anything that needs to be done for a given implementation must be done during that call. Without re-designing this factory I don't think there's much I can do.

hardware/kubernetes_client.go

Http handlers require a specific metadata format for querying using jq. The metadata format of the Kubernetes object isn't equivilent so we create an explicit mapping. This has the side effect of being much clearer and intentional about the serialization of metadata. Signed-off-by: Chris Doherty <[email protected]>

jacobweinstock

lgtm

Builds on #82 The hardware clients has 3 implementations with a fourth, file based for testing, being written. This change introduces a simple source file split to reduce cognitive load.

chrisdoherty4 changed the title ~~Add Kubernetes back-end client.~~ Add Kubernetes back-end client Apr 15, 2022

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch 3 times, most recently from 2377d79 to 93b3de6 Compare April 15, 2022 15:49

chrisdoherty4 commented Apr 15, 2022

View reviewed changes

hardware/kubernetes.go Outdated Show resolved Hide resolved

micahhausler suggested changes Apr 15, 2022

View reviewed changes

hardware/client.go Outdated Show resolved Hide resolved

hardware/kubernetes.go Outdated Show resolved Hide resolved

hardware/kubernetes.go Show resolved Hide resolved

hardware/kubernetes.go Outdated Show resolved Hide resolved

hardware/kubernetes_test.go Outdated Show resolved Hide resolved

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch 5 times, most recently from 2547eda to 4bc9339 Compare April 17, 2022 02:19

chrisdoherty4 commented Apr 17, 2022

View reviewed changes

.gitattributes Show resolved Hide resolved

chrisdoherty4 commented Apr 17, 2022

View reviewed changes

hardware/kubernetes_client.go Outdated Show resolved Hide resolved

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from 3c042da to 3e0cd30 Compare April 18, 2022 13:42

chrisdoherty4 requested a review from micahhausler April 18, 2022 14:34

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from 308bcf3 to 360938c Compare April 18, 2022 14:36

chrisdoherty4 marked this pull request as ready for review April 18, 2022 14:38

displague reviewed Apr 18, 2022

View reviewed changes

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from bc16c9c to bb999a9 Compare April 18, 2022 19:21

chrisdoherty4 requested a review from displague April 18, 2022 19:22

mmlb added the do-not-merge Signal to Mergify to block merging of the PR. label Apr 18, 2022

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch 2 times, most recently from 291dfe6 to e2f8bad Compare April 19, 2022 16:10

chrisdoherty4 mentioned this pull request Apr 20, 2022

Implement Cobra and Viper for configuring Hegel #84

Merged

displague mentioned this pull request Apr 20, 2022

Disable errorf option for errorlint. tinkerbell/lint-install#41

Merged

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from e2f8bad to c8a1202 Compare April 21, 2022 11:43

chrisdoherty4 mentioned this pull request Apr 21, 2022

Add Makefile and Dockerfile improvements #85

Closed

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from 4069e86 to 0e7bcd7 Compare April 21, 2022 12:05

micahhausler previously approved these changes Apr 21, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

hardware/kubernetes_client.go Outdated Show resolved Hide resolved

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch 2 times, most recently from 236bb9c to d5ea9ab Compare April 27, 2022 18:49

chrisdoherty4 mentioned this pull request Apr 27, 2022

[Organization/member]: request for chrisdoherty4 tinkerbell/org#16

Closed

7 tasks

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from d5ea9ab to 326c59a Compare April 29, 2022 11:44

micahhausler reviewed Apr 29, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch 6 times, most recently from 1c5944d to 7679986 Compare May 6, 2022 19:30

chrisdoherty4 removed the do-not-merge Signal to Mergify to block merging of the PR. label May 6, 2022

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from 7679986 to 8ee9572 Compare May 6, 2022 19:32

Add Kubernetes back-end client.

c30326a

The Kubernetes back-end client enables Hegel to talk to a Kubernetes API Server to retrieve Hardware metadata from CRDs available in the Tink project. This realizes the Hegel portion of proposal 0026. Signed-off-by: Chris Doherty <[email protected]>

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch 2 times, most recently from 5407406 to b6787b8 Compare May 6, 2022 19:45

chrisdoherty4 requested review from micahhausler and jacobweinstock May 6, 2022 19:45

jacobweinstock reviewed May 6, 2022

View reviewed changes

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from b6787b8 to 26c8747 Compare May 7, 2022 15:05

chrisdoherty4 requested a review from jacobweinstock May 7, 2022 22:38

chrisdoherty4 force-pushed the cpd/impl-kubernetes-client branch from 26c8747 to f6fccce Compare May 8, 2022 17:42

chrisdoherty4 mentioned this pull request May 8, 2022

Split hardware clients into logical source files #100

Merged

jacobweinstock approved these changes May 9, 2022

View reviewed changes

jacobweinstock added the ready-to-merge Signal to Mergify to merge the PR. label May 9, 2022

jacobweinstock removed request for displague and micahhausler May 9, 2022 15:37

mergify bot merged commit 7285eb8 into tinkerbell:main May 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Kubernetes back-end client #82

Add Kubernetes back-end client #82

chrisdoherty4 commented Apr 15, 2022 •

edited

Loading

micahhausler left a comment

codecov bot commented Apr 18, 2022 •

edited

Loading

displague left a comment

micahhausler left a comment

jacobweinstock May 6, 2022

chrisdoherty4 May 7, 2022

jacobweinstock left a comment

Add Kubernetes back-end client #82

Add Kubernetes back-end client #82

Conversation

chrisdoherty4 commented Apr 15, 2022 • edited Loading

Description

How Has This Been Tested?

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

Future work

micahhausler left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 18, 2022 • edited Loading

Codecov Report

displague left a comment

Choose a reason for hiding this comment

micahhausler left a comment

Choose a reason for hiding this comment

jacobweinstock May 6, 2022

Choose a reason for hiding this comment

chrisdoherty4 May 7, 2022

Choose a reason for hiding this comment

jacobweinstock left a comment

Choose a reason for hiding this comment

chrisdoherty4 commented Apr 15, 2022 •

edited

Loading

codecov bot commented Apr 18, 2022 •

edited

Loading