From bf86e8ad7957acaa99015fa2f5b694ff7fa2ab80 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Thu, 12 Nov 2020 19:31:24 +0000 Subject: [PATCH] workerapi: serialize koji errors as strings Serializing an interface does not work, let us simply use the string representation and treat the empty string as no error. This is compatible with the current API in the success case, and fixes the error case, which is currently broken. Also extend the test matrix for the kojiapi to ensure that all the different kinds of errors can be serialized correctly and leads to the correct status being returned. Fixes #1079 and #1080. --- cmd/osbuild-worker/jobimpl-koji-finalize.go | 14 +- cmd/osbuild-worker/jobimpl-koji-init.go | 5 +- cmd/osbuild-worker/jobimpl-osbuild-koji.go | 7 +- internal/kojiapi/server.go | 12 +- internal/kojiapi/server_test.go | 394 ++++++++++++++------ internal/worker/json.go | 6 +- 6 files changed, 299 insertions(+), 139 deletions(-) diff --git a/cmd/osbuild-worker/jobimpl-koji-finalize.go b/cmd/osbuild-worker/jobimpl-koji-finalize.go index ae7099975..7479aabc9 100644 --- a/cmd/osbuild-worker/jobimpl-koji-finalize.go +++ b/cmd/osbuild-worker/jobimpl-koji-finalize.go @@ -105,7 +105,7 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error { return err } - if initArgs.KojiError != nil { + if initArgs.KojiError != "" { failure = true } @@ -127,7 +127,7 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error { if err != nil { return err } - if !buildArgs.OSBuildOutput.Success || buildArgs.KojiError != nil { + if !buildArgs.OSBuildOutput.Success || buildArgs.KojiError != "" { failure = true break } @@ -167,9 +167,15 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error { var result worker.KojiFinalizeJobResult if failure { - result.KojiError = impl.kojiFail(args.Server, int(initArgs.BuildID), initArgs.Token) + err = impl.kojiFail(args.Server, int(initArgs.BuildID), initArgs.Token) + if err != nil { + result.KojiError = err.Error() + } } else { - result.KojiError = impl.kojiImport(args.Server, build, buildRoots, images, args.KojiDirectory, initArgs.Token) + err = impl.kojiImport(args.Server, build, buildRoots, images, args.KojiDirectory, initArgs.Token) + if err != nil { + result.KojiError = err.Error() + } } err = job.Update(&result) diff --git a/cmd/osbuild-worker/jobimpl-koji-init.go b/cmd/osbuild-worker/jobimpl-koji-init.go index ca1970db1..21d54b506 100644 --- a/cmd/osbuild-worker/jobimpl-koji-init.go +++ b/cmd/osbuild-worker/jobimpl-koji-init.go @@ -60,7 +60,10 @@ func (impl *KojiInitJobImpl) Run(job worker.Job) error { } var result worker.KojiInitJobResult - result.Token, result.BuildID, result.KojiError = impl.kojiInit(args.Server, args.Name, args.Version, args.Release) + result.Token, result.BuildID, err = impl.kojiInit(args.Server, args.Name, args.Version, args.Release) + if err != nil { + result.KojiError = err.Error() + } err = job.Update(&result) if err != nil { diff --git a/cmd/osbuild-worker/jobimpl-osbuild-koji.go b/cmd/osbuild-worker/jobimpl-osbuild-koji.go index 0087a2ddc..f7d2d26fb 100644 --- a/cmd/osbuild-worker/jobimpl-osbuild-koji.go +++ b/cmd/osbuild-worker/jobimpl-osbuild-koji.go @@ -84,7 +84,7 @@ func (impl *OSBuildKojiJobImpl) Run(job worker.Job) error { return err } - if initArgs.KojiError == nil { + if initArgs.KojiError == "" { result.OSBuildOutput, err = RunOSBuild(args.Manifest, impl.Store, outputDirectory, os.Stderr) if err != nil { return err @@ -95,7 +95,10 @@ func (impl *OSBuildKojiJobImpl) Run(job worker.Job) error { if err != nil { return err } - result.ImageHash, result.ImageSize, result.KojiError = impl.kojiUpload(f, args.KojiServer, args.KojiDirectory, args.KojiFilename) + result.ImageHash, result.ImageSize, err = impl.kojiUpload(f, args.KojiServer, args.KojiDirectory, args.KojiFilename) + if err != nil { + result.KojiError = err.Error() + } } } diff --git a/internal/kojiapi/server.go b/internal/kojiapi/server.go index 09e928bd1..76b6eb336 100644 --- a/internal/kojiapi/server.go +++ b/internal/kojiapi/server.go @@ -190,7 +190,7 @@ func (h *apiHandlers) PostCompose(ctx echo.Context) error { } time.Sleep(500 * time.Millisecond) } - if initResult.KojiError != nil { + if initResult.KojiError != "" { return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Could not initialize build with koji: %v", initResult.KojiError)) } @@ -224,7 +224,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni return "failure" } - if initResult.KojiError != nil { + if initResult.KojiError != "" { return "failure" } @@ -232,7 +232,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni if buildResult.OSBuildOutput != nil && !buildResult.OSBuildOutput.Success { return "failure" } - if buildResult.KojiError != nil { + if buildResult.KojiError != "" { return "failure" } } @@ -241,7 +241,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni return "pending" } - if result.KojiError == nil { + if result.KojiError == "" { return "success" } @@ -253,7 +253,7 @@ func imageStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiInitJ return "failure" } - if initResult.KojiError != nil { + if initResult.KojiError != "" { return "failure" } @@ -265,7 +265,7 @@ func imageStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiInitJ return "building" } - if buildResult.OSBuildOutput != nil && buildResult.OSBuildOutput.Success && buildResult.KojiError == nil { + if buildResult.OSBuildOutput != nil && buildResult.OSBuildOutput.Success && buildResult.KojiError == "" { return "success" } diff --git a/internal/kojiapi/server_test.go b/internal/kojiapi/server_test.go index f240e9507..c57b44c01 100644 --- a/internal/kojiapi/server_test.go +++ b/internal/kojiapi/server_test.go @@ -17,6 +17,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/kojiapi/api" distro_mock "github.com/osbuild/osbuild-composer/internal/mocks/distro" rpmmd_mock "github.com/osbuild/osbuild-composer/internal/mocks/rpmmd" + "github.com/osbuild/osbuild-composer/internal/osbuild" "github.com/osbuild/osbuild-composer/internal/test" "github.com/osbuild/osbuild-composer/internal/worker" "github.com/stretchr/testify/require" @@ -55,6 +56,10 @@ func TestStatus(t *testing.T) { test.TestRoute(t, handler, false, "GET", "/api/composer-koji/v1/status", ``, http.StatusOK, `{"status":"OK"}`, "message") } +type jobResult struct { + Result interface{} `json:"result"` +} + func TestCompose(t *testing.T) { dir, err := ioutil.TempDir("", "osbuild-composer-test-kojiapi-") if err != nil { @@ -64,139 +69,282 @@ func TestCompose(t *testing.T) { kojiServer, workerServer := newTestKojiServer(t, dir) handler := kojiServer.Handler("/api/composer-koji/v1") - var wg sync.WaitGroup - wg.Add(1) - go func() { - token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-init"}) - require.NoError(t, err) - require.Equal(t, "koji-init", jobType) + type kojiCase struct { + initResult worker.KojiInitJobResult + buildResult worker.OSBuildKojiJobResult + finalizeResult worker.KojiFinalizeJobResult + composeReplyCode int + composeReply string + composeStatus string + } - var initJob worker.KojiInitJob - err = json.Unmarshal(rawJob, &initJob) + var cases = []kojiCase{ + { + initResult: worker.KojiInitJobResult{ + BuildID: 42, + Token: `"foobar"`, + }, + buildResult: worker.OSBuildKojiJobResult{ + Arch: "x86_64", + HostOS: "fedora-30", + ImageHash: "browns", + ImageSize: 42, + OSBuildOutput: &osbuild.Result{ + Success: true, + }, + }, + composeReplyCode: http.StatusCreated, + composeReply: `{"koji_build_id":42}`, + composeStatus: `{ + "image_statuses": [ + { + "status": "success" + }, + { + "status": "success" + } + ], + "koji_build_id": 42, + "koji_task_id": 0, + "status": "success" + }`, + }, + { + initResult: worker.KojiInitJobResult{ + KojiError: "failure", + }, + buildResult: worker.OSBuildKojiJobResult{ + Arch: "x86_64", + HostOS: "fedora-30", + ImageHash: "browns", + ImageSize: 42, + OSBuildOutput: &osbuild.Result{ + Success: true, + }, + }, + composeReplyCode: http.StatusBadRequest, + composeReply: `{"message":"Could not initialize build with koji: failure"}`, + composeStatus: `{ + "image_statuses": [ + { + "status": "failure" + }, + { + "status": "failure" + } + ], + "koji_task_id": 0, + "status": "failure" + }`, + }, + { + initResult: worker.KojiInitJobResult{ + BuildID: 42, + Token: `"foobar"`, + }, + buildResult: worker.OSBuildKojiJobResult{ + Arch: "x86_64", + HostOS: "fedora-30", + ImageHash: "browns", + ImageSize: 42, + OSBuildOutput: &osbuild.Result{ + Success: false, + }, + }, + composeReplyCode: http.StatusCreated, + composeReply: `{"koji_build_id":42}`, + composeStatus: `{ + "image_statuses": [ + { + "status": "failure" + }, + { + "status": "success" + } + ], + "koji_build_id": 42, + "koji_task_id": 0, + "status": "failure" + }`, + }, + { + initResult: worker.KojiInitJobResult{ + BuildID: 42, + Token: `"foobar"`, + }, + buildResult: worker.OSBuildKojiJobResult{ + Arch: "x86_64", + HostOS: "fedora-30", + ImageHash: "browns", + ImageSize: 42, + OSBuildOutput: &osbuild.Result{ + Success: true, + }, + KojiError: "failure", + }, + composeReplyCode: http.StatusCreated, + composeReply: `{"koji_build_id":42}`, + composeStatus: `{ + "image_statuses": [ + { + "status": "failure" + }, + { + "status": "success" + } + ], + "koji_build_id": 42, + "koji_task_id": 0, + "status": "failure" + }`, + }, + { + initResult: worker.KojiInitJobResult{ + BuildID: 42, + Token: `"foobar"`, + }, + buildResult: worker.OSBuildKojiJobResult{ + Arch: "x86_64", + HostOS: "fedora-30", + ImageHash: "browns", + ImageSize: 42, + OSBuildOutput: &osbuild.Result{ + Success: true, + }, + }, + finalizeResult: worker.KojiFinalizeJobResult{ + KojiError: "failure", + }, + composeReplyCode: http.StatusCreated, + composeReply: `{"koji_build_id":42}`, + composeStatus: `{ + "image_statuses": [ + { + "status": "success" + }, + { + "status": "success" + } + ], + "koji_build_id": 42, + "koji_task_id": 0, + "status": "failure" + }`, + }, + } + for _, c := range cases { + var wg sync.WaitGroup + wg.Add(1) + + go func(t *testing.T, result worker.KojiInitJobResult) { + token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-init"}) + require.NoError(t, err) + require.Equal(t, "koji-init", jobType) + + var initJob worker.KojiInitJob + err = json.Unmarshal(rawJob, &initJob) + require.NoError(t, err) + require.Equal(t, "koji.example.com", initJob.Server) + require.Equal(t, "foo", initJob.Name) + require.Equal(t, "1", initJob.Version) + require.Equal(t, "2", initJob.Release) + + initJobResult, err := json.Marshal(&jobResult{Result: result}) + require.NoError(t, err) + test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(initJobResult), http.StatusOK, `{}`) + + wg.Done() + }(t, c.initResult) + + test.TestRoute(t, handler, false, "POST", "/api/composer-koji/v1/compose", ` + { + "name":"foo", + "version":"1", + "release":"2", + "distribution":"fedora-30", + "image_requests": [ + { + "architecture": "x86_64", + "image_type": "qcow2", + "repositories": [ + { + "baseurl": "https://repo.example.com/" + } + ] + }, + { + "architecture": "x86_64", + "image_type": "qcow2", + "repositories": [ + { + "baseurl": "https://repo.example.com/" + } + ] + } + ], + "koji": { + "server": "koji.example.com" + } + }`, c.composeReplyCode, c.composeReply, "id") + wg.Wait() + + token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"}) require.NoError(t, err) - require.Equal(t, "koji.example.com", initJob.Server) - require.Equal(t, "foo", initJob.Name) - require.Equal(t, "1", initJob.Version) - require.Equal(t, "2", initJob.Release) + require.Equal(t, "osbuild-koji", jobType) + + var osbuildJob worker.OSBuildKojiJob + err = json.Unmarshal(rawJob, &osbuildJob) + require.NoError(t, err) + require.Equal(t, "koji.example.com", osbuildJob.KojiServer) + require.Equal(t, "test.img", osbuildJob.ImageName) + require.NotEmpty(t, osbuildJob.KojiDirectory) + + buildJobResult, err := json.Marshal(&jobResult{Result: c.buildResult}) + require.NoError(t, err) + test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(buildJobResult), http.StatusOK, `{}`) + + token, _, jobType, rawJob, _, err = workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"}) + require.NoError(t, err) + require.Equal(t, "osbuild-koji", jobType) + + err = json.Unmarshal(rawJob, &osbuildJob) + require.NoError(t, err) + require.Equal(t, "koji.example.com", osbuildJob.KojiServer) + require.Equal(t, "test.img", osbuildJob.ImageName) + require.NotEmpty(t, osbuildJob.KojiDirectory) test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{ - "result": { - "build_id": 42, - "token": "foobar" - } - }`, http.StatusOK, `{}`) - wg.Done() - }() - - test.TestRoute(t, handler, false, "POST", "/api/composer-koji/v1/compose", ` - { - "name":"foo", - "version":"1", - "release":"2", - "distribution":"fedora-30", - "image_requests": [ - { - "architecture": "x86_64", - "image_type": "qcow2", - "repositories": [ - { - "baseurl": "https://repo.example.com/" - } - ] - }, - { - "architecture": "x86_64", - "image_type": "qcow2", - "repositories": [ - { - "baseurl": "https://repo.example.com/" - } - ] + "result": { + "arch": "x86_64", + "host_os": "fedora-30", + "image_hash": "browns", + "image_size": 42, + "osbuild_output": { + "success": true + } } - ], - "koji": { - "server": "koji.example.com" - } - }`, http.StatusCreated, `{"koji_build_id":42}`, "id") - wg.Wait() + }`, http.StatusOK, `{}`) - token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"}) - require.NoError(t, err) - require.Equal(t, "osbuild-koji", jobType) + token, finalizeID, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-finalize"}) + require.NoError(t, err) + require.Equal(t, "koji-finalize", jobType) - var osbuildJob worker.OSBuildKojiJob - err = json.Unmarshal(rawJob, &osbuildJob) - require.NoError(t, err) - require.Equal(t, "koji.example.com", osbuildJob.KojiServer) - require.Equal(t, "test.img", osbuildJob.ImageName) - require.NotEmpty(t, osbuildJob.KojiDirectory) + var kojiFinalizeJob worker.KojiFinalizeJob + err = json.Unmarshal(rawJob, &kojiFinalizeJob) + require.NoError(t, err) + require.Equal(t, "koji.example.com", kojiFinalizeJob.Server) + require.Equal(t, "1", kojiFinalizeJob.Version) + require.Equal(t, "2", kojiFinalizeJob.Release) + require.ElementsMatch(t, []string{"foo-1-2.x86_64.img", "foo-1-2.x86_64.img"}, kojiFinalizeJob.KojiFilenames) + require.NotEmpty(t, kojiFinalizeJob.KojiDirectory) - test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{ - "result": { - "arch": "x86_64", - "host_os": "fedora-30", - "image_hash": "browns", - "image_size": 42, - "osbuild_output": { - "success": true - } - } - }`, http.StatusOK, `{}`) + finalizeResult, err := json.Marshal(&jobResult{Result: c.finalizeResult}) + require.NoError(t, err) + test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(finalizeResult), http.StatusOK, `{}`) - token, _, jobType, rawJob, _, err = workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"}) - require.NoError(t, err) - require.Equal(t, "osbuild-koji", jobType) - - err = json.Unmarshal(rawJob, &osbuildJob) - require.NoError(t, err) - require.Equal(t, "koji.example.com", osbuildJob.KojiServer) - require.Equal(t, "test.img", osbuildJob.ImageName) - require.NotEmpty(t, osbuildJob.KojiDirectory) - - test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{ - "result": { - "arch": "x86_64", - "host_os": "fedora-30", - "image_hash": "browns", - "image_size": 42, - "osbuild_output": { - "success": true - } - } - }`, http.StatusOK, `{}`) - - token, finalizeID, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-finalize"}) - require.NoError(t, err) - require.Equal(t, "koji-finalize", jobType) - - var kojiFinalizeJob worker.KojiFinalizeJob - err = json.Unmarshal(rawJob, &kojiFinalizeJob) - require.NoError(t, err) - require.Equal(t, "koji.example.com", kojiFinalizeJob.Server) - require.Equal(t, "1", kojiFinalizeJob.Version) - require.Equal(t, "2", kojiFinalizeJob.Release) - require.ElementsMatch(t, []string{"foo-1-2.x86_64.img", "foo-1-2.x86_64.img"}, kojiFinalizeJob.KojiFilenames) - require.NotEmpty(t, kojiFinalizeJob.KojiDirectory) - - test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{ - "result": { - } - }`, http.StatusOK, `{}`) - - test.TestRoute(t, handler, false, "GET", fmt.Sprintf("/api/composer-koji/v1/compose/%v", finalizeID), ``, http.StatusOK, `{ - "image_statuses": [ - { - "status": "success" - }, - { - "status": "success" - } - ], - "koji_build_id": 42, - "koji_task_id": 0, - "status": "success" - }`) + test.TestRoute(t, handler, false, "GET", fmt.Sprintf("/api/composer-koji/v1/compose/%v", finalizeID), ``, http.StatusOK, c.composeStatus) + } } func TestRequest(t *testing.T) { diff --git a/internal/worker/json.go b/internal/worker/json.go index 8828f316b..708a26fcb 100644 --- a/internal/worker/json.go +++ b/internal/worker/json.go @@ -36,7 +36,7 @@ type KojiInitJob struct { type KojiInitJobResult struct { BuildID uint64 `json:"build_id"` Token string `json:"token"` - KojiError error `json:"koji_error"` + KojiError string `json:"koji_error"` } type OSBuildKojiJob struct { @@ -53,7 +53,7 @@ type OSBuildKojiJobResult struct { OSBuildOutput *osbuild.Result `json:"osbuild_output"` ImageHash string `json:"image_hash"` ImageSize uint64 `json:"image_size"` - KojiError error `json:"koji_error"` + KojiError string `json:"koji_error"` } type KojiFinalizeJob struct { @@ -68,7 +68,7 @@ type KojiFinalizeJob struct { } type KojiFinalizeJobResult struct { - KojiError error `json:"koji_error"` + KojiError string `json:"koji_error"` } //