add formatting for hcl parsing error messages #5972

jazzyfresh · 2019-07-17T16:16:01Z

Overview

Clean up error messages produced by hcl2 library

Behavior

Before (error messages had these weird numbers in them)

`nomad agent -dev` logs

`nomad alloc status` command

After (removed weird numbers and make message about invalid labels, not JSON)

`nomad agent -dev` logs

`nomad alloc status` command

Implementation

added private method to helper/pluginutils/hclutils/utils.go for hcl diagnostics error message formatting
- it constructs an error from the diagnostic summary and detail
- it ignores the diagnostic subject, which is just garbage "file line numbers" constructed from streamed bytes
- if it finds a diagnostic that matches "Extraneous JSON label..." it creates an error with a nicer message
- future work: submit pull request to hcl2 library with better error messages
also changed where the task event emission for an error happens
- moved it inside runDriver (was formerly in MAIN: in run())
- added task events for each error
- config parsing errors emit TaskFailedValidation event
- this appears to remove the "failed to parse config" logs that were added to nomad alloc status results when the event was TaskDriverFailure, so I might need to look into that more if we want to preserve that behavior

nickethier

I've never been so excited for error formatting! Looking good, just want to clear up my confusion on the comment about the err var.

nickethier · 2019-07-17T16:22:48Z

