Skip to content

Commit

Permalink
Fix hardware not found response (tinkerbell#365)
Browse files Browse the repository at this point in the history 
## Description

<!--- Please describe what this PR is going to change -->
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

<!--- Link to issue you have raised -->
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.com/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.com/tinkerbell/boots/packet..
...
```
(I know, sorry for the bad formatting)

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

Manually

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

<!--- Fixes a bug, unblocks installation, removes a component of the stack etc -->
<!--- Requires a DB migration script, etc. -->

N/A

## Checklist:

I have:

- [ ] updated the documentation and/or roadmap (if required)
- [ ] added unit or e2e tests
- [ ] provided instructions on how to upgrade
  • Loading branch information
@mergify
mergify[bot] authored Nov 19, 2020
2 parents 47f1e12 + 99800d5 commit a2d22a0
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 58 deletions.
13 changes: 3 additions & 10 deletions db/db.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,18 +102,11 @@ func get(ctx context.Context, db *sql.DB, query string, args ...interface{}) (st

buf := []byte{}
err := row.Scan(&buf)
if err == nil {
return string(buf), nil
}

if err != sql.ErrNoRows {
err = errors.Wrap(err, "SELECT")
if err != nil {
logger.Error(err)
} else {
err = nil
return "", errors.Wrap(err, "SELECT")
}

return "", err
return string(buf), nil
}

// buildGetCondition builds a where condition string in the format "column_name = 'field_value' AND"
Expand Down
2 changes: 1 addition & 1 deletion db/template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

query := `
Expand Down
26 changes: 17 additions & 9 deletions db/workflow.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@ func (d TinkDB) CreateWorkflow(ctx context.Context, wf Workflow, data string, id

err = insertActionList(ctx, d.instance, data, id, tx)
if err != nil {
return errors.Wrap(err, "Failed to insert in workflow_state")
return errors.Wrap(err, "failed to create workflow")

}
err = insertInWorkflow(ctx, d.instance, wf, tx)
if err != nil {
return errors.Wrap(err, "Failed to workflow")
return errors.Wrap(err, "failed to create workflow")

}
err = tx.Commit()
Expand Down Expand Up @@ -109,9 +109,7 @@ func insertActionList(ctx context.Context, db *sql.DB, yamlData string, id uuid.

workerID, err := getWorkerID(ctx, db, task.WorkerAddr)
if err != nil {
return err
} else if workerID == "" {
return fmt.Errorf("hardware mentioned with reference %s not found", task.WorkerAddr)
return errors.WithMessage(err, "unable to insert into action list")
}
workerUID, err := uuid.Parse(workerID)
if err != nil {
Expand Down Expand Up @@ -689,7 +687,11 @@ func getWorkerIDbyMac(ctx context.Context, db *sql.DB, mac string) (string, erro
data @> $1
`

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

func getWorkerIDbyIP(ctx context.Context, db *sql.DB, ip string) (string, error) {
Expand Down Expand Up @@ -733,7 +735,11 @@ func getWorkerIDbyIP(ctx context.Context, db *sql.DB, ip string) (string, error)
)
`

return get(ctx, db, query, instance, hardwareOrManagement)
id, err := get(ctx, db, query, instance, hardwareOrManagement)
if errors.Cause(err) == sql.ErrNoRows {
err = errors.WithMessage(errors.New(ip), "ip")
}
return id, err
}

func getWorkerID(ctx context.Context, db *sql.DB, addr string) (string, error) {
Expand All @@ -743,10 +749,12 @@ func getWorkerID(ctx context.Context, db *sql.DB, addr string) (string, error) {
if ip == nil || ip.To4() == nil {
return "", fmt.Errorf("invalid worker address: %s", addr)
}
return getWorkerIDbyIP(ctx, db, addr)
id, err := getWorkerIDbyIP(ctx, db, addr)
return id, errors.WithMessage(err, "no worker found")

}
return getWorkerIDbyMac(ctx, db, addr)
id, err := getWorkerIDbyMac(ctx, db, addr)
return id, errors.WithMessage(err, "no worker found")
}

func init() {
Expand Down
91 changes: 55 additions & 36 deletions grpc-server/template_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,19 @@ import (
)

const (
templateNotFoundID = "abstract-beef-beyond-meat-abominations"
templateNotFoundName = "my-awesome-mock-name"
templateNotFoundTemplate = `version: "0.1"
name: not_found_workflow
global_timeout: 600
tasks:
- name: "not found"
worker: "{{.device_1}}"
actions:
- name: "not_found"
image: not-found
timeout: 60`

templateID1 = "7cd79119-1959-44eb-8b82-bc15bad4888e"
templateName1 = "template_1"
template1 = `version: "0.1"
Expand Down Expand Up @@ -124,6 +137,9 @@ func TestGetTemplate(t *testing.T) {
)
testCases := map[string]struct {
args args
id string
name string
data string
err bool
}{
"SuccessfulTemplateGet_Name": {
Expand All @@ -135,18 +151,18 @@ func TestGetTemplate(t *testing.T) {
GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
t.Log("in get template func")

if fields["id"] == templateID1 {
return templateID1, templateName1, template1, nil
}
if fields["name"] == templateName1 {
if fields["id"] == templateID1 || fields["name"] == templateName1 {
return templateID1, templateName1, template1, nil
}
return "", "", "", errors.New("failed to get template")
return templateNotFoundID, templateNotFoundName, templateNotFoundTemplate, errors.New("failed to get template")
},
},
getRequest: &pb.GetRequest{GetBy: &pb.GetRequest_Name{Name: templateName1}},
},
err: false,
id: templateID1,
name: templateName1,
data: template1,
err: false,
},

"FailedTemplateGet_Name": {
Expand All @@ -158,18 +174,18 @@ func TestGetTemplate(t *testing.T) {
GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
t.Log("in get template func")

if fields["id"] == templateID1 {
return templateID1, templateName1, template1, nil
}
if fields["name"] == templateName1 {
if fields["id"] == templateID1 || fields["name"] == templateName1 {
return templateID1, templateName1, template1, nil
}
return "", "", "", errors.New("failed to get template")
return templateNotFoundID, templateNotFoundName, templateNotFoundTemplate, errors.New("failed to get template")
},
},
getRequest: &pb.GetRequest{GetBy: &pb.GetRequest_Name{Name: templateName2}},
},
err: true,
id: templateNotFoundID,
name: templateNotFoundName,
data: templateNotFoundTemplate,
err: true,
},

"SuccessfulTemplateGet_ID": {
Expand All @@ -181,18 +197,18 @@ func TestGetTemplate(t *testing.T) {
GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
t.Log("in get template func")

if fields["id"] == templateID1 {
return templateID1, templateName1, template1, nil
}
if fields["name"] == templateName1 {
if fields["id"] == templateID1 || fields["name"] == templateName1 {
return templateID1, templateName1, template1, nil
}
return "", "", "", errors.New("failed to get template")
return templateNotFoundID, templateNotFoundName, templateNotFoundTemplate, errors.New("failed to get template")
},
},
getRequest: &pb.GetRequest{GetBy: &pb.GetRequest_Id{Id: templateID1}},
},
err: false,
id: templateID1,
name: templateName1,
data: template1,
err: false,
},

"FailedTemplateGet_ID": {
Expand All @@ -204,18 +220,18 @@ func TestGetTemplate(t *testing.T) {
GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
t.Log("in get template func")

if fields["id"] == templateID1 {
return templateID1, templateName1, template1, nil
}
if fields["name"] == templateName1 {
if fields["id"] == templateID1 || fields["name"] == templateName1 {
return templateID1, templateName1, template1, nil
}
return "", "", "", errors.New("failed to get template")
return templateNotFoundID, templateNotFoundName, templateNotFoundTemplate, errors.New("failed to get template")
},
},
getRequest: &pb.GetRequest{GetBy: &pb.GetRequest_Id{Id: templateID2}},
},
err: true,
id: templateNotFoundID,
name: templateNotFoundName,
data: templateNotFoundTemplate,
err: true,
},

"FailedTemplateGet_EmptyRequest": {
Expand All @@ -227,18 +243,18 @@ func TestGetTemplate(t *testing.T) {
GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
t.Log("in get template func")

if fields["id"] == templateID1 {
if fields["id"] == templateID1 || fields["name"] == templateName1 {
return templateID1, templateName1, template1, nil
}
if fields["name"] == templateName1 {
return templateID1, templateName1, template1, nil
}
return "", "", "", errors.New("failed to get template")
return templateNotFoundID, templateNotFoundName, templateNotFoundTemplate, errors.New("failed to get template")
},
},
getRequest: &pb.GetRequest{},
},
err: true,
id: templateNotFoundID,
name: templateNotFoundName,
data: templateNotFoundTemplate,
err: true,
},

"FailedTemplateGet_NilRequest": {
Expand All @@ -250,17 +266,17 @@ func TestGetTemplate(t *testing.T) {
GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
t.Log("in get template func")

if fields["id"] == templateID1 {
return templateID1, templateName1, template1, nil
}
if fields["name"] == templateName1 {
if fields["id"] == templateID1 || fields["name"] == templateName1 {
return templateID1, templateName1, template1, nil
}
return "", "", "", errors.New("failed to get template")
return templateNotFoundID, templateNotFoundName, templateNotFoundTemplate, errors.New("failed to get template")
},
},
},
err: true,
id: templateNotFoundID,
name: templateNotFoundName,
data: templateNotFoundTemplate,
err: true,
},
}

Expand All @@ -278,6 +294,9 @@ func TestGetTemplate(t *testing.T) {
assert.Nil(t, err)
assert.NotEmpty(t, res)
}
assert.Equal(t, res.Id, tc.id)
assert.Equal(t, res.Name, tc.name)
assert.Equal(t, res.Data, tc.data)
})
}
}
4 changes: 2 additions & 2 deletions grpc-server/workflow.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ var state = map[int32]workflow.State{
}

const (
errFailedToGetTemplate = "failed to get template with ID: %s"
errTemplateParsing = "failed to parse template with ID: %s"
errFailedToGetTemplate = "failed to get template with ID %s"
errTemplateParsing = "failed to parse template with ID %s"
)

// CreateWorkflow implements workflow.CreateWorkflow
Expand Down

0 comments on commit a2d22a0

Please sign in to comment.