Skip to content

Commit

Permalink
Better error handling on the provider side (#122)
Browse files Browse the repository at this point in the history
* Add chmod error test

* Remove docker workflow call input

* Add docker make source deps

* Mostly just capitalizing things

* Actually handle errors

* More failure tests
  • Loading branch information
UnstoppableMango authored Aug 12, 2024
1 parent 4a43800 commit 8b6dc89
Show file tree
Hide file tree
Showing 13 changed files with 132 additions and 27 deletions.
2 changes: 0 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,6 @@ jobs:
docker:
name: Docker
uses: ./.github/workflows/docker.yml
with:
tags: baremetal-provisioner:ci

provider:
name: Provider
Expand Down
8 changes: 1 addition & 7 deletions .github/workflows/docker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,6 @@ name: Docker

on:
workflow_call:
inputs:
tags:
type: string
description: Override the tags on the image
required: false
push:
branches: [main]
tags:
Expand All @@ -24,7 +19,6 @@ jobs:
uses: docker/setup-buildx-action@v3

- name: Docker meta
if: github.event_name == 'push'
id: meta
uses: docker/metadata-action@v5
with:
Expand Down Expand Up @@ -52,7 +46,7 @@ jobs:
uses: docker/build-push-action@v6
with:
file: provider/cmd/provisioner/Dockerfile
tags: ${{ github.event_name == 'push' && steps.meta.outputs.tags || inputs.tags }}
tags: ${{ steps.meta.outputs.tags }}
labels: ${{ steps.meta.outputs.labels }}
platforms: linux/amd64,linux/arm64
push: ${{ github.event_name == 'push' }}
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ $(GO_MODULES:%=.make/tidy/%): .make/tidy/%: $(addprefix %/,go.mod go.sum)
buf lint --path $?
@touch $@

.make/provisioner_docker: provider/cmd/provisioner/Dockerfile .dockerignore
.make/provisioner_docker: provider/cmd/provisioner/Dockerfile .dockerignore $(PROVIDER_SRC)
docker build ${WORKING_DIR} -f $< -t ${PROVISIONER_NAME}:${DOCKER_TAG}
@touch $@

Expand Down
14 changes: 10 additions & 4 deletions provider/pkg/provider/cmd/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ func (s *State[T]) Create(ctx context.Context, inputs CommandArgs[T], preview bo
log := logger.FromContext(ctx)
if preview {
// Could dial the host and warn if the connection fails
log.DebugStatus("skipping during preview")
log.DebugStatus("Skipping during preview")
return nil
}

Expand All @@ -25,20 +25,26 @@ func (s *State[T]) Create(ctx context.Context, inputs CommandArgs[T], preview bo

p, err := provisioner.FromContext(ctx)
if err != nil {
log.Error("failed creating provisioner")
log.Error("Failed creating provisioner")
return fmt.Errorf("creating provisioner: %w", err)
}

log.InfoStatus("Sending create request to provisioner")
log.DebugStatus("Sending create request to provisioner")
res, err := p.Create(ctx, &pb.CreateRequest{
Command: command,
ExpectCreated: inputs.ExpectCreated(),
ExpectMoved: inputs.ExpectMoved(),
})
if err != nil {
log.Errorf("Failed provisioning: %s", err)
return fmt.Errorf("sending create request: %w", err)
}

if res.Result.ExitCode > 0 {
log.Errorf("Failed provisioning: %s", res.Result)
return fmt.Errorf("sending create request: %s", res.Result)
}

if res.CreatedFiles == nil {
log.DebugStatusf("%#v was empty, this is probably a bug somewhere else", res.CreatedFiles)
res.CreatedFiles = []string{}
Expand All @@ -56,6 +62,6 @@ func (s *State[T]) Create(ctx context.Context, inputs CommandArgs[T], preview bo
s.CreatedFiles = res.CreatedFiles
s.MovedFiles = res.MovedFiles

log.InfoStatus("Create success")
log.InfoStatusf("✅ %s", res.Result)
return nil
}
14 changes: 8 additions & 6 deletions provider/pkg/provider/cmd/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ func (s *State[T]) Delete(ctx context.Context) error {
log := logger.FromContext(ctx)
p, err := provisioner.FromContext(ctx)
if err != nil {
log.Error("failed creating provisioner")
log.Error("Failed creating provisioner")
return fmt.Errorf("creating provisioner: %w", err)
}

Expand All @@ -25,7 +25,7 @@ func (s *State[T]) Delete(ctx context.Context) error {
return fmt.Errorf("parsing custom command: %w", err)
}
} else {
log.InfoStatus("Normal delete")
log.DebugStatus("Normal delete")
}

prev, err := s.Operation()
Expand All @@ -34,32 +34,34 @@ func (s *State[T]) Delete(ctx context.Context) error {
return fmt.Errorf("failed to generate operation from state: %w", err)
}

log.InfoStatus("Sending delete request to provisioner")
log.DebugStatus("Sending delete request to provisioner")
res, err := p.Delete(ctx, &pb.DeleteRequest{
Command: command,
Previous: prev,
})
if err != nil {
log.Errorf("Failed provisioning: %s", err)
return fmt.Errorf("sending delete request: %w", err)
}

if len(res.Commands) == 0 {
log.InfoStatus("provisioner did not perform any operations")
log.DebugStatus("Provisioner did not perform any operations")
} else {
failed := []*pb.Operation{}
for _, c := range res.Commands {
exitCode := c.Result.ExitCode
if exitCode != 0 {
log.Errorf("provisioner returned non-zero exit code: %d", exitCode)
log.Errorf("Provisioner returned non-zero exit code: %d", exitCode)
failed = append(failed, c)
}
}

if len(failed) > 0 {
log.ErrorStatusf("A delete operation failed: %s", failed)
return fmt.Errorf("a delete operation failed: %s", failed)
}
}

log.InfoStatus("Delete success")
log.InfoStatusf("✅ %s", res)
return nil
}
19 changes: 12 additions & 7 deletions provider/pkg/provider/cmd/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ func (s *State[T]) Update(ctx context.Context, inputs CommandArgs[T], preview bo
log := logger.FromContext(ctx)
if preview {
// Could dial the host and warn if the connection fails
log.DebugStatus("skipping during preview")
log.DebugStatus("Skipping during preview")
return s.Copy(), nil
}

p, err := provisioner.FromContext(ctx)
if err != nil {
log.Error("failed creating provisioner")
log.Error("Failed creating provisioner")
return s.Copy(), fmt.Errorf("creating provisioner: %w", err)
}

Expand All @@ -30,14 +30,14 @@ func (s *State[T]) Update(ctx context.Context, inputs CommandArgs[T], preview bo
if len(inputs.CustomUpdate) > 0 {
command, err = parseCommand(inputs.CustomUpdate)
if err != nil {
log.Errorf("Failed to parse custom update: %s", err)
log.Errorf("Failed parsing custom update: %s", err)
return s.Copy(), fmt.Errorf("parsing custom command: %w", err)
}
} else {
command, err = inputs.Cmd()
if err != nil {
log.Errorf("Failed to build command from inputs: %s", err)
return s.Copy(), fmt.Errorf("failed to build command from inputs: %w", err)
log.Errorf("Failed building command from inputs: %s", err)
return s.Copy(), fmt.Errorf("failed building command from inputs: %w", err)
}

expectCreated = inputs.ExpectCreated()
Expand All @@ -58,10 +58,15 @@ func (s *State[T]) Update(ctx context.Context, inputs CommandArgs[T], preview bo
Previous: prev,
})
if err != nil {
log.Errorf("Failed sending update request: %s", err)
log.Errorf("Failed provisioning: %s", err)
return s.Copy(), fmt.Errorf("sending update request: %w", err)
}

if res.Result.ExitCode > 0 {
log.Errorf("Failed provisioning: %s", res.Result)
return s.Copy(), fmt.Errorf("sending update request: %s", res.Result)
}

if res.CreatedFiles == nil {
log.DebugStatusf("%#v was empty, this is probably a bug somewhere else", res.CreatedFiles)
res.CreatedFiles = []string{}
Expand All @@ -72,7 +77,7 @@ func (s *State[T]) Update(ctx context.Context, inputs CommandArgs[T], preview bo
res.MovedFiles = map[string]string{}
}

log.InfoStatus("Update success")
log.InfoStatusf("✅ %s", res.Result)
return State[T]{
CommandArgs: inputs,
ExitCode: int(res.Result.ExitCode),
Expand Down
15 changes: 15 additions & 0 deletions tests/chmod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,19 @@ var _ = Describe("Chmod", func() {
_, err = provisioner.Exec(ctx, "touch", "blah")
Expect(err).NotTo(HaveOccurred())
})

It("should fail when file doesn't exist", func() {
run(server, integration.LifeCycleTest{
Resource: resource,
Create: integration.Operation{
Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{
"args": map[string]interface{}{
"files": []string{"/does/not/exist"},
"octalMode": "0700",
},
}),
ExpectFailure: true,
},
})
})
})
14 changes: 14 additions & 0 deletions tests/mkdir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,18 @@ var _ = Describe("Mkdir", func() {
_, err := provisioner.Exec(ctx, "touch", "blah")
Expect(err).NotTo(HaveOccurred())
})

It("should fail when path doesn't exist", func() {
run(server, integration.LifeCycleTest{
Resource: resource,
Create: integration.Operation{
Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{
"args": map[string]interface{}{
"directory": []string{"/does/not/exist"},
},
}),
ExpectFailure: true,
},
})
})
})
14 changes: 14 additions & 0 deletions tests/mktemp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,18 @@ var _ = Describe("Mktemp", func() {
_, err := provisioner.Exec(ctx, "touch", "blah")
Expect(err).NotTo(HaveOccurred())
})

It("should fail when template is invalid", func() {
run(server, integration.LifeCycleTest{
Resource: resource,
Create: integration.Operation{
Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{
"args": map[string]interface{}{
"template": "does-not-have-enough-x",
},
}),
ExpectFailure: true,
},
})
})
})
14 changes: 14 additions & 0 deletions tests/mv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,18 @@ var _ = Describe("Mv", func() {
Expect(provisioner).NotTo(ContainFile(ctx, dest))
Expect(provisioner).To(ContainFile(ctx, final))
})