client/allocrunner/taskrunner/task_runner.go

 	if diag.HasErrors() {
-		return multierror.Append(errors.New("failed to parse config"), diag.Errs()...)
+		tr.EmitEvent(structs.NewTaskEvent(structs.TaskFailedValidation).SetDriverError(err))


I can't tell where this err comes from, is it the correct error to use here or should it be something from diagErrs?

thank you, good catch! I am supposed to pass in the multierror that gets created in the next line, but blindly copy-pastad and forgot to change it. This was part of an attempt to create more granular task events (eg. TaskFailedValidation for config parsing failures, and TaskDriverFailure for failures that were because of the driver), so I moved the event emission out of MAIN: and into runDriver(). I am not sure if that is too messy for a small change like this, so I can put the task events back the way they were...

I think this is fixed, but please give a double check

client/allocrunner/taskrunner/task_runner.go

preetapan · 2019-07-17T16:56:43Z

helper/pluginutils/hclutils/testing.go

@@ -70,7 +70,7 @@ func (b *HCLParser) parse(t *testing.T, config, out interface{}) {
 	decSpec, diags := hclspecutils.Convert(b.spec)
 	require.Empty(t, diags)

-	ctyValue, diag := ParseHclInterface(config, decSpec, b.vars)
+	ctyValue, diag, _ := ParseHclInterface(config, decSpec, b.vars)


use err instead of _ here, and require.Nil on the error

preetapan

Had one test suggestion.

Also, could you update the PR description with a before and after? Was hard to tell what this change ends up doing to error messages.

jazzyfresh · 2019-07-17T17:14:12Z

Thank you @preetapan, I also added a separate TestParseInvalid to test this behavior directly. Working on the description now...

jazzyfresh · 2019-07-17T17:42:57Z

@preetapan @nickethier description is updated

nickethier · 2019-07-17T20:05:51Z

@jazzyfresh Could you look into the test failures they look to be related to your changes

jazzyfresh · 2019-07-17T20:30:02Z

@nickethier yes.. I knew I would inevitably break something in GOTEST_PKGS_EXCLUDE="./api|./client|./drivers/docker|./drivers/exec|./nomad"

https://travis-ci.org/hashicorp/nomad/jobs/560098109#L793

taskrunner/task_runner.go:489: task_runner: running driver failed: task=web error="driver failure"

My guess is that changing the task emit events changed something... or it's something else, I'm still looking into it.

nickethier

Nice work! LGTM once lint/tests are passing!

notnoop

Great. This feels much better already; thanks! I noticed a couple of formatting issues, but good otherwise.

notnoop · 2019-07-18T03:15:17Z

helper/pluginutils/hclutils/util.go

+func formattedDiagnosticErrors(diag hcl.Diagnostics) []error {
+	var errs []error
+	for _, d := range diag {
+		if d.Summary == "Extraneous JSON object property" {


I would add a comment pointing to where this text originates, so future us and notice when it changes or if this is no longer needed.

👍 Added the comment before the method

notnoop · 2019-07-18T03:26:34Z

client/allocrunner/taskrunner/task_runner.go

 	if diag.HasErrors() {
-		return multierror.Append(errors.New("failed to parse config"), diag.Errs()...)
+		parseErr := multierror.Append(errors.New("failed to parse config"), diagErrs...)
+		tr.EmitEvent(structs.NewTaskEvent(structs.TaskFailedValidation).SetDriverError(parseErr))


I see that in the snippet that this returns a generic Validation of task failed as the message. It's because TaskFailedValidation expects ValidationError[1] to be set instead of DriverError.

Also, using the multierror.Append generates very awkward messages, like:

2019-07-18T10:22:30+07:00 Failed Validation 2 error(s) occurred: * failed to parse config * Invalid label: No argument or block type is named "bad_key".

It'd be nice to format the errors ourselves and present something like the following

2019-07-18T10:22:30+07:00 Failed Validation failed to parse config; found the following: * Invalid label: No argument or block type is named "bad_key".

[1]

nomad/command/alloc_status.go

Lines 374 to 379 in 87d716e

case api.TaskFailedValidation:

if event.ValidationError != "" {

desc = event.ValidationError

} else {

desc = "Validation of task failed"

}

Going to address better multierror formatting in another PR

notnoop · 2019-07-18T03:41:51Z

helper/pluginutils/loader/init.go

-		multierror.Append(&mErr, diag.Errs()...)
-		return multierror.Prefix(&mErr, "failed parsing config:")
+		multierror.Append(&mErr, diagErrs...)
+		return multierror.Prefix(&mErr, "failed to parse config")


I'd keep the trailing colon here, as Prefix() prefixes all elements rather than just once on top. I have a sample output in https://play.golang.org/p/sj0JcGRRVGH .

Though, I'd suggest avoiding multierror formatting. I find it awkward and we can reuse the formatter from other callers.

notnoop · 2019-07-18T03:43:33Z

plugins/shared/cmd/launcher/command/device.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"github.com/hashicorp/go-multierror"


This line should appear in following grouping of imports, along with other non-stdlib imports. I'd suggest configuring your IDE to use goimports on save instead of plain gofmt.

notnoop · 2019-07-18T03:44:06Z

plugins/shared/cmd/launcher/command/device.go

-			errStr = fmt.Sprintf("%s\n* %s", errStr, err.Error())
-		}
-		return errors.New(errStr)
+		return multierror.Append(errors.New("failed to parse config"), diagErrs...)


Of all of these, I like this previous formatting the most :P.

github-actions · 2023-02-06T02:15:49Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

add formatting for hcl parsing error messages

22732ab

jazzyfresh added the 0.9.4 label Jul 17, 2019

jazzyfresh requested a review from notnoop July 17, 2019 16:16

nickethier requested changes Jul 17, 2019

View reviewed changes

preetapan reviewed Jul 17, 2019

View reviewed changes

preetapan requested changes Jul 17, 2019

View reviewed changes

ParseHclInterface test for invalid labels

c0bb2af

fix task event errors

b473b3b

preetapan approved these changes Jul 17, 2019

View reviewed changes

nickethier approved these changes Jul 18, 2019

View reviewed changes

notnoop suggested changes Jul 18, 2019

View reviewed changes

fix broken test by adding TaskDriverFailure and refactor for code review

90d72e0

jazzyfresh merged commit e31db57 into master Jul 19, 2019

github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add formatting for hcl parsing error messages #5972

add formatting for hcl parsing error messages #5972

jazzyfresh commented Jul 17, 2019 •

edited

Loading

nickethier left a comment

nickethier Jul 17, 2019

jazzyfresh Jul 17, 2019

jazzyfresh Jul 17, 2019

preetapan Jul 17, 2019

jazzyfresh Jul 17, 2019

preetapan left a comment

jazzyfresh commented Jul 17, 2019

jazzyfresh commented Jul 17, 2019

nickethier commented Jul 17, 2019

jazzyfresh commented Jul 17, 2019 •

edited

Loading

nickethier left a comment

notnoop left a comment

notnoop Jul 18, 2019

jazzyfresh Jul 19, 2019

notnoop Jul 18, 2019

jazzyfresh Jul 19, 2019

notnoop Jul 18, 2019

notnoop Jul 18, 2019

notnoop Jul 18, 2019

github-actions bot commented Feb 6, 2023

	case api.TaskFailedValidation:
	if event.ValidationError != "" {
	desc = event.ValidationError
	} else {
	desc = "Validation of task failed"
	}

add formatting for hcl parsing error messages #5972

add formatting for hcl parsing error messages #5972

Conversation

jazzyfresh commented Jul 17, 2019 • edited Loading

Overview

Behavior

Before (error messages had these weird numbers in them)

nomad agent -dev logs

nomad alloc status command

After (removed weird numbers and make message about invalid labels, not JSON)

nomad agent -dev logs

nomad alloc status command

Implementation

nickethier 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

preetapan left a comment

Choose a reason for hiding this comment

jazzyfresh commented Jul 17, 2019

jazzyfresh commented Jul 17, 2019

nickethier commented Jul 17, 2019

jazzyfresh commented Jul 17, 2019 • edited Loading

nickethier left a comment

Choose a reason for hiding this comment

notnoop 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

github-actions bot commented Feb 6, 2023

jazzyfresh commented Jul 17, 2019 •

edited

Loading

`nomad agent -dev` logs

`nomad alloc status` command

`nomad agent -dev` logs

`nomad alloc status` command

jazzyfresh commented Jul 17, 2019 •

edited

Loading