support a more generic ipxe installer #192

mikemrm · 2021-08-10T19:29:25Z

Description

Allows more generic support for ipxe installers for installation of an OS which cannot be modified to work with the OSIE installation method.

The new ipxe installer works similar to custom_ipxe however differs in that it is selected earlier in the process for different handling of data and which is used for any os which has the installer set to ipxe.

Why is this needed

Some os installation processes have unique installation environments which do not lend themselves easily to osie liveos / workflow setup as seen with other installers in boots like vmware and coreos. However instead of hard coding a new OS into boots, this method allows for those dynamic configurations to be handled separately by allowing the installer to be set to ipxe which supports chaining or injecting a script to boot the installer.

The model has changed for packet.OperatingSystem to now have two additional optional fields Installer and InstallerData.

The ipxe installer is configurable through the InstallerData field which it expects to be a string encoded JSON blob of which two fields may be specified chain which should be a full URL to a valid iPXE script and script which allows for injecting an ipxe script directly.

Example of both configurable fields:

{
  "chain": "https://boot.example.com/unique_os_installer.ipxe",
  "script": "echo Welcome to unique os installer\nshell\n"
}

Example using iPXE installer with chain loading:

{
  "distro": "Unique OS",
  "os_slug": "unique_os",
  "slug": "unique_os_7",
  "version": "7",
  "installer": "ipxe",
  "installer_data": "{\"chain\": \"https://boot.example.com/unique_os_installer.ipxe\"}"
}

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

No changes to existing functionality, only adding additional support.

codecov · 2021-08-10T19:32:21Z

Codecov Report

Merging #192 (1f966ce) into master (94f4394) will increase coverage by 0.56%.
The diff coverage is 72.00%.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   41.38%   41.94%   +0.56%     
==========================================
  Files          41       41              
  Lines        2576     2620      +44     
==========================================
+ Hits         1066     1099      +33     
- Misses       1423     1436      +13     
+ Partials       87       85       -2