It("should fail when source doesn't exist", func() {
run(server, integration.LifeCycleTest{
Resource: resource,
Create: integration.Operation{
Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{
"args": map[string]interface{}{
"source": []string{"/does/not/exist"},
},
}),
ExpectFailure: true,
},
})
})
})
14 changes: 14 additions & 0 deletions tests/rm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,18 @@ var _ = Describe("Rm", func() {

Expect(provisioner).NotTo(ContainFile(ctx, file))
})

It("should fail when file doesn't exist", func() {
run(server, integration.LifeCycleTest{
Resource: resource,
Create: integration.Operation{
Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{
"args": map[string]interface{}{
"files": []string{"/does/not/exist"},
},
}),
ExpectFailure: true,
},
})
})
})
14 changes: 14 additions & 0 deletions tests/tar_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,18 @@ var _ = Describe("Tar", func() {
Expect(provisioner).To(ContainFile(ctx, archive))
Expect(provisioner).NotTo(ContainFile(ctx, expectedFile))
})

It("should fail when archive doesn't exist", func() {
run(server, integration.LifeCycleTest{
Resource: resource,
Create: integration.Operation{
Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{
"args": map[string]interface{}{
"file": "/does/not/exist",
},
}),
ExpectFailure: true,
},
})
})
})
15 changes: 15 additions & 0 deletions tests/tee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,4 +228,19 @@ var _ = Describe("Tee", func() {
Expect(provisioner).NotTo(ContainFile(ctx, newFile))
Expect(provisioner).NotTo(ContainFile(ctx, file))
})

It("should fail when file doesn't exist", func() {
run(server, integration.LifeCycleTest{
Resource: resource,
Create: integration.Operation{
Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{
"args": map[string]interface{}{
"stdin": "does not matter for this test",
"files": []string{"/does/not/exist"},
},
}),
ExpectFailure: true,
},
})
})
})

0 comments on commit 8b6dc89

Please sign in to comment.