Spring cleaning #258

mmlb · 2022-04-22T20:51:21Z

Description

I was getting ready to PR my branch that is updated to work in /cert-less tink (now that tinkerbell/tink#584 is merged) and it was basically a situation where pulling one thread unraveled a bunch of other things. Dropping /cert begot some refactors (started by dropping the buildWorkerParams silliness) which begot some test fixes which begot some other refactors which was also dumb to do for rancher, nixos, and centos. So here we are.

Why is this needed

Makes updating to post tinkerbell/tink#584 easier/nicer.

How Has This Been Tested?

Ran unit tests.

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

Less EM specific junk is always good. Less duplicate/unclear tests too. This PR does introduce a change wrt sandbox, where MIRROR_HOST is dropped in favor of MIRROR_BASE_URL, I'll follow up with a PR there once this merges.

Checklist:

I have:

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

codecov · 2022-04-22T21:31:17Z

Codecov Report

Merging #258 (007fbc0) into main (0199cbc) will decrease coverage by 1%.
The diff coverage is 56%.

❗ Current head 007fbc0 differs from pull request most recent head 05cfeab. Consider uploading reports for the commit 05cfeab to get more accurate results

@@         Coverage Diff          @@
##           main   #258    +/-   ##
====================================
- Coverage    40%    39%    -2%     
====================================
  Files        41     37     -4     
  Lines      2580   2424   -156     
====================================
- Hits       1037    948    -89     
+ Misses     1468   1413    -55     
+ Partials     75     63    -12

Impacted Files	Coverage Δ
cmd/boots/http.go	`20% <0%> (+<1%)`	⬆️
cmd/boots/main.go	`16% <0%> (-1%)`	⬇️
installers/flatcar/ignition.go	`0% <0%> (ø)`
installers/flatcar/networkd.go	`12% <ø> (ø)`
job/ipxe.go	`0% <0%> (ø)`
job/mock.go	`60% <0%> (-1%)`	⬇️
installers/osie/main.go	`64% <59%> (+<1%)`	⬆️
installers/vmware/main.go	`74% <63%> (+11%)`	⬆️
conf/mirror.go	`85% <85%> (+22%)`	⬆️
installers/custom_ipxe/main.go	`100% <100%> (ø)`
... and 3 more

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 0199cbc...05cfeab. Read the comment docs.

micahhausler

LGTM, but I only reviewed for language formatting and idioms. I'd want another set of eyes for PXE content

micahhausler · 2022-04-25T20:28:46Z

@mmlb do you think we can get #255 in first?

jacobweinstock

This is a terribly large PR, quite a lot of movement. Would have really preferred if it was broken into smaller chunks.

jacobweinstock · 2022-04-26T17:23:54Z

conf/mirror.go

+func mustBuildMirrorBaseURL() string {
+	s, err := buildMirrorBaseURL()
+	if s == "" {
+		panic(fmt.Sprintf(`conf: MirrorBaseURL: %q: %s\n`, s, err))


The panic here violates Boots design philosophy.

"Only func main has the right to decide which flags, env variables, config files are available to the user" -- ref
"Func main should normally be the only one calling fatal errors or os.Exit" -- ref

this was already here from before, I've actually removed one other case of unwanted behavior. I'll rework the code though so that we don't need the conf package anymore.

I tried re-working but then came across all the other things in https://github.com/tinkerbell/boots/blob/main/conf/config.go and stopped trying. I've made it so osie is better about this as a small step in the right direction. Getting rid of all the must* in conf is going to explode this PR even further.

jacobweinstock · 2022-04-26T17:42:40Z

go.mod

 	google.golang.org/grpc v1.44.0
 	inet.af/netaddr v0.0.0-20211027220019-c74959edd3b6
 )

+require github.com/golangci/golangci-lint v1.45.2


what does this provide? Feels out of place. does this package get included in the binary build? how much size does it add to the binary? The go.mod in this package has a lot of dependencies.

The Makefile has a target that runs golangci-lint and says @echo be sure golangci-lint is installed: https://golangci-lint.run/usage/install/ which really makes no sense when we are pinning go tools using tools.go.

the other imports in tools.go don't show up in the go.mod. i wondering why golangci-lint needs to?

jacobweinstock · 2022-04-26T17:54:04Z

installers/osie/main.go

+type installer struct{}
+
+func Installer() job.BootScripter {
+	if env.Get("DATA_MODEL_VERSION") != "1" {


Why is this being introduced here?

I've lifted this up to here from where it used to be, this env var was being checked for every ipxe script that was generated (https://github.com/tinkerbell/boots/blob/main/installers/osie/main.go#L115-L116) instead of just once at boot since they aren't going to change. I'll move this to be done in cmd/boots/main.go and pass it in as args.

jacobweinstock · 2022-04-26T17:57:06Z

installers/osie/main.go

+		"grpc_cert_url=" + require("TINKERBELL_GRPC_AUTHORITY"),
+		"packet_base_url=" + workflowURL,
+	}
+	if registry := env.Get("DOCKER_REGISTRY"); registry != "" {


Environment variables should endeavor to be read in func main.

"Only func main has the right to decide which flags, env variables, config files are available to the user" -- ref

similar to the other comment and I'll rework to avoid this now

mmlb · 2022-05-02T18:54:04Z

Hey @jacobweinstock yeah I recognize this is a big PR. I kind of went that way to avoid a long series of small quick PRs when effectively they are all refactors/tidying up of things. I can definitely split up into smaller PRs but honestly I think it'd be easier to go through this commit by commit, each should be pretty self-explanatory/stand-alone and digestable. Let me know if that works or not. #255 is close so to ready and I'll be rebasing on top of it.

micahhausler

Reviewed for go style and backend support, I will defer to other reviewers for most of the content

micahhausler · 2022-05-02T22:39:07Z

installers/osie/main.go

+		"grpc_authority=" + require("TINKERBELL_CERT_URL"),
+		"grpc_cert_url=" + require("TINKERBELL_GRPC_AUTHORITY"),


Mix up here:

Suggested change

"grpc_authority=" + require("TINKERBELL_CERT_URL"),

"grpc_cert_url=" + require("TINKERBELL_GRPC_AUTHORITY"),

"grpc_authority=" + require("TINKERBELL_GRPC_AUTHORITY"),

"grpc_cert_url=" + require("TINKERBELL_CERT_URL"),

Also:

Isn't TINKERBELL_CERT_URL deprecated/gone and replaced with TINKERBELL_TLS=bool?

You'll need a way to plumb down the tink API for the Kubernetes backend data model

oops I've still forgotten about k8s backend, have to update that too.

I kept CERT_URL and left TLS out originally because it didn't seem like "spring cleaning", but after you mentioned it I think its fine, it should have been done when those respective PRs landed in tink so I can see this being "dirty" in need of cleaning ... if you squint :D. So I've fixed this now.

@micahhausler the kubernetes backend pr (#250) isn't merged yet.

micahhausler · 2022-05-02T22:43:59Z

rules.mk

+packet/mock_cacher/cacher_mock.go: packet/models_cacher.go
+packet/mock_workflow/workflow_mock.go: packet/models_tinkerbell.go


These paths need updating

Suggested change

packet/mock_cacher/cacher_mock.go: packet/models_cacher.go

packet/mock_workflow/workflow_mock.go: packet/models_tinkerbell.go

client/cacher/mock_cacher/cacher_mock.go: client/cacher/discovery.go

client/tinkerbell/mock_workflow/workflow_mock.go: client/tinkerbell/discovery.go

client/tinkerbell/mock_hardware/hardware_mock.go: client/tinkerbell/discovery.go

fixed, good catch.

I changed the range because I'm tired of seeing red. Lets get to green more positively. Also no longer need the vendor or ipxe ignores because they no longer exist. Removed fields that the codecov validator complained about as unknown. Signed-off-by: Manuel Mendez <[email protected]>

No need to test/verify combinations that do not influence the outcome. Signed-off-by: Manuel Mendez <[email protected]>

EM no longer supports coreos provisions and coreos is long EOL and gone. This is the first of a series of commits that will drop mentions of coreos in favor of flatcar. Signed-off-by: Manuel Mendez <[email protected]>

Flatcar does not fetch oem.tgz, it doesn't even have code that mentions oem_url. I came to realize this when I noticed in the last commit that we set oem_url=.../flatcar/oem.tgz but only ever serve .../coreos/oem.tgz in cmd/boots/http.go which should have caused flatcar to fail if it needed it. I've verified that flatcar doesn't even try to fetch the oem tarball. Drop files/tarball because it was only used in oem.go file. Signed-off-by: Manuel Mendez <[email protected]>

Its all flatcar now, woop! Signed-off-by: Manuel Mendez <[email protected]>

No need to test 2 plans that just run `Exec`. Also update the names to the current plan name schemes. Change s1.large special handling to s3.xlarge as flatcar only needs work arounds for that the s3.xlarge. Signed-off-by: Manuel Mendez <[email protected]>

These packages are only used by coreso (aka flatcar) so move them there. Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Manuel Mendez <[email protected]>

At the top of the script too. Signed-off-by: Manuel Mendez <[email protected]>

This way the tests better reflect what is expected to happen in real world. Signed-off-by: Manuel Mendez <[email protected]>

There's only one actual case where returning the Script was nice (the `Set("iface",...).Or("shell")`) but its not worth keeping around. Its confusing seeing the script being modified in place within a function body but having to copy and return within and after passing to a function. Now we know the script is always modified in place. Signed-off-by: Manuel Mendez <[email protected]>

This interface will be how main gets the func to use from the installers. Signed-off-by: Manuel Mendez <[email protected]>

This way cmd/boots doesn't need to violate any layers/boundaries by setting up structs and calling internal functions. This way we avoid tight coupling of installers to main's needs. Signed-off-by: Manuel Mendez <[email protected]>

Previously the tinkerbell connection params were being checked for each request unnecessarily. We'd also fail late, not early, if anything was invalid. This now fails early at start up and doesn't waste time/cycles getting the vaules and then throwing them away. Now that the env is being read only once there's not a whole lot of reason for having mirror.go around for just one function that is called only twice. Signed-off-by: Manuel Mendez <[email protected]>

… args These values are static once boots starts up so we can precalculate a partial slice of args and avoid a bunch of string allocation/concatenation. I don't think this is premature optimization as I prefer doing this for readability aspect over the optimization. Signed-off-by: Manuel Mendez <[email protected]>

But only if false because we want to preserve the default of true as default. Signed-off-by: Manuel Mendez <[email protected]>

This is no longer a thing so no need to propogate. Signed-off-by: Manuel Mendez <[email protected]>

jacobweinstock · 2022-05-03T23:15:27Z

installers/osie/main.go

-	ExtraKernelArgs string
+type installer struct {
+	osieURL string
+


nit: remove extra space

jacobweinstock · 2022-05-03T23:28:38Z

installers/osie/main.go

-		}
-		s.Args("grpc_authority=" + grpcAuthority)
-		s.Args("grpc_cert_url=" + grpcCertURL)
+		s.Args(i.tinkParams)


in my tests this appears to add a single space if i.tinkParams == "".

tinkParams will never be empty since its initialized with grpc_authority and packet_base_url.

if dataModelVersion != "1" then i.tinkParams == ""

hmm good point, this is still guarded by CanWorkflow which doesn't make any sense when dataModelVersion != 1. 🤔 ideally CanWorkflow returns false if dataModelVersion != 1 always, let me try that out.

I'm going to make sure we're in tink mode as a prereq for the CanWorkflow checks. Will also open up an issue so that we drop CanWorkflow all together as boots doesn't really support a mixed-mode operation we're either in cacher, tink, standalone and the hw has no real say in it.

I believe this has been fixed now, can you make sure please?

jacobweinstock · 2022-05-03T23:30:43Z

cmd/boots/main.go

+	dataModelVersion := env.Get("DATA_MODEL_VERSION")
+	auth := env.Get("TINKERBELL_GRPC_AUTHORITY")
+	if dataModelVersion == "1" && auth == "" {
+		mainlog.Fatal(errors.New("TINKERBELL_GRPC_AUTHORITY is required when in tinkerbell mode"))


Can this panic be moved to func main?

go.mod

jacobweinstock · 2022-05-04T14:24:09Z

installers/osie/main.go

+	if registry != "" {
+		tinkParams = append(tinkParams, "docker_registry="+registry)
+	}
+	if registryUsername != "" {
+		tinkParams = append(tinkParams, "registry_username="+registryUsername)
+	}
+	if registryPassword != "" {
+		tinkParams = append(tinkParams, "registry_password="+registryPassword)
+	}


I believe these would still be needed even if dataModelVersion != "1"

thats not how the code is today: https://github.com/tinkerbell/boots/blob/main/installers/osie/main.go#L123-L139. This is behind CanWorkflow but that only ever happens when dataModelVersion == 1 anyway. We shouldn't have 2 ways to do this check.

thats not my experience. i can use the DATA_MODEL_VERSION=standalone and still use CanWorkflow.

hmm this seems very accidental and confusing, if you're in dmv=standalone and canworkflow=true then why would tink connection params matter? dmv is actually doing 2 different jobs and causing confusion (surprise, surprise... 😄). Should probably be something like DATA_SOURCE=cacher|tink|standalone|something-in-the-future/pluggable and DATA_MODEL=cacher|tink. But oh well, not going to try and do that now. I'll fix it so tinkArgs isn't dependent on DMV=1.

Lets make `Environment variables should endeavor to be read in func main` that much closer to being reality. This does that by getting rid of all the direct lookups into the environment, instead getting the values via the call to `Installer()`. This commit also makes use of the arguments by building up the space separated string that gets passed to script.Args only once instead of storing each value and building up the same substring over and over and over again. Signed-off-by: Manuel Mendez <[email protected]>

jacobweinstock

Thanks @mmlb !

mmlb force-pushed the refactor-oh-so-many-things branch 3 times, most recently from b85712a to 53bef91 Compare April 22, 2022 21:29

mmlb force-pushed the refactor-oh-so-many-things branch 11 times, most recently from de9d836 to 8b2f83b Compare April 25, 2022 19:05

mmlb requested review from jacobweinstock and micahhausler April 25, 2022 20:07

mmlb marked this pull request as ready for review April 25, 2022 20:07

micahhausler previously approved these changes Apr 25, 2022

View reviewed changes

jacobweinstock requested changes Apr 26, 2022

View reviewed changes

mmlb mentioned this pull request Apr 27, 2022

Add kernel commandline parameter for specifying the tink-worker image: #256

Closed

3 tasks

mmlb dismissed micahhausler’s stale review via 1fecb7b May 2, 2022 21:14

mmlb force-pushed the refactor-oh-so-many-things branch 4 times, most recently from f671dce to 16c6bac Compare May 2, 2022 21:33

micahhausler suggested changes May 2, 2022

View reviewed changes

mmlb mentioned this pull request May 3, 2022

CLI flag to allow overriding of OSIE path #263

Merged

3 tasks

mmlb force-pushed the refactor-oh-so-many-things branch from 488e22c to 3b8ab08 Compare May 3, 2022 21:27

mmlb and others added 17 commits May 3, 2022 17:48

tests: Minimize ipxe script test combinations

6c7b9e5

No need to test/verify combinations that do not influence the outcome. Signed-off-by: Manuel Mendez <[email protected]>

coreos: Drop coreos from dynamic checks

4f08a76

EM no longer supports coreos provisions and coreos is long EOL and gone. This is the first of a series of commits that will drop mentions of coreos in favor of flatcar. Signed-off-by: Manuel Mendez <[email protected]>

coreos: Get rid of coreos distro from tests

a907a20

Its all flatcar now, woop! Signed-off-by: Manuel Mendez <[email protected]>

coreos: Move files/ into installers/coreos

2ab9392

These packages are only used by coreso (aka flatcar) so move them there. Signed-off-by: Manuel Mendez <[email protected]>

coreos: rename to flatcar

f9a2b2c

Signed-off-by: Manuel Mendez <[email protected]>

vmware: Test ipxe scripts for all supported vmware versions

f4aaf1f

Signed-off-by: Manuel Mendez <[email protected]>

ipxe: Always do the echo Tinkerbell Boots iPXE

61ed82a

At the top of the script too. Signed-off-by: Manuel Mendez <[email protected]>

tests: Make the ipxe scripts in the ipxe_script_test files more accurate

443c387

This way the tests better reflect what is expected to happen in real world. Signed-off-by: Manuel Mendez <[email protected]>

job: Add BootScripter interface

087ceb6

This interface will be how main gets the func to use from the installers. Signed-off-by: Manuel Mendez <[email protected]>

osie: Pass TINKERBELL_TLS env var down via kernel command line

383ae2c

But only if false because we want to preserve the default of true as default. Signed-off-by: Manuel Mendez <[email protected]>

osie: Drop support for the /cert url

f5e5729

This is no longer a thing so no need to propogate. Signed-off-by: Manuel Mendez <[email protected]>

mmlb force-pushed the refactor-oh-so-many-things branch from 3b8ab08 to 3b28614 Compare May 3, 2022 21:48

mmlb mentioned this pull request May 3, 2022

Add kubernetes-based backend #250

Merged

3 tasks

jacobweinstock reviewed May 3, 2022

View reviewed changes

jacobweinstock reviewed May 4, 2022

View reviewed changes

mmlb force-pushed the refactor-oh-so-many-things branch from 3b28614 to 05cfeab Compare May 4, 2022 14:53

jacobweinstock approved these changes May 4, 2022

View reviewed changes

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

mergify bot merged commit 56f67aa into tinkerbell:main May 4, 2022

mmlb deleted the refactor-oh-so-many-things branch May 4, 2022 15:05

mmlb mentioned this pull request May 4, 2022

pass tink server tls mode as kernel cmdline arg #257

Closed

3 tasks

		"grpc_authority=" + require("TINKERBELL_CERT_URL"),
		"grpc_cert_url=" + require("TINKERBELL_GRPC_AUTHORITY"),

		packet/mock_cacher/cacher_mock.go: packet/models_cacher.go
		packet/mock_workflow/workflow_mock.go: packet/models_tinkerbell.go

-packet/mock_cacher/cacher_mock.go: packet/models_cacher.go
-packet/mock_workflow/workflow_mock.go: packet/models_tinkerbell.go
+client/cacher/mock_cacher/cacher_mock.go: client/cacher/discovery.go
+client/tinkerbell/mock_workflow/workflow_mock.go: client/tinkerbell/discovery.go
+client/tinkerbell/mock_hardware/hardware_mock.go: client/tinkerbell/discovery.go

Spring cleaning #258

Spring cleaning #258

Conversation

mmlb commented Apr 22, 2022 • edited Loading

Description

Why is this needed

How Has This Been Tested?

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

Checklist:

codecov bot commented Apr 22, 2022 • edited Loading

Codecov Report

micahhausler left a comment

Choose a reason for hiding this comment

micahhausler commented Apr 25, 2022

jacobweinstock 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

jacobweinstock May 3, 2022 • 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

mmlb commented May 2, 2022

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

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

jacobweinstock May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmlb May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobweinstock left a comment

Choose a reason for hiding this comment

mmlb commented Apr 22, 2022 •

edited

Loading

codecov bot commented Apr 22, 2022 •

edited

Loading

jacobweinstock May 3, 2022 •

edited

Loading

jacobweinstock May 4, 2022 •

edited

Loading

mmlb May 4, 2022 •

edited

Loading