Status Report for Packet Instance #566

Bubblemelon · 2018-06-19T01:28:12Z

This references issue #2444.

The ./ignition build works when it is sent to a Packet instance.

However, has trouble logging via ssh when an image with the changes in igntion is booted onto an instance. I roughly know what is causing this problem.

The commits in the PR are numerous but I will eventually squash it into one commit after removing the debug prints some from logger.Info() and fmt.Println(). So I'm ignoring the tailor check for now, until I
have one squashed commit.

coreosbot · 2018-06-19T01:28:14Z

Can one of the admins verify this patch?

Bubblemelon · 2018-06-19T01:42:31Z

On a Packet Instance, run ./igntion -oem file -clear-cache -log-to-stdout -stage files in the directory where the binary was scp-ed into.

To supply the instance with an ignition config file, use $IGNITION_CONFIG_FILE to lead to the path of the file, i.e. /path/to/config.ign

The Timeline will an error message if the config is invalid. But will not show any messages if no config was given or was correctly booted with. Unless that is desired, but as far as my discussions went with Derek, only show the message on the Timeline if there is an error i.e. when ignition fails, all else remains the same.

Bubblemelon · 2018-06-26T22:48:31Z

internal/oem/oem.go

+		// r := report.Report{
+		// 	Entries: []Entry{
+		// 		{
+		// 			Message: "Ignition Failed",


@ajeddeloh Is it necessary to use report.Report?

No, the report is just for config validation. We should send the actual error to the cloud though (which might be from a report.Report{}, but might not be as well)

Bubblemelon · 2018-06-27T22:45:14Z

Link to blog about metadata.packet.net : https://help.packet.net/technical/hacking/user-state

Bubblemelon · 2018-06-28T17:19:20Z

@ajeddeloh I've added a more detailed error/status message that posts to the Packet timeline (to the user).

After looking at Kola, it looks like it's using an external package from Packet to form an instance, it's not a simple rest api call if I understand correctly. So let me know how you would like me to tell Packet that ignition failed, right now its a simple os.Exit(1) in /internal/main.go

ajeddeloh · 2018-06-28T17:41:56Z

internal/exec/engine.go

