Fix hardware not found response #365

kqdeng · 2020-11-13T16:35:27Z

Description

When getting hardware and hardware is not found, the error unexpected end of JSON input is no longer returned.
The error returned will be a hardware not found: sql: no rows in result set instead.

In addition, the error message received when trying to create a workflow with a worker/hardware not present in the database is now:

2020/11/18 21:24:01 rpc error: code = Unknown desc = failed to create workflow: unable to insert into action list: no worker found: mac: b4:96:91:5f:ac:00

Why is this needed

The error message was vague and misleading.

Error message before (as seen in boots):

boots_1        | {"level":"info","ts":1605204343.6437285,"caller":"src/dhcp.go:76","msg":"retrieved job is empty","service":"github.com/tinkerbell/boots","pkg":"main","type":"DHCPDISCOVER","mac":"f8:f2:1e:a6:cd:10","err":"discover from dhcp message: get hardware by mac from tink: rpc error: code = Unknown desc = unexpected end of JSON input","errVerbose":"rpc error: code = Unknown desc = unexpected end of JSON input\nget hardware by mac from tink\ngithub.51.al/tinkerbell/boots/packet.
...

Error message after:

boots_1        | {"level":"info","ts":1605289315.5323317,"caller":"myapp/dhcp.go:76","msg":"retrieved job is empty","service":"github.com/tinkerbell/boots","pkg":"main","type":"DHCPDISCOVER","mac":"b4:96:91:5f:ac:e0","err":"discover from dhcp message: get hardware by mac from tink: rpc error: code = Unknown desc = sql: no rows in result set","errVerbose":"rpc error: code = Unknown desc = sql: no rows in result set\nget hardware by mac from tink\ngithub.51.al/tinkerbell/boots/packet..
...

(I know, sorry for the bad formatting)

How Has This Been Tested?

Manually

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

N/A

Checklist:

I have:

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

codecov · 2020-11-13T16:38:26Z

Codecov Report

Merging #365 (99800d5) into master (47f1e12) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #365   +/-   ##
=======================================
  Coverage   24.74%   24.74%           
=======================================
  Files          14       14           
  Lines        1277     1277           
=======================================
  Hits          316      316           
  Misses        940      940           
  Partials       21       21

Impacted Files	Coverage Δ
grpc-server/workflow.go	`21.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 47f1e12...99800d5. Read the comment docs.

mmlb · 2020-11-13T19:52:08Z

db/db.go

-		err = errors.Wrap(err, "SELECT")
+	if err != nil {
+		if err == sql.ErrNoRows {
+			return "", err // if sql.ErrNoRows, return error as is


we should wrap this error

oops i missed this comment. The reason I didn't wrap this error was because sql.ErrNoRows is checked here:

tink/db/workflow.go

Line 112 in 71a1c33

if err == sql.ErrNoRows {

If I wrap the error, it becomes harder to check (unless you have any suggestions).

ah true, that line should actually do if err.Cause() == sql.ErrNoRows { then.

mmlb · 2020-11-13T19:53:30Z

db/workflow.go

@@ -108,7 +108,7 @@ func insertActionList(ctx context.Context, db *sql.DB, yamlData string, id uuid.
 		}

 		workerID, err := getWorkerID(ctx, db, task.WorkerAddr)
-		if err != nil {
+		if err != nil && err != sql.ErrNoRows {


Can you clear up why its ok to not return an error when err == sql.ErrNoRows, its not immediately obvious to me.
On second thought why don't we return an error in that case?

oh sorry it's because workerID == "" is basically the same as sql.ErrNoRows in this case. But I realize it's a better idea to switch the order of the check so it checks for ErrNoRows first and then any other error.

ah yes I see. Yeah lets handle ErrNoRows first&explicitly so its clearer :D 👍

mmlb · 2020-11-16T18:50:40Z

db/template.go

@@ -49,7 +49,7 @@ func (d TinkDB) CreateTemplate(ctx context.Context, name string, data string, id
 func (d TinkDB) GetTemplate(ctx context.Context, fields map[string]string) (string, string, string, error) {
 	getCondition, err := buildGetCondition(fields)
 	if err != nil {
-		return "", "", "", errors.Wrap(err, "failed to build get condition")
+		return "", "", "", errors.Wrap(err, "failed to get template")


this should actually be errors.WithMessage not wrap but we don't need to change that now. Using errors.Wrap on an already Wrapped error will show up with a new stack trace for each Wrap which isn't really what we want. WithMessage just adds more messages to the original stack trace.

mmlb · 2020-11-16T18:52:59Z

db/workflow.go

@@ -109,9 +109,10 @@ func insertActionList(ctx context.Context, db *sql.DB, yamlData string, id uuid.

 		workerID, err := getWorkerID(ctx, db, task.WorkerAddr)
 		if err != nil {
+			if err == sql.ErrNoRows {


this should be if errors.Cause(err) == sql.ErrNoRows { right?

Right... I put the check for errors.Cause somewhere else.. mistakenly 🤦‍♀️

mmlb · 2020-11-16T18:54:07Z

db/db.go

+			return "", errors.Wrap(func() error {
+				return func() error {
+					return err
+				}()
+			}(), "hardware not found")


what is going on here? :D. Why isn't this just return "", errors.Wrap(err, "hardware not found")?

This is implementing the method for the Cause to work, basically making the sql.ErrNoRows error the "cause"

Should I just create a global var HardwareNotFound and then check for that instead?

Signed-off-by: Kelly Deng <kelly@packet.com>

mmlb · 2020-11-18T17:39:21Z

db/workflow.go

@@ -109,9 +109,10 @@ func insertActionList(ctx context.Context, db *sql.DB, yamlData string, id uuid.

 		workerID, err := getWorkerID(ctx, db, task.WorkerAddr)
 		if err != nil {
+			if errors.Cause(err) == sql.ErrNoRows {
+				return fmt.Errorf("hardware mentioned with reference %s not found", task.WorkerAddr)


why not `errors.WithMessage() here instead of dropping the stack frame and all the goodness from errors.Wrap?

…own desc = unexpected end of JSON input' error Signed-off-by: Kelly Deng <kelly@packet.com>

mmlb

one last change and we're done!

mmlb · 2020-11-19T18:25:22Z

db/workflow.go

-	return get(ctx, db, query, arg)
+	id, err := get(ctx, db, query, arg)
+	if errors.Cause(err) == sql.ErrNoRows {
+		err = errors.WithMessagef(errors.New(mac), "mac")


errors.WithMessage

Signed-off-by: Kelly Deng <kelly@packet.com>

kqdeng force-pushed the fix-hardware-not-found-response branch from ff43d32 to 1de2a45 Compare November 13, 2020 17:34

mmlb suggested changes Nov 13, 2020

View reviewed changes

kqdeng force-pushed the fix-hardware-not-found-response branch 2 times, most recently from 71a1c33 to d82eefd Compare November 16, 2020 15:17

mmlb reviewed Nov 16, 2020

View reviewed changes

mmlb suggested changes Nov 16, 2020

View reviewed changes

kqdeng force-pushed the fix-hardware-not-found-response branch 2 times, most recently from 0d31a6b to 09e19c1 Compare November 16, 2020 22:10

mmlb self-requested a review November 16, 2020 22:46

converge if statements in TestGetTemplate

ce52ece

Signed-off-by: Kelly Deng <kelly@packet.com>

kqdeng force-pushed the fix-hardware-not-found-response branch from 43d0527 to 4aab209 Compare November 18, 2020 14:32

mmlb reviewed Nov 18, 2020

View reviewed changes

fix: get returns sql.ErrNoRows when hardware not found to avoid 'Unkn…

7473ee9

…own desc = unexpected end of JSON input' error Signed-off-by: Kelly Deng <kelly@packet.com>

kqdeng force-pushed the fix-hardware-not-found-response branch 2 times, most recently from bae07fb to 14c327f Compare November 18, 2020 21:34

kqdeng requested a review from mmlb November 18, 2020 21:55

mmlb suggested changes Nov 19, 2020

View reviewed changes

kqdeng added 2 commits November 19, 2020 13:28

fix error message format

8f0e53c

Signed-off-by: Kelly Deng <kelly@packet.com>

clean up some error messages

99800d5

Signed-off-by: Kelly Deng <kelly@packet.com>

kqdeng force-pushed the fix-hardware-not-found-response branch from 14c327f to 99800d5 Compare November 19, 2020 18:29

mmlb approved these changes Nov 19, 2020

View reviewed changes

kqdeng added the ready-to-merge Signal to Mergify to merge the PR. label Nov 19, 2020

mergify bot merged commit a2d22a0 into tinkerbell:master Nov 19, 2020

mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hardware not found response #365

Fix hardware not found response #365

kqdeng commented Nov 13, 2020 •

edited

Loading

codecov bot commented Nov 13, 2020 •

edited

Loading

mmlb Nov 13, 2020

kqdeng Nov 13, 2020

mmlb Nov 13, 2020 •

edited

Loading

mmlb Nov 13, 2020

kqdeng Nov 13, 2020

mmlb Nov 13, 2020

mmlb Nov 16, 2020

mmlb Nov 16, 2020

kqdeng Nov 16, 2020 •

edited

Loading

mmlb Nov 16, 2020

kqdeng Nov 16, 2020

kqdeng Nov 16, 2020 •

edited

Loading

mmlb Nov 18, 2020

mmlb left a comment

mmlb Nov 19, 2020

kqdeng Nov 19, 2020

Fix hardware not found response #365

Fix hardware not found response #365

Conversation

kqdeng commented Nov 13, 2020 • 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 Nov 13, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmlb Nov 13, 2020 • 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

kqdeng Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqdeng Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmlb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqdeng commented Nov 13, 2020 •

edited

Loading

codecov bot commented Nov 13, 2020 •

edited

Loading

mmlb Nov 13, 2020 •

edited

Loading

kqdeng Nov 16, 2020 •

edited

Loading

kqdeng Nov 16, 2020 •

edited

Loading