Impacted Files	Coverage Δ
job/ipxe.go	`0.00% <0.00%> (ø)`
job/mock.go	`60.91% <0.00%> (-1.92%)`	⬇️
packet/models.go	`78.00% <ø> (ø)`
installers/custom_ipxe/main.go	`100.00% <100.00%> (+30.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 94f4394...1f966ce. Read the comment docs.

jacobweinstock · 2021-08-10T19:59:03Z

Hey @mikemrm, I'm not seeing how this is different than the existing custom_ipxe? custom_ipxe can already handle ipxe scripts via a URL to chain load or just a custom IPXE script.

Also, I've put out a proposal that includes deprecating all installers besides a default. I believe there has been an unofficial desire amongst the community to deprecate all installers too.

mikemrm · 2021-08-10T20:08:09Z

Correct @jacobweinstock however this differs in how it is triggered. The custom_ipxe is only used when an OperatingSystem distro is set to custom_ipxe which it then looks at a field attached to the instance ipxe_script_url.

The trouble is that this process is only triggered due to the distro being custom_ipxe so if we needed to support a new OS which would have a distro of My OS (my_os) which needed to chain load an ipxe script. We would have to overwrite the distro field sent to cacher/tink with custom_ipxe which is undesireable.

This change makes this path more direct by not relying on faking / overriding a field.

mikemrm · 2021-08-10T20:10:03Z

Also, I've put out a proposal that includes deprecating all installers besides a default. I believe there has been an unofficial desire amongst the community to deprecate all installers too.

I agree with that desire and I believe this functionality would allow us to remove the other installers leaving only this method.

jacobweinstock · 2021-08-10T20:18:18Z

@mikemrm , there are actually 2 fields that can trigger a Boots Installer. metadata.instance.operating_system.slug and metadata.instance.operating_system.distro. slug is checked first and then distro is checked.

If I understand the use-case correctly, then the slug can be set to custom_ipxe, and the distro can be anything (my_os) for example.

mikemrm · 2021-08-10T20:24:28Z

Correct, it does appear custom_ipxe is actually only being registered for a slug. Meaning slug is the field that would need to be set.

This however doesn't change the issue, as slugs are typically understood as a unique string. Which in this case is the Operating System Version slug which an OS may have many OSVs.

mmlb

chaining and script can both be done with custom_ipxe, why do we need this instead of custom_ipxe?

mmlb · 2021-08-10T20:53:20Z

chaining and script can both be done with custom_ipxe, why do we need this instead of custom_ipxe?

well I should refresh before commenting it seems

jacobweinstock · 2021-08-10T21:00:58Z

@mikemrm and I talked offline. (@mikemrm correct me if I misspeak here at all!) Here's the synopsis. There is a desire to add an additional hardware data field metadata.instance.operatiing_system.Installer that will be checked before slug and distro. Also, there is a desire to add another new add hardware data field: metadata.instance.operatiing_system.IntallerData. This field would be a string but expected to be a JSON string of {"chain": "", "script":""}.
The reasoning behind this is that it makes sense for the custom business logic internal to Equinix metal that interacts with hardware data via tink/cacher. As the hardware data model in Tink is arbitrary and not enforced, I don't see why this shouldn't be allowed. I did ask Mike to compromise and not create a new Boots Installer but to modify custom_ipxe without breaking backwards compatibility. Mike said he will see what he can do and update this PR.

mmlb · 2021-08-10T21:37:23Z

Do we really need support for both chain and script fields? Chain can already by done by filling into script.

mikemrm · 2021-08-10T21:42:10Z

Do we really need support for both chain and script fields? Chain can already by done by filling into script.

Script was included to make it easier to push an exact script rather than relying on an externally hosted one. This is just a beneficial addition I can see useful for many things including testing without relying on external services.

packet/models.go

installers/custom_ipxe/main.go

jacobweinstock · 2021-08-11T02:01:31Z

I think it's important to note the following. This functionality can already be accomplished via the existing custom_ipxe Installer. In my opinion, what this PR does do is create a more clear mapping from hardware data to Boots installer.
New:
metadata.instance.operating_system.installer -> Boots Installer
Existing:
metadata.instance.operating_system.slug -> Boots Installer
metadata.instance.operating_system.distro -> Boots Installer

jacobweinstock

Thank you for being open to change so much!

installers/custom_ipxe/main.go

jacobweinstock

just a DCO issue to address and this should be good to go!

installers/custom_ipxe/main.go

mmlb · 2021-08-11T15:42:23Z

Do we really need support for both chain and script fields? Chain can already by done by filling into script.

Script was included to make it easier to push an exact script rather than relying on an externally hosted one. This is just a beneficial addition I can see useful for many things including testing without relying on external services.

Right, so why don't we just support only script and if the use wants to chain they can provide the script that does so.

mikemrm · 2021-08-11T19:44:54Z

Do we really need support for both chain and script fields? Chain can already by done by filling into script.

Script was included to make it easier to push an exact script rather than relying on an externally hosted one. This is just a beneficial addition I can see useful for many things including testing without relying on external services.

Right, so why don't we just support only script and if the use wants to chain they can provide the script that does so.

Besides the fact that it supports the two code paths of the original custom_ipxe method. I think it also makes an easy way to do the chain loading and adds minimal code. However if you feel this cannot be, I can remove.

installers/custom_ipxe/main.go

mmlb · 2021-08-11T19:58:45Z

Do we really need support for both chain and script fields? Chain can already by done by filling into script.

Script was included to make it easier to push an exact script rather than relying on an externally hosted one. This is just a beneficial addition I can see useful for many things including testing without relying on external services.

Right, so why don't we just support only script and if the use wants to chain they can provide the script that does so.

Besides the fact that it supports the two code paths of the original custom_ipxe method. I think it also makes an easy way to do the chain loading and adds minimal code. However if you feel this cannot be, I can remove.

Yeah I think this makes sense, it also makes explicit what was implicit before (either you feed in an ipxe script or we'll chainload what is presumably a url).

mmlb · 2021-08-11T21:04:53Z

installers/custom_ipxe/main.go

+	logger := installers.Logger("custom_ipxe")
+	if j.InstanceID() != "" {
+		logger = logger.With("instance.id", j.InstanceID())
+	}


the job should have its own logger already with these fields (and more) already populated, so this should just be:

logger := job.logger.With("installer", "custom_ipxe")

good point. updating

mmlb · 2021-08-11T21:05:48Z

installers/custom_ipxe/main.go

-func bootScript(j job.Job, s *ipxe.Script) {
+func ipxeScript(j job.Job, s *ipxe.Script) {
+	logger := installers.Logger("custom_ipxe")
+	if j.InstanceID() != "" {


Hmm we should probably return a s.Shell if there's no instance id right?

I see your point, will update.

mmlb · 2021-08-11T21:06:13Z

installers/custom_ipxe/main.go

+		if cfg == nil {
+			s.Echo("Installer data not provided")
+			s.Shell()
+			logger.Error(err, "empty installer data")


err is currently nil here, actually this looks like the only use of err at all so no need for var err error right?

good catch, with all the changes this is no longer needed.

Signed-off-by: Mike Mason <mason@packet.com>

displague · 2021-08-11T23:49:22Z

I'm late to the party, but I have brought up previously that a data uri should be supported as a way to offer ipxe scripts. #110

This would be a minimal change that introduces added functionality within the existing spec.

Thoughts, @mikemrm?

mikemrm · 2021-08-12T13:45:07Z

That is an interesting idea @displague however I'm not sure how common it is for someone to want to data uri encode a script and would be interested to see others thoughts about supporting it.

displague · 2021-08-12T14:51:33Z

however I'm not sure how common it is for someone to want to data uri encode a script

I imagine users are not writing templates by hand and would use CLIs and APIs to get the value in place.

We have a URL field and data URLs are an established pattern for providing any data as a URL. Rather than fetch the resource over HTTP, ftp, nntp, gopher, we have the data contained in the URL and the field can be said to support HTTP, HTTPS, and data URLs.

Existing APIs and CLIs that wrap the Tinkerbell API and pass the IPXE URL to Tinkerbell will get this functionality for free (they won't need to be updated for a changed Tinkerbell spec).

displague · 2021-08-12T15:03:18Z

installers/custom_ipxe/main.go

+	} else if j.IPXEScriptURL() != "" {
+		cfg = &packet.InstallerData{Chain: j.IPXEScriptURL()}


To illustrate how we could benefit from Data URL handling (using https://pkg.go.dev/github.com/vincent-petithory/dataurl#example-DecodeString):

} else if dataURL, err := dataurl.DecodeString(j.IPXEScriptURL()); err == nil { // I assume err is returned unless there is a real data URL, but we might need different logic to ensure the URL was actually a data URL and not just empty cfg = &packet.InstallerData{Script: dataURL.Data} } else if j.IPXEScriptURL() != "" { ...

@displague lets give it a go in a different PR.

mmlb reviewed Aug 10, 2021

View reviewed changes

mikemrm force-pushed the add-generic-ipxe-installer branch from b895f56 to 19bd269 Compare August 10, 2021 21:12

mikemrm requested a review from mmlb August 10, 2021 21:44

jacobweinstock requested changes Aug 11, 2021

View reviewed changes

mikemrm force-pushed the add-generic-ipxe-installer branch 2 times, most recently from 2216b3d to 2c9cfa4 Compare August 11, 2021 14:17

mikemrm requested a review from jacobweinstock August 11, 2021 14:20

jacobweinstock requested changes Aug 11, 2021

View reviewed changes

installers/custom_ipxe/main.go Outdated Show resolved Hide resolved

jacobweinstock previously approved these changes Aug 11, 2021

View reviewed changes

jacobweinstock reviewed Aug 11, 2021

View reviewed changes

installers/custom_ipxe/main.go Outdated Show resolved Hide resolved

mikemrm dismissed jacobweinstock’s stale review via cefd35e August 11, 2021 15:39

mikemrm force-pushed the add-generic-ipxe-installer branch from 4e05bd2 to cefd35e Compare August 11, 2021 15:39

mikemrm requested a review from jacobweinstock August 11, 2021 15:45

mmlb reviewed Aug 11, 2021

View reviewed changes

installers/custom_ipxe/main.go Outdated Show resolved Hide resolved

mikemrm force-pushed the add-generic-ipxe-installer branch from cefd35e to 0d7fb58 Compare August 11, 2021 20:07

mikemrm requested a review from mmlb August 11, 2021 20:21

jacobweinstock previously approved these changes Aug 11, 2021

View reviewed changes

mmlb suggested changes Aug 11, 2021

View reviewed changes

mikemrm dismissed jacobweinstock’s stale review via 48fdcab August 11, 2021 21:33

mikemrm added 7 commits August 11, 2021 16:41

add os support for specifying an ipxe configuration for its installer

cf548d0

Signed-off-by: Mike Mason <mason@packet.com>

allow custom_ipxe to handle both paths for booting an ipxe script

c1cb592

Signed-off-by: Mike Mason <mason@packet.com>

adjustments from pr review

6fd00dc

Signed-off-by: Mike Mason <mason@packet.com>

function should not be exported

943bbe5

Signed-off-by: Mike Mason <mason@packet.com>

match package name

6727da0

Signed-off-by: Mike Mason <mason@packet.com>

use job logger instead of creating a new one

b910cdf

Signed-off-by: Mike Mason <mason@packet.com>

handle instance path with no data gracefully

1f966ce

Signed-off-by: Mike Mason <mason@packet.com>

mikemrm force-pushed the add-generic-ipxe-installer branch from 48fdcab to 1f966ce Compare August 11, 2021 21:41

jacobweinstock approved these changes Aug 11, 2021

View reviewed changes

mikemrm requested a review from mmlb August 12, 2021 14:02

displague reviewed Aug 12, 2021

View reviewed changes

mmlb approved these changes Aug 12, 2021

View reviewed changes

mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Aug 12, 2021

mergify bot merged commit 78d4f74 into tinkerbell:master Aug 12, 2021

mikemrm deleted the add-generic-ipxe-installer branch August 12, 2021 15:30

jacobweinstock mentioned this pull request Aug 12, 2021

When an IPXe URL is a data-url, expand the url into ipxe content #110

Open

displague mentioned this pull request Aug 14, 2021

support ipxe scripts over data uris in ipxe script urls #194

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support a more generic ipxe installer #192

support a more generic ipxe installer #192

mikemrm commented Aug 10, 2021

codecov bot commented Aug 10, 2021 •

edited

jacobweinstock commented Aug 10, 2021 •

edited

mikemrm commented Aug 10, 2021 •

edited

mikemrm commented Aug 10, 2021

jacobweinstock commented Aug 10, 2021 •

edited

mikemrm commented Aug 10, 2021

mmlb left a comment

mmlb commented Aug 10, 2021

jacobweinstock commented Aug 10, 2021

mmlb commented Aug 10, 2021

mikemrm commented Aug 10, 2021

jacobweinstock commented Aug 11, 2021

jacobweinstock left a comment

jacobweinstock left a comment

mmlb commented Aug 11, 2021

mikemrm commented Aug 11, 2021

mmlb commented Aug 11, 2021

mmlb Aug 11, 2021

mikemrm Aug 11, 2021

mmlb Aug 11, 2021

mikemrm Aug 11, 2021

mmlb Aug 11, 2021

mikemrm Aug 11, 2021

displague commented Aug 11, 2021

mikemrm commented Aug 12, 2021

displague commented Aug 12, 2021 •

edited

displague Aug 12, 2021

mmlb Aug 12, 2021

		} else if j.IPXEScriptURL() != "" {
		cfg = &packet.InstallerData{Chain: j.IPXEScriptURL()}

support a more generic ipxe installer #192

support a more generic ipxe installer #192

Conversation

mikemrm commented Aug 10, 2021

Description

Why is this needed

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

codecov bot commented Aug 10, 2021 • edited

Codecov Report

jacobweinstock commented Aug 10, 2021 • edited

mikemrm commented Aug 10, 2021 • edited

mikemrm commented Aug 10, 2021

jacobweinstock commented Aug 10, 2021 • edited

mikemrm commented Aug 10, 2021

mmlb left a comment

Choose a reason for hiding this comment

mmlb commented Aug 10, 2021

jacobweinstock commented Aug 10, 2021

mmlb commented Aug 10, 2021

mikemrm commented Aug 10, 2021

jacobweinstock commented Aug 11, 2021

jacobweinstock left a comment

Choose a reason for hiding this comment

jacobweinstock left a comment

Choose a reason for hiding this comment

mmlb commented Aug 11, 2021

mikemrm commented Aug 11, 2021

mmlb commented Aug 11, 2021

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

displague commented Aug 11, 2021

mikemrm commented Aug 12, 2021

displague commented Aug 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2021 •

edited

jacobweinstock commented Aug 10, 2021 •

edited

mikemrm commented Aug 10, 2021 •

edited

jacobweinstock commented Aug 10, 2021 •

edited

displague commented Aug 12, 2021 •

edited