@@ -53,10 +53,10 @@ type Engine struct {

 // Run executes the stage of the given name. It returns true if the stage
 // successfully ran and false if there were any errors.
-func (e Engine) Run(stageName string) bool {
+func (e Engine) Run(stageName string) (bool, string) {


This should just return an error. It should have from the start.

ajeddeloh · 2018-06-28T17:42:57Z

internal/exec/engine.go

 	}

 	e.Logger.PushPrefix(stageName)
 	defer e.Logger.PopPrefix()

-	return stages.Get(stageName).Create(e.Logger, e.Root, *e.Fetcher).Run(config.Append(baseConfig, config.Append(systemBaseConfig, cfg)))
+	return stages.Get(stageName).Create(e.Logger, e.Root, *e.Fetcher).Run(config.Append(baseConfig, config.Append(systemBaseConfig, cfg))), "Valid Ignition Config"


The stages should also return an error (this is some long overdue refactoring)

ajeddeloh · 2018-06-28T17:43:29Z

internal/main.go

-	if !engine.Run(flags.stage.String()) {
+	if !ignSuccess {
+		// ignition fails
+		logger.Info("Ignition Fails")


nit: s/Fails/Failed

ajeddeloh · 2018-06-28T17:45:03Z

internal/main.go

 		os.Exit(1)
+	} else {
+		logger.Info("Ignition Passes")


I don't think we need to log sucess, but I'm not that opinionated. If you do want to, maybe "Ignition finished successfully".

ajeddeloh · 2018-06-28T17:45:41Z

internal/resource/http.go

@@ -199,6 +199,7 @@ func (f *Fetcher) newHttpClient() {
 // code, a cancel function for the result's context, and error (if any). By
 // default, User-Agent is added to the header but this can be overridden.
 func (c HttpClient) getReaderWithHeader(url string, header http.Header) (io.ReadCloser, int, context.CancelFunc, error) {
+


spurious newline

ajeddeloh · 2018-06-28T18:28:32Z

internal/providers/packet/packet.go

+			Message: "Ignition: valid config",
+		}
+
+		// Marshall Message


this part is common to both branches of the if and can be broken out

ajeddeloh · 2018-06-28T18:34:36Z

internal/oem/oem.go

@@ -61,6 +62,21 @@ func (c Config) NewFetcherFunc() providers.FuncNewFetcher {
 	}
 }

+// Status takes b as whether ignition passes (true) or fails
+// msg is the error or success message
+func (c Config) Status(b bool, msg string) providers.FuncStatusFetcher {


there shouldn't be packet specific stuff in oem/oem. Instead the packet version should be registered below.

ajeddeloh · 2018-06-28T18:42:39Z

internal/oem/oem.go

@@ -40,6 +40,7 @@ type Config struct {
 	name       string
 	fetch      providers.FuncFetchConfig
 	newFetcher providers.FuncNewFetcher
+	status     providers.FuncStatusFetcher


maybe statusFunc?

ajeddeloh · 2018-06-28T18:47:13Z

internal/resource/url.go

@@ -219,9 +219,13 @@ func (f *Fetcher) FetchFromTFTP(u url.URL, dest *os.File, opts FetchOptions) err
 // FetchFromHTTP fetches a resource from u via HTTP(S) into dest, returning an
 // error if one is encountered.
 func (f *Fetcher) FetchFromHTTP(u url.URL, dest *os.File, opts FetchOptions) error {
+


spurious newline

ajeddeloh · 2018-06-28T18:47:41Z

internal/resource/url.go

 	if f.client == nil {
+		logger := log.New(true)


why this addition?

f enters FetchfromHTTP as nil, for Looger and client

Rather than instantiating the logger here would it work to change the Fetcher instantiation here to call rf, err := c.NewFetcherFunc()(log.New(true)?

arithx · 2018-06-28T19:02:49Z

internal/providers/packet/packet.go

+		fmt.Println("Error: ", err)
+	}
+
+	type Metadata struct {


nit:
Rather than declaring this type, which is only used once inline, can you not just do this when assinging the metadata variable:

metadata := struct { PhoneHomeURL string `json:"phone_home_url"` }{}

If you do want to keep the type I think it should be moved outside of the function.

arithx · 2018-06-28T19:10:54Z

internal/providers/packet/packet.go

+		fmt.Println("Error: ", err)
+	}
+	postReq.Header.Set("Content-Type", "application/json")
+	client := &http.Client{}


@ajeddeloh: would it make sense to make a GetHttpClient function of the Fetcher object and/or make a Post call? That way we can get clients with the correct Transport & http.Client customizations easier?

hmmm actually I don't think so. Ignition could have failed because of wonky timeouts/CAs. We shouldn't need those customizations here, especially since this is packet specific.

arithx · 2018-06-28T19:16:20Z

internal/providers/packet/packet.go

+	}
+
+	if err != nil {
+		fmt.Println("Error: ", err)


Drop fmt calls. If you need to log a message use log

arithx · 2018-07-02T15:18:12Z

internal/distro/distro.go

@@ -59,7 +59,7 @@ func DiskByPartUUIDDir() string { return diskByPartUUIDDir }
 func OEMDevicePath() string     { return fromEnv("OEM_DEVICE", oemDevicePath) }

 func KernelCmdlinePath() string { return kernelCmdlinePath }
-func SystemConfigDir() string   { return fromEnv("SYSTEM_CONFIG_DIR", systemConfigDir) }
+func SystemConfigDir() string   { return fromEnv("SYSTEM_CONFIG_DIR", "/home/core") }


Why is this being changed?

When running ignition and setting -oem as packet, ignition doesn't seem to take the exported env SYSTEM_CONFIG_DIRso I hard coded the path for the mean time.

arithx · 2018-07-02T15:19:05Z

internal/exec/engine.go

 	if e.Fetcher == nil || e.Logger == nil {
 		fmt.Fprintf(os.Stderr, "engine incorrectly configured\n")
-		return false
+		err := errors.ErrEngineConfiguration
+		return false, err


You can just directly return errors.ErrEngineConfiguration rather than using a temporary variable.

arithx · 2018-07-02T15:23:17Z

internal/exec/engine.go

 	}

 	e.Logger.PushPrefix(stageName)
 	defer e.Logger.PopPrefix()

-	return stages.Get(stageName).Create(e.Logger, e.Root, *e.Fetcher).Run(config.Append(baseConfig, config.Append(systemBaseConfig, cfg)))
+	return stages.Get(stageName).Create(e.Logger, e.Root, *e.Fetcher).Run(config.Append(baseConfig, config.Append(systemBaseConfig, cfg))), err


Have you tested this change in a situation where you'd receive a ErrNoProvider?

In internal/providers/system/system.go (line 55) when a config is not given at the default path as specified by $SYSTEM_CONFIG_DIR, ErrNoProvider will be called for example, and will show: Ignition error: config provider was not online

arithx · 2018-07-02T15:24:19Z

internal/oem/oem.go

+// Status takes b as whether ignition passes (true) or fails
+// msg is the error or success message
+func (c Config) Status(f resource.Fetcher, status error) error {
+


Looks like your editor is sneaking in a lot of random newlines.

arithx · 2018-07-02T15:25:05Z

internal/providers/packet/packet.go

@@ -18,7 +18,12 @@
 package packet

 import (
+	"bytes"
+	"encoding/json"
+


Drop this extra newline

arithx · 2018-07-02T15:28:31Z

internal/providers/packet/packet.go

+	data, err := f.FetchToBuffer(metadataUrl, resource.FetchOptions{
+		Headers: nil,
+	})
+	if data == nil {


Is there a possible case where a nil is returned for both data & err? If so then this function will appear to pass despite not working. Also this check should probably be combined with the err != nil check.

Data Err return value

!nil !nil err

!nil nil do nothing

nil !nil err

nil nil ErrValidFetchEmptyData

for this:

if err != nil { return err } if data == nil { return errors.ErrValidFetchEmptyData }

internal/providers/packet/packet.go ( line 72 )

arithx · 2018-07-02T15:32:17Z

internal/resource/url.go

-	if f.client == nil {
-		f.newHttpClient()
-	}
+	// loging := log.New(true)


Drop the commented code.

Bubblemelon · 2018-07-10T03:49:40Z

Here's my .sh file to build and test ignition Packet changes on the SDK if anyone wants this.

Also please ^ critique ! 😄

ajeddeloh · 2018-07-10T20:07:51Z

internal/exec/engine.go

@@ -53,10 +53,10 @@ type Engine struct {

 // Run executes the stage of the given name. It returns true if the stage
 // successfully ran and false if there were any errors.
-func (e Engine) Run(stageName string) bool {
+func (e Engine) Run(stageName string) (bool, error) {


The bool here is redundant and can be dropped completely. It's always true when the error is nil and false when the error is non-nil.

ajeddeloh · 2018-07-10T20:08:59Z

internal/exec/engine.go

 	}

 	e.Logger.PushPrefix(stageName)
 	defer e.Logger.PopPrefix()

-	return stages.Get(stageName).Create(e.Logger, e.Root, *e.Fetcher).Run(config.Append(baseConfig, config.Append(systemBaseConfig, cfg)))
+	return stages.Get(stageName).Create(e.Logger, e.Root, *e.Fetcher).Run(config.Append(baseConfig, config.Append(systemBaseConfig, cfg))), err


The stage should be changed to return an error instead of a bool and that should be returned here.

ajeddeloh · 2018-07-10T20:10:42Z

internal/main.go

+		} else {
+			logger.Info("ERRORRRRR IS NIL")
+		}
+		statusErr := engine.OEMConfig.Status(*engine.Fetcher, err)


minor nit: these twoi lines can be combined into something like:

if statusErr := engine.OEM... ; statusErr != nil {

ajeddeloh · 2018-07-10T20:11:35Z

internal/oem/oem.go

+// msg is the error or success message
+func (c Config) Status(f resource.Fetcher, statusErr error) error {
+	if c.status != nil {
+		err := c.status(f, statusErr)


you can just return this here and let the caller log it.

ajeddeloh · 2018-07-10T20:11:53Z

internal/resource/url.go

@@ -241,7 +246,6 @@ func (f *Fetcher) FetchFromHTTP(u url.URL, dest *os.File, opts FetchOptions) err
 	default:
 		return ErrFailed
 	}
-


dropped this

ajeddeloh · 2018-07-10T20:13:02Z

internal/providers/packet/packet.go

+			State:   "failed",
+			Message: message,
+		}
+


extra newline

ajeddeloh · 2018-07-10T20:18:02Z

internal/providers/packet/packet.go

+
+	} else {
+
+		err = postMessage(errMsg, f, "Ignition: valid config", postMessageURL)


s/Ignition: valid config/Ignition stage exited successful
You can have a valid config and still fail.

ajeddeloh · 2018-07-10T20:18:24Z

internal/providers/packet/packet.go

+	postMessageURL := phonehomeURL + "/events"
+	if errMsg != nil {
+
+		err = postMessage(errMsg, f, "Ignition error: "+errMsg.Error(), postMessageURL)


Probably want to include the stage (e.g. files/disks)

the stage name?

Bubblemelon · 2018-07-12T04:21:30Z

The script that terminates Packet instances running for more than a day doesn't seem to be working.

@ajeddeloh I believe you've mentioned of such script before.

arithx · 2018-07-12T04:25:02Z

@Bubblemelon are you referring to the gc jobs? If so was the instance spawned by kola (or does it have a mantle tag), because only machines with that tag are automatically cleaned up.

ajeddeloh · 2018-07-13T18:28:46Z

internal/distro/distro.go

@@ -59,7 +59,7 @@ func DiskByPartUUIDDir() string { return diskByPartUUIDDir }
 func OEMDevicePath() string     { return fromEnv("OEM_DEVICE", oemDevicePath) }

 func KernelCmdlinePath() string { return kernelCmdlinePath }
-func SystemConfigDir() string   { return fromEnv("SYSTEM_CONFIG_DIR", systemConfigDir) }
+func SystemConfigDir() string   { return fromEnv("SYSTEM_CONFIG_DIR", "/home/core/") }


this should be changed back before merging

ajeddeloh · 2018-07-13T18:29:03Z

internal/exec/engine.go

@@ -16,6 +16,7 @@ package exec

 import (
 	"encoding/json"
+


remove extra newline

ajeddeloh · 2018-07-13T20:52:42Z

internal/exec/engine.go

 	}

 	e.Logger.PushPrefix(stageName)
 	defer e.Logger.PopPrefix()

-	return stages.Get(stageName).Create(e.Logger, e.Root, *e.Fetcher).Run(config.Append(baseConfig, config.Append(systemBaseConfig, cfg)))
+	if err = stages.Get(stageName).Create(e.Logger, e.Root, *e.Fetcher).Run(config.Append(baseConfig, config.Append(systemBaseConfig, cfg))); err != nil {
+		e.Logger.Crit("%s did not pass", stageName)


I would actually use fmt.Fprintf(os.Stderr, ... since one of the failure modes is that Logger could be nil.

nit: "Stage %s failed" instead of "%s did not pass".

ajeddeloh · 2018-07-13T20:54:06Z

internal/main.go

-	if !engine.Run(flags.stage.String()) {
+	if err = engine.Run(flags.stage.String()); err != nil {
+		logger.Crit("Ignition failed")
+		if err != nil {


this is always true

ajeddeloh · 2018-07-13T21:31:07Z

internal/main.go

+		if err != nil {
+			logger.Err(err.Error())
+		}
+		if statusErr := engine.OEMConfig.Status(flags.stage.String(), *engine.Fetcher, err); statusErr != nil {


would be good to break this common code out of the if.

ajeddeloh · 2018-07-13T21:33:36Z

internal/exec/stages/disks/disks.go

@@ -163,3 +165,8 @@ func (s stage) waitOnDevicesAndCreateAliases(devs []string, ctxt string) error {

 	return nil
 }
+
+func newDiskStageError(msg string, err error) error {


I would just use fmt.Errorf in the call sites.

ajeddeloh · 2018-07-13T21:34:50Z

internal/providers/packet/packet.go

+	// POST Message to phonehome IP
+	postMessageURL := phonehomeURL + "/events"
+
+	err = postMessage(stageName, errMsg, f, postMessageURL)


just return postMessage(...)

ajeddeloh · 2018-07-13T21:38:24Z

internal/providers/packet/packet.go

+}
+
+// postMessage makes a post request with the supplied message to the url
+func postMessage(stageName string, e error, f resource.Fetcher, url string) error {


I think f is unused.

ajeddeloh

Mostly lgtm, except a few nits. Can you rework the commits to have something like:

rework run to return an error
add the framework for reporting statues
add the packet implementation

ajeddeloh · 2018-07-20T17:08:42Z

internal/exec/engine.go

+		return err
+	}
+	e.Logger.Info("passed")
+	return err


nit: return nil here (since err must be nil) to make it clear that this is the sucessful case

ajeddeloh · 2018-07-20T17:10:34Z

internal/main.go

 		os.Exit(1)
+	} else {


nit: Don't need the else since the other branch exits

ajeddeloh · 2018-07-20T17:11:19Z

internal/providers/packet/packet.go

+	err = json.Unmarshal(data, &metadata)
+	if err != nil {
+		return err
+


nit: extra newline

Covered suggested/requested items and had gone over them with Andrew.

ajeddeloh

Two minor nits: some wording in a message and an extra newline, other than that LGTM

ajeddeloh · 2018-07-31T23:16:48Z

internal/exec/engine.go

+		fmt.Fprintf(os.Stderr, "%s failed", stageName)
+		return err
+	}
+	e.Logger.Info("passed")


Add stage name here like in the failed message.

okay fixed the new line in packet.go on line 131 and changed "passed" to "%s passed", stageName see here.

ajeddeloh · 2018-07-31T23:24:38Z

internal/providers/packet/packet.go

+		fmt.Println("Error: ", err)
+	}
+	postReq.Header.Set("Content-Type", "application/json")
+	client := &http.Client{}


hmmm actually I don't think so. Ignition could have failed because of wonky timeouts/CAs. We shouldn't need those customizations here, especially since this is packet specific.

Run() returns an error instead of a bool.

main.go checks if stage Run() is successful. oem.go implements the a Status() function that calls the oem specific fucntion to report the ignition status. FetchFromHTTP in url.go will check if f.client is nill.

packet.go POST igntion status messages to Packet.net via Packet's api url.

Bubblemelon changed the title ~~Status Report for Packet Instance~~ WIP: Status Report for Packet Instance Jun 19, 2018

Bubblemelon force-pushed the status-report-packetcloud branch 2 times, most recently from e07d915 to 69d0703 Compare June 26, 2018 22:54

Bubblemelon commented Jun 26, 2018

View reviewed changes

Bubblemelon force-pushed the status-report-packetcloud branch from 9e76179 to ed97ed5 Compare June 28, 2018 00:10

Bubblemelon changed the title ~~WIP: Status Report for Packet Instance~~ Status Report for Packet Instance Jun 28, 2018

Bubblemelon force-pushed the status-report-packetcloud branch from 9a5c914 to f59dbcd Compare June 28, 2018 17:12

ajeddeloh suggested changes Jun 28, 2018

View reviewed changes

arithx suggested changes Jun 28, 2018

View reviewed changes

Bubblemelon force-pushed the status-report-packetcloud branch 4 times, most recently from b730e32 to e1b840a Compare June 30, 2018 01:16

arithx previously requested changes Jul 2, 2018

View reviewed changes

Bubblemelon force-pushed the status-report-packetcloud branch from 053bc11 to 70c3c0e Compare July 10, 2018 03:39

ajeddeloh suggested changes Jul 10, 2018

View reviewed changes

Bubblemelon force-pushed the status-report-packetcloud branch from 7dd8d61 to 849c30c Compare July 12, 2018 04:15

ajeddeloh suggested changes Jul 13, 2018

View reviewed changes

Bubblemelon force-pushed the status-report-packetcloud branch 2 times, most recently from 17210d0 to f6a5001 Compare July 17, 2018 20:56

ajeddeloh suggested changes Jul 20, 2018

View reviewed changes

Bubblemelon force-pushed the status-report-packetcloud branch 2 times, most recently from 777cb1c to e066080 Compare July 23, 2018 21:54

Bubblemelon force-pushed the status-report-packetcloud branch 2 times, most recently from 4895b7a to b7856d2 Compare July 30, 2018 23:57

coreos deleted a comment from arithx Jul 31, 2018

ajeddeloh approved these changes Jul 31, 2018

View reviewed changes

Bubblemelon added 3 commits July 31, 2018 20:30

stages: Change Run() to return error

ce93f0a

Run() returns an error instead of a bool.

internal/oem: Implement framework for reporting status

9526d0f

main.go checks if stage Run() is successful. oem.go implements the a Status() function that calls the oem specific fucntion to report the ignition status. FetchFromHTTP in url.go will check if f.client is nill.

oem/packet: Add Packet report status implementation

d604cd9

packet.go POST igntion status messages to Packet.net via Packet's api url.

Bubblemelon force-pushed the status-report-packetcloud branch from b7856d2 to d604cd9 Compare August 1, 2018 03:34

Bubblemelon merged commit 6fe6988 into coreos:master Aug 1, 2018

jlebon mentioned this pull request Aug 1, 2018

stages/files: relabel files for SELinux after boot #569

Merged

Bubblemelon deleted the status-report-packetcloud branch August 7, 2018 18:12

ajeddeloh mentioned this pull request Aug 7, 2018

Add ability for Ignition to report provisioning failures #592

Closed

Data	Err	return value
`!nil`	`!nil`	`err`
`!nil`	`nil`	do nothing
`nil`	`!nil`	`err`
`nil`	`nil`	`ErrValidFetchEmptyData`


		} else {

		err = postMessage(errMsg, f, "Ignition: valid config", postMessageURL)

Status Report for Packet Instance #566

Status Report for Packet Instance #566

Conversation

Bubblemelon commented Jun 19, 2018 • edited Loading

coreosbot commented Jun 19, 2018

Bubblemelon commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bubblemelon commented Jun 27, 2018

Bubblemelon commented Jun 28, 2018

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

arithx Jun 29, 2018 • 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

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

Bubblemelon commented Jul 10, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bubblemelon commented Jul 12, 2018

arithx commented Jul 12, 2018

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

ajeddeloh 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

ajeddeloh 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

Bubblemelon commented Jun 19, 2018 •

edited

Loading

arithx Jun 29, 2018 •

edited

Loading

Bubblemelon commented Jul 10, 2018 •

edited

Loading