job: dhcp: Get rid of isArm and isUEFI bools in favor descriptive str…

…ings This way is more extensible since we don't have to add more and more bool args for newer cpus or firmware interfaces. Signed-off-by: Manuel Mendez <mmendez@equinix.com>
tinkerbell · Apr 23, 2021 · 38c8d46 · 38c8d46
1 parent b834928
commit 38c8d46
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 24 deletions.
diff --git a/job/dhcp.go b/job/dhcp.go
@@ -59,22 +59,34 @@ func (j Job) configureDHCP(rep, req *dhcp4.Packet) bool {
 		return false
 	}
 	if dhcp.SetupPXE(rep, req) {
+		arch := "x86"
+		firmware := "bios"
+
 		isARM := dhcp.IsARM(req)
 		if dhcp.Arch(req) != j.Arch() {
 			j.With("dhcp", dhcp.Arch(req), "job", j.Arch()).Info("arch mismatch, using dhcp")
 		}
+		if isARM {
+			arch = "arm"
+			if parch := j.PArch(); parch == "2a2" || parch == "hua" {
+				arch = "hua"
+			}
+		}
 
 		isUEFI := dhcp.IsUEFI(req)
 		if isUEFI != j.IsUEFI() {
 			j.With("dhcp", isUEFI, "job", j.IsUEFI()).Info("uefi mismatch, using dhcp")
 		}
+		if isUEFI {
+			firmware = "uefi"
+		}
 
 		isOuriPXE := ipxe.IsOuriPXE(req)
 		if isOuriPXE {
 			ipxe.Setup(rep)
 		}
 
-		if filename := j.getPXEFilename(isOuriPXE, isARM, isUEFI); filename != "" {
+		if filename := j.getPXEFilename(arch, firmware, isOuriPXE); filename != "" {
 			dhcp.SetFilename(rep, filename, conf.PublicIPv4)
 		}
 	}
@@ -99,7 +111,7 @@ func (j Job) areWeProvisioner() bool {
 	return j.hardware.HardwareProvisioner() == j.ProvisionerEngineName()
 }
 
-func (j Job) getPXEFilename(isOuriPXE, isARM, isUEFI bool) string {
+func (j Job) getPXEFilename(arch, firmware string, isOuriPXE bool) string {
 	if !j.isPXEAllowed() {
 		if j.instance != nil && j.instance.State == "active" {
 			// We set a filename because if a machine is actually trying to PXE and nothing is sent it may hang for
@@ -112,13 +124,14 @@ func (j Job) getPXEFilename(isOuriPXE, isARM, isUEFI bool) string {
 
 	var filename string
 	if !isOuriPXE {
-		if j.PArch() == "hua" || j.PArch() == "2a2" {
+		switch {
+		case arch == "hua":
 			filename = "snp-hua.efi"
-		} else if isARM {
+		case arch == "arm" && firmware == "uefi":
 			filename = "snp-nolacp.efi"
-		} else if isUEFI {
+		case arch == "x86" && firmware == "uefi":
 			filename = "ipxe.efi"
-		} else {
+		case arch == "x86" && firmware == "bios":
 			filename = "undionly.kpxe"
 		}
 	} else {

diff --git a/job/dhcp_test.go b/job/dhcp_test.go
@@ -16,11 +16,10 @@ func TestGetPXEFilename(t *testing.T) {
 	var getPXEFilenameTests = []struct {
 		name     string
 		iState   string
-		plan     string
 		allowPXE bool
 		ouriPXE  bool
-		arm      bool
-		uefi     bool
+		arch     string
+		firmware string
 		filename string
 	}{
 		{name: "inactive instance",
@@ -29,52 +28,51 @@ func TestGetPXEFilename(t *testing.T) {
 			iState:   "active",
 			filename: "/pxe-is-not-allowed"},
 		{name: "PXE is allowed for non active instance",
-			allowPXE: true,
+			iState: "not_active", allowPXE: true, arch: "x86", firmware: "bios",
 			filename: "undionly.kpxe"},
 		{name: "our embedded iPXE wants iPXE script",
 			ouriPXE: true, allowPXE: true,
 			filename: "http://" + conf.PublicFQDN + "/auto.ipxe"},
 		{name: "2a2",
-			plan: "2a2", allowPXE: true,
+			arch: "hua", allowPXE: true,
 			filename: "snp-hua.efi"},
 		{name: "arm",
-			arm: true, allowPXE: true,
+			arch: "arm", firmware: "uefi", allowPXE: true,
 			filename: "snp-nolacp.efi"},
 		{name: "hua",
-			plan: "hua", allowPXE: true,
+			arch: "hua", allowPXE: true,
 			filename: "snp-hua.efi"},
 		{name: "x86 bios",
-			allowPXE: true,
+			arch: "x86", firmware: "bios", allowPXE: true,
 			filename: "undionly.kpxe"},
 		{name: "x86 uefi",
-			uefi: true, allowPXE: true,
+			arch: "x86", firmware: "uefi", allowPXE: true,
 			filename: "ipxe.efi"},
+		{name: "unknown arch",
+			arch: "riscv", allowPXE: true},
+		{name: "unknown firmware",
+			arch: "coreboot", allowPXE: true},
 	}
 
 	for i, tt := range getPXEFilenameTests {
 		t.Run(tt.name, func(t *testing.T) {
-			t.Logf("index=%d iState=%q plan=%q allowPXE=%v ouriPXE=%v arm=%v uefi=%v filename=%q",
-				i, tt.iState, tt.plan, tt.allowPXE, tt.ouriPXE, tt.arm, tt.uefi, tt.filename)
-
-			if tt.plan == "" {
-				tt.plan = "0"
-			}
+			t.Logf("index=%d iState=%q allowPXE=%v ouriPXE=%v arch=%v platfrom=%v filename=%q",
+				i, tt.iState, tt.allowPXE, tt.ouriPXE, tt.arch, tt.firmware, tt.filename)
 
 			instance := &packet.Instance{
 				ID:    uuid.New().String(),
 				State: packet.InstanceState(tt.iState),
 			}
 			j := Job{
-				Logger: joblog.With("index", i, "iState", tt.iState, "plan", tt.plan, "allowPXE", tt.allowPXE, "ouriPXE", tt.ouriPXE, "arm", tt.arm, "uefi", tt.uefi, "filename", tt.filename),
+				Logger: joblog.With("index", i, "iState", tt.iState, "allowPXE", tt.allowPXE, "ouriPXE", tt.ouriPXE, "arch", tt.arch, "firmware", tt.firmware, "filename", tt.filename),
 				hardware: &packet.HardwareCacher{
 					ID:       uuid.New().String(),
 					AllowPXE: tt.allowPXE,
-					PlanSlug: "baremetal_" + tt.plan,
 					Instance: instance,
 				},
 				instance: instance,
 			}
-			filename := j.getPXEFilename(tt.ouriPXE, tt.arm, tt.uefi)
+			filename := j.getPXEFilename(tt.arch, tt.firmware, tt.ouriPXE)
 			if tt.filename != filename {
 				t.Fatalf("unexpected filename want:%q, got:%q", tt.filename, filename)
 			}