Repository: osbuild/osbuild-composer
Pull request #347 (2020-03-18 09:18:23)

store/jobqueue: remove `distro` field from jobs
A job's purpose is to build an osbuild manifest and upload the results
somewhere. It should not know about which distro was used to generate
the pipeline.

Workers depended on the distro package in two ways:

1. To set an osbuild `--build-env`. This is not necessary anymore in new
   versions of osbuild. More importantly, it was wrong: it passed the
   runner from the distro that is being built, instead of one that
   matches the host.

   This patch simply removes that logic.

2. To fetch the output filename with `Distro.FilenameFromType()`. While
   that is useful, I don't think it warrants the dependency.

   This patch uses the fact that all current pipelines output exactly
   one file and uploads that. This should probably be extended in the
   future to upload all output files, or to name them explicitly in the
   upload target.

The worker should now compile to a smaller binary and do less
unnecessary work on startup (like reading repository files).

larskarlitski: Force-puhshed to resolve conflicts.
larskarlitski: > For a follow up: I think we should pass in the filename rather than expect it to be only one. Alternatively we could make the filename predictable based on the compose ID or something like that.

Agreed. Let's discuss this here: https://github.com/osbuild/osbuild-composer/issues/348

Patch:
diff --git a/internal/jobqueue/api.go b/internal/jobqueue/api.go
index adbf80642..f68200a2b 100644
--- a/internal/jobqueue/api.go
+++ b/internal/jobqueue/api.go
@@ -105,7 +105,6 @@ func (api *API) addJobHandler(writer http.ResponseWriter, request *http.Request,
 	_ = json.NewEncoder(writer).Encode(Job{
 		ID:           nextJob.ComposeID,
 		ImageBuildID: nextJob.ImageBuildID,
-		Distro:       nextJob.Distro,
 		Manifest:     nextJob.Manifest,
 		Targets:      nextJob.Targets,
 		OutputType:   nextJob.ImageType,
diff --git a/internal/jobqueue/job.go b/internal/jobqueue/job.go
index 74f5aab94..06f7d5bbc 100644
--- a/internal/jobqueue/job.go
+++ b/internal/jobqueue/job.go
@@ -6,13 +6,13 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"path"
 
 	"github.com/osbuild/osbuild-composer/internal/osbuild"
 
 	"github.com/google/uuid"
 
 	"github.com/osbuild/osbuild-composer/internal/common"
-	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/target"
 	"github.com/osbuild/osbuild-composer/internal/upload/awsupload"
 )
@@ -20,7 +20,6 @@ import (
 type Job struct {
 	ID           uuid.UUID         `json:"id"`
 	ImageBuildID int               `json:"image_build_id"`
-	Distro       string            `json:"distro"`
 	Manifest     *osbuild.Manifest `json:"manifest"`
 	Targets      []*target.Target  `json:"targets"`
 	OutputType   string            `json:"output_type"`
@@ -46,32 +45,6 @@ func (e *TargetsError) Error() string {
 }
 
 func (job *Job) Run(uploader LocalTargetUploader) (*common.ComposeResult, error) {
-	distros, err := distro.NewDefaultRegistry([]string{"/etc/osbuild-composer", "/usr/share/osbuild-composer"})
-	if err != nil {
-		return nil, fmt.Errorf("error loading distros: %v", err)
-	}
-
-	d := distros.GetDistro(job.Distro)
-	if d == nil {
-		return nil, fmt.Errorf("unknown distro: %s", job.Distro)
-	}
-
-	build := osbuild.Build{
-		Runner: d.Runner(),
-	}
-
-	buildFile, err := ioutil.TempFile("", "osbuild-worker-build-env-*")
-	if err != nil {
-		return nil, err
-	}
-	// FIXME: how to handle errors in defer?
-	defer os.Remove(buildFile.Name())
-
-	err = json.NewEncoder(buildFile).Encode(build)
-	if err != nil {
-		return nil, fmt.Errorf("error encoding build environment: %v", err)
-	}
-
 	tmpStore, err := ioutil.TempDir("/var/tmp", "osbuild-store")
 	if err != nil {
 		return nil, fmt.Errorf("error setting up osbuild store: %v", err)
@@ -82,7 +55,6 @@ func (job *Job) Run(uploader LocalTargetUploader) (*common.ComposeResult, error)
 	cmd := exec.Command(
 		"osbuild",
 		"--store", tmpStore,
-		"--build-env", buildFile.Name(),
 		"--json", "-",
 	)
 	cmd.Stderr = os.Stderr
@@ -125,13 +97,23 @@ func (job *Job) Run(uploader LocalTargetUploader) (*common.ComposeResult, error)
 	for _, t := range job.Targets {
 		switch options := t.Options.(type) {
 		case *target.LocalTargetOptions:
-			filename, _, err := d.FilenameFromType(job.OutputType)
+			outputDir := path.Join(tmpStore, "refs", result.OutputID)
+
+			files, err := ioutil.ReadDir(outputDir)
 			if err != nil {
 				r = append(r, err)
 				continue
 			}
 
-			f, err := os.Open(tmpStore + "/refs/" + result.OutputID + "/" + filename)
+			// TODO osbuild pipelines can have multiple outputs. All the pipelines we
+			// are currently generating have exactly one, but we should support
+			// uploading all results in the future.
+			if len(files) != 1 {
+				r = append(r, fmt.Errorf("expected exactly one resulting image file"))
+				continue
+			}
+
+			f, err := os.Open(path.Join(outputDir, files[0].Name()))
 			if err != nil {
 				r = append(r, err)
 				continue
diff --git a/internal/store/store.go b/internal/store/store.go
index 2d3b440ca..47986391e 100644
--- a/internal/store/store.go
+++ b/internal/store/store.go
@@ -53,7 +53,6 @@ type Store struct {
 type Job struct {
 	ComposeID    uuid.UUID
 	ImageBuildID int
-	Distro       string
 	Manifest     *osbuild.Manifest
 	Targets      []*target.Target
 	ImageType    string
@@ -166,10 +165,6 @@ func New(stateDir *string, distroRegistryArg distro.Registry) *Store {
 					// s.Composes[composeID] = compose
 				case common.IBWaiting:
 					// Push waiting composes back into the pending jobs queue
-					distroStr, exists := imgBuild.Distro.ToString()
-					if !exists {
-						panic("fatal error, distro tag should exist but does not")
-					}
 					imageTypeCompat, exists := imgBuild.ImageType.ToCompatString()
 					if !exists {
 						panic("fatal error, image type tag should exist but does not")
@@ -177,7 +172,6 @@ func New(stateDir *string, distroRegistryArg distro.Registry) *Store {
 					s.pendingJobs <- Job{
 						ComposeID:    composeID,
 						ImageBuildID: imgBuild.Id,
-						Distro:       distroStr,
 						Manifest:     imgBuild.Manifest,
 						Targets:      imgBuild.Targets,
 						ImageType:    imageTypeCompat,
@@ -641,7 +635,6 @@ func (s *Store) PushCompose(distro distro.Distro, composeID uuid.UUID, bp *bluep
 	s.pendingJobs <- Job{
 		ComposeID:    composeID,
 		ImageBuildID: 0,
-		Distro:       distro.Name(),
 		Manifest:     manifestStruct,
 		Targets:      targets,
 		ImageType:    composeType,


Test patch:
diff --git a/internal/jobqueue/api_test.go b/internal/jobqueue/api_test.go
index 88a36338a..d9b8a68f5 100644
--- a/internal/jobqueue/api_test.go
+++ b/internal/jobqueue/api_test.go
@@ -60,7 +60,7 @@ func TestCreate(t *testing.T) {
 	}
 
 	test.TestRoute(t, api, false, "POST", "/job-queue/v1/jobs", `{}`, http.StatusCreated,
-		`{"id":"ffffffff-ffff-ffff-ffff-ffffffffffff","image_build_id":0,"distro":"fedora-30","manifest":{"sources":{},"pipeline":{}},"targets":[],"output_type":"qcow2"}`, "created", "uuid")
+		`{"id":"ffffffff-ffff-ffff-ffff-ffffffffffff","image_build_id":0,"manifest":{"sources":{},"pipeline":{}},"targets":[],"output_type":"qcow2"}`, "created", "uuid")
 }
 
 func testUpdateTransition(t *testing.T, from, to string, expectedStatus int, expectedResponse string) {