Repository: osbuild/osbuild-composer
Pull request #4221 (2024-06-20 11:46:26)

Set executor hostname via cloud-init
This way much more of the journal will be captured under the new hostname.



Patch:
diff --git a/cmd/osbuild-worker-executor/handler_build.go b/cmd/osbuild-worker-executor/handler_build.go
index 13edd9ab7..eed9b740d 100644
--- a/cmd/osbuild-worker-executor/handler_build.go
+++ b/cmd/osbuild-worker-executor/handler_build.go
@@ -13,7 +13,6 @@ import (
 	"strings"
 
 	"golang.org/x/exp/slices"
-	"golang.org/x/sys/unix"
 
 	"github.com/sirupsen/logrus"
 )
@@ -109,7 +108,6 @@ func runOsbuild(logger *logrus.Logger, buildDir string, control *controlJSON, ou
 type controlJSON struct {
 	Environments []string `json:"environments"`
 	Exports      []string `json:"exports"`
-	JobID        string   `json:"job-id"`
 }
 
 func mustRead(atar *tar.Reader, name string) error {
@@ -237,15 +235,6 @@ func handleIncludedSources(atar *tar.Reader, buildDir string) error {
 	}
 }
 
-var unixSethostname = unix.Sethostname
-
-func setHostname(name string) error {
-	if name == "" {
-		return nil
-	}
-	return unixSethostname([]byte(name))
-}
-
 // test for real via:
 // curl -o - --data-binary "@./test.tar" -H "Content-Type: application/x-tar"  -X POST http://localhost:8001/api/v1/build
 func handleBuild(logger *logrus.Logger, config *Config) http.Handler {
@@ -273,11 +262,6 @@ func handleBuild(logger *logrus.Logger, config *Config) http.Handler {
 				http.Error(w, "cannot decode control.json", http.StatusBadRequest)
 				return
 			}
-			if err := setHostname(control.JobID); err != nil {
-				logger.Error(err)
-				http.Error(w, "cannot set hostname", http.StatusBadRequest)
-				return
-			}
 
 			buildDir, err := createBuildDir(config)
 			if err != nil {
diff --git a/cmd/osbuild-worker/jobimpl-osbuild.go b/cmd/osbuild-worker/jobimpl-osbuild.go
index 98945de0b..eca86fb5b 100644
--- a/cmd/osbuild-worker/jobimpl-osbuild.go
+++ b/cmd/osbuild-worker/jobimpl-osbuild.go
@@ -515,7 +515,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 			return err
 		}
 		defer os.RemoveAll(tmpDir)
-		executor = osbuildexecutor.NewAWSEC2Executor(impl.OSBuildExecutor.IAMProfile, impl.OSBuildExecutor.KeyName, impl.OSBuildExecutor.CloudWatchGroup, tmpDir)
+		executor = osbuildexecutor.NewAWSEC2Executor(impl.OSBuildExecutor.IAMProfile, impl.OSBuildExecutor.KeyName, impl.OSBuildExecutor.CloudWatchGroup, job.Id().String(), tmpDir)
 	default:
 		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorInvalidConfig, "No osbuild executor defined", nil)
 		return err
@@ -533,7 +533,6 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 		ExportPaths: exportPaths,
 		ExtraEnv:    extraEnv,
 		Result:      true,
-		JobID:       job.Id().String(),
 	}
 	osbuildJobResult.OSBuildOutput, err = executor.RunOSBuild(jobArgs.Manifest, opts, os.Stderr)
 	// First handle the case when "running" osbuild failed
diff --git a/internal/cloud/awscloud/secure-instance.go b/internal/cloud/awscloud/secure-instance.go
index 28c150eec..7f2db55d2 100644
--- a/internal/cloud/awscloud/secure-instance.go
+++ b/internal/cloud/awscloud/secure-instance.go
@@ -19,14 +19,15 @@ type SecureInstance struct {
 }
 
 // SecureInstanceUserData returns the cloud-init user data for a secure instance.
-func SecureInstanceUserData(CloudWatchGroup string) string {
+func SecureInstanceUserData(cloudWatchGroup, hostname string) string {
 	additionalFiles := ""
 
-	if CloudWatchGroup != "" {
+	if cloudWatchGroup != "" {
 		additionalFiles += fmt.Sprintf(`  - path: /tmp/cloud_init_vars
     content: |
       OSBUILD_EXECUTOR_CLOUDWATCH_GROUP='%s'
-`, CloudWatchGroup)
+      OSBUILD_EXECUTOR_HOSTNAME='%s'
+`, cloudWatchGroup, hostname)
 	}
 
 	return fmt.Sprintf(`#cloud-config
@@ -38,7 +39,7 @@ write_files:
 
 // Runs an instance with a security group that only allows traffic to
 // the host. Will replace resources if they already exists.
-func (a *AWS) RunSecureInstance(iamProfile, keyName, CloudWatchGroup string) (*SecureInstance, error) {
+func (a *AWS) RunSecureInstance(iamProfile, keyName, cloudWatchGroup, hostname string) (*SecureInstance, error) {
 	identity, err := a.ec2metadata.GetInstanceIdentityDocument()
 	if err != nil {
 		logrus.Errorf("Error getting the identity document, %s", err)
@@ -79,7 +80,7 @@ func (a *AWS) RunSecureInstance(iamProfile, keyName, CloudWatchGroup string) (*S
 		return nil, err
 	}
 
-	ltID, err := a.createOrReplaceLT(identity.InstanceID, imageID, sgID, instanceType, iamProfile, keyName, CloudWatchGroup)
+	ltID, err := a.createOrReplaceLT(identity.InstanceID, imageID, sgID, instanceType, iamProfile, keyName, cloudWatchGroup, hostname)
 	if ltID != "" {
 		secureInstance.LTID = ltID
 	}
@@ -104,7 +105,7 @@ func (a *AWS) RunSecureInstance(iamProfile, keyName, CloudWatchGroup string) (*S
 		return nil, fmt.Errorf("Expected at least 1 subnet in the VPC, got 0")
 	}
 
-	createFleetOutput, err := a.ec2.CreateFleet(&ec2.CreateFleetInput{
+	createFleetOutput, err := a.createFleet(&ec2.CreateFleetInput{
 		LaunchTemplateConfigs: []*ec2.FleetLaunchTemplateConfigRequest{
 			&ec2.FleetLaunchTemplateConfigRequest{
 				LaunchTemplateSpecification: &ec2.FleetLaunchTemplateSpecificationRequest{
@@ -141,22 +142,8 @@ func (a *AWS) RunSecureInstance(iamProfile, keyName, CloudWatchGroup string) (*S
 	if err != nil {
 		return nil, err
 	}
-	if len(createFleetOutput.Errors) > 0 {
-		fleetErrs := []string{}
-		for _, fleetErr := range createFleetOutput.Errors {
-			fleetErrs = append(fleetErrs, *fleetErr.ErrorMessage)
-		}
-		return nil, fmt.Errorf("Unable to create fleet: %v", strings.Join(fleetErrs, "; "))
-	}
 	secureInstance.FleetID = *createFleetOutput.FleetId
 
-	if len(createFleetOutput.Instances) != 1 {
-		return nil, fmt.Errorf("Unable to create fleet with exactly one instance, got %d instances", len(createFleetOutput.Instances))
-	}
-	if len(createFleetOutput.Instances[0].InstanceIds) != 1 {
-		return nil, fmt.Errorf("Expected exactly one instance ID on fleet %v, got %d", secureInstance.FleetID, len(createFleetOutput.Instances[0].InstanceIds))
-	}
-
 	instanceID := createFleetOutput.Instances[0].InstanceIds[0]
 	err = a.ec2.WaitUntilInstanceStatusOk(&ec2.DescribeInstanceStatusInput{
 		InstanceIds: []*string{
@@ -294,7 +281,7 @@ func isLaunchTemplateNotFoundError(err error) bool {
 
 }
 
-func (a *AWS) createOrReplaceLT(hostInstanceID, imageID, sgID, instanceType, iamProfile, keyName, CloudWatchGroup string) (string, error) {
+func (a *AWS) createOrReplaceLT(hostInstanceID, imageID, sgID, instanceType, iamProfile, keyName, cloudWatchGroup, hostname string) (string, error) {
 	ltName := fmt.Sprintf("launch-template-for-%s-runner-instance", hostInstanceID)
 	descrLTOutput, err := a.ec2.DescribeLaunchTemplates(&ec2.DescribeLaunchTemplatesInput{
 		LaunchTemplateNames: []*string{
@@ -347,7 +334,7 @@ func (a *AWS) createOrReplaceLT(hostInstanceID, imageID, sgID, instanceType, iam
 			SecurityGroupIds: []*string{
 				aws.String(sgID),
 			},
-			UserData: aws.String(base64.StdEncoding.EncodeToString([]byte(SecureInstanceUserData(CloudWatchGroup)))),
+			UserData: aws.String(base64.StdEncoding.EncodeToString([]byte(SecureInstanceUserData(cloudWatchGroup, hostname)))),
 		},
 		TagSpecifications: []*ec2.TagSpecification{
 			&ec2.TagSpecification{
@@ -436,3 +423,35 @@ func (a *AWS) deleteSGIfExists(si *SecureInstance) error {
 	}
 	return err
 }
+
+func (a *AWS) createFleet(input *ec2.CreateFleetInput) (*ec2.CreateFleetOutput, error) {
+	createFleetOutput, err := a.ec2.CreateFleet(input)
+	if err != nil {
+		return nil, fmt.Errorf("Unable to create spot fleet: %w", err)
+	}
+
+	if len(createFleetOutput.Errors) > 0 && createFleetOutput.Errors[0].ErrorCode == aws.String("UnfillableCapacity") {
+		logrus.Warn("Received UnfillableCapacity from CreateFleet, retrying CreateFleet with OnDemand instance")
+		input.SpotOptions = nil
+		createFleetOutput, err = a.ec2.CreateFleet(input)
+	}
+	if err != nil {
+		return nil, fmt.Errorf("Unable to create on-demand fleet: %w", err)
+	}
+
+	if len(createFleetOutput.Errors) > 0 {
+		fleetErrs := []string{}
+		for _, fleetErr := range createFleetOutput.Errors {
+			fleetErrs = append(fleetErrs, *fleetErr.ErrorMessage)
+		}
+		return nil, fmt.Errorf("Unable to create fleet: %v", strings.Join(fleetErrs, "; "))
+	}
+
+	if len(createFleetOutput.Instances) != 1 {
+		return nil, fmt.Errorf("Unable to create fleet with exactly one instance, got %d instances", len(createFleetOutput.Instances))
+	}
+	if len(createFleetOutput.Instances[0].InstanceIds) != 1 {
+		return nil, fmt.Errorf("Expected exactly one instance ID on fleet, got %d", len(createFleetOutput.Instances[0].InstanceIds))
+	}
+	return createFleetOutput, nil
+}
diff --git a/internal/osbuildexecutor/osbuild-executor.go b/internal/osbuildexecutor/osbuild-executor.go
index 9748d1e42..ed3c35eae 100644
--- a/internal/osbuildexecutor/osbuild-executor.go
+++ b/internal/osbuildexecutor/osbuild-executor.go
@@ -14,9 +14,6 @@ type OsbuildOpts struct {
 	Checkpoints []string
 	ExtraEnv    []string
 	Result      bool
-
-	// not strict a osbuild opt
-	JobID string
 }
 
 type Executor interface {
diff --git a/internal/osbuildexecutor/runner-impl-aws-ec2.go b/internal/osbuildexecutor/runner-impl-aws-ec2.go
index d49beb7a9..012947c63 100644
--- a/internal/osbuildexecutor/runner-impl-aws-ec2.go
+++ b/internal/osbuildexecutor/runner-impl-aws-ec2.go
@@ -25,6 +25,7 @@ type awsEC2Executor struct {
 	iamProfile      string
 	keyName         string
 	cloudWatchGroup string
+	hostname        string
 	tmpDir          string
 }
 
@@ -73,17 +74,15 @@ func waitForSI(ctx context.Context, host string) bool {
 	}
 }
 
-func writeInputArchive(cacheDir, store, jobID string, exports []string, manifestData []byte) (string, error) {
+func writeInputArchive(cacheDir, store string, exports []string, manifestData []byte) (string, error) {
 	archive := filepath.Join(cacheDir, "input.tar")
 	control := filepath.Join(cacheDir, "control.json")
 	manifest := filepath.Join(cacheDir, "manifest.json")
 
 	controlData := struct {
 		Exports []string `json:"exports"`
-		JobID   string   `json:"job-id"`
 	}{
 		Exports: exports,
-		JobID:   jobID,
 	}
 	controlDataBytes, err := json.Marshal(controlData)
 	if err != nil {
@@ -270,7 +269,7 @@ func (ec2e *awsEC2Executor) RunOSBuild(manifest []byte, opts *OsbuildOpts, error
 		return nil, fmt.Errorf("Failed to get default aws client in %s region: %w", region, err)
 	}
 
-	si, err := aws.RunSecureInstance(ec2e.iamProfile, ec2e.keyName, ec2e.cloudWatchGroup)
+	si, err := aws.RunSecureInstance(ec2e.iamProfile, ec2e.keyName, ec2e.cloudWatchGroup, ec2e.hostname)
 	if err != nil {
 		return nil, fmt.Errorf("Unable to start secure instance: %w", err)
 	}
@@ -289,7 +288,7 @@ func (ec2e *awsEC2Executor) RunOSBuild(manifest []byte, opts *OsbuildOpts, error
 		return nil, fmt.Errorf("Timeout waiting for executor to come online")
 	}
 
-	inputArchive, err := writeInputArchive(ec2e.tmpDir, opts.StoreDir, opts.JobID, opts.Exports, manifest)
+	inputArchive, err := writeInputArchive(ec2e.tmpDir, opts.StoreDir, opts.Exports, manifest)
 	if err != nil {
 		logrus.Errorf("Unable to write input archive: %v", err)
 		return nil, err
@@ -319,11 +318,12 @@ func (ec2e *awsEC2Executor) RunOSBuild(manifest []byte, opts *OsbuildOpts, error
 	return osbuildResult, nil
 }
 
-func NewAWSEC2Executor(iamProfile, keyName, cloudWatchGroup, tmpDir string) Executor {
+func NewAWSEC2Executor(iamProfile, keyName, cloudWatchGroup, hostname, tmpDir string) Executor {
 	return &awsEC2Executor{
 		iamProfile,
 		keyName,
 		cloudWatchGroup,
+		hostname,
 		tmpDir,
 	}
 }
diff --git a/templates/packer/ansible/roles/common/files/worker-executor.service b/templates/packer/ansible/roles/common/files/worker-executor.service
index 7b1233b0d..c908a0042 100644
--- a/templates/packer/ansible/roles/common/files/worker-executor.service
+++ b/templates/packer/ansible/roles/common/files/worker-executor.service
@@ -6,6 +6,7 @@ After=cloud-final.service
 
 [Service]
 Type=oneshot
+ExecStart=/usr/local/libexec/worker-initialization-scripts/set_executor_hostname.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/worker_executor.sh
 
 [Install]
diff --git a/templates/packer/ansible/roles/common/files/worker-initialization-scripts/set_executor_hostname.sh b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/set_executor_hostname.sh
new file mode 100644
index 000000000..513a423d8
--- /dev/null
+++ b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/set_executor_hostname.sh
@@ -0,0 +1,11 @@
+#!/bin/bash
+set -euo pipefail
+source /tmp/cloud_init_vars
+
+if [[ -z "$OSBUILD_EXECUTOR_HOSTNAME" ]]; then
+    echo "OSBUILD_EXECUTOR_HOSTNAME not set, skipping."
+    exit 0
+fi
+
+echo "Setting system hostname to $OSBUILD_EXECUTOR_HOSTNAME."
+hostnamectl set-hostname "$OSBUILD_EXECUTOR_HOSTNAME"


Test patch:
diff --git a/cmd/osbuild-worker-executor/export_test.go b/cmd/osbuild-worker-executor/export_test.go
index b47976633..9698623c8 100644
--- a/cmd/osbuild-worker-executor/export_test.go
+++ b/cmd/osbuild-worker-executor/export_test.go
@@ -12,14 +12,6 @@ var (
 	HandleIncludedSources = handleIncludedSources
 )
 
-func MockUnixSethostname(new func([]byte) error) (restore func()) {
-	saved := unixSethostname
-	unixSethostname = new
-	return func() {
-		unixSethostname = saved
-	}
-}
-
 func MockOsbuildBinary(t *testing.T, new string) (restore func()) {
 	t.Helper()
 
diff --git a/cmd/osbuild-worker-executor/handler_build_test.go b/cmd/osbuild-worker-executor/handler_build_test.go
index d70f1c19f..57decab58 100644
--- a/cmd/osbuild-worker-executor/handler_build_test.go
+++ b/cmd/osbuild-worker-executor/handler_build_test.go
@@ -324,30 +324,3 @@ func TestBuildErrorHandlingTar(t *testing.T) {
 	assert.Contains(t, string(body), "cannot tar output directory:")
 	assert.Contains(t, loggerHook.LastEntry().Message, "cannot tar output directory:")
 }
-
-func TestBuildSethostname(t *testing.T) {
-	baseURL, baseBuildDir, _ := runTestServer(t)
-	endpoint := baseURL + "api/v1/build"
-
-	restore := main.MockOsbuildBinary(t, fmt.Sprintf(`#!/bin/sh -e
-# simulate output
-mkdir -p %[1]s/build/output/image
-echo "fake-build-result" > %[1]s/build/output/image/disk.img
-`, baseBuildDir))
-	t.Cleanup(restore)
-
-	var hostnameCalls []string
-	restore = main.MockUnixSethostname(func(hn []byte) error {
-		hostnameCalls = append(hostnameCalls, string(hn))
-		return nil
-	})
-	t.Cleanup(restore)
-
-	buf := makeTestPost(t, `{"exports": ["tree"], "job-id": "1234-56"}`, `{"fake": "manifest"}`)
-	rsp, err := http.Post(endpoint, "application/x-tar", buf)
-	assert.NoError(t, err)
-	defer func() { _, _ = io.ReadAll(rsp.Body) }()
-	defer rsp.Body.Close()
-
-	assert.Equal(t, []string{"1234-56"}, hostnameCalls)
-}
diff --git a/cmd/osbuild-worker-executor/main_test.go b/cmd/osbuild-worker-executor/main_test.go
index f989d24f8..e50d05add 100644
--- a/cmd/osbuild-worker-executor/main_test.go
+++ b/cmd/osbuild-worker-executor/main_test.go
@@ -3,8 +3,10 @@ package main_test
 import (
 	"context"
 	"fmt"
+	"net"
 	"net/http"
 	"os"
+	"strconv"
 	"testing"
 	"time"
 
@@ -49,16 +51,26 @@ func waitReady(ctx context.Context, timeout time.Duration, endpoint string) erro
 	}
 }
 
+// getFreePort() returns a free port.
+// This is racy but better than using a fixed one
+func getFreePort() (int, error) {
+	l, err := net.Listen("tcp", ":0")
+	if err != nil {
+		return 0, err
+	}
+	defer l.Close()
+
+	addr := l.Addr().(*net.TCPAddr)
+	return addr.Port, nil
+}
+
 func runTestServer(t *testing.T) (baseURL, buildBaseDir string, loggerHook *logrusTest.Hook) {
 	host := "localhost"
-	port := "18002"
-	buildBaseDir = t.TempDir()
-	baseURL = fmt.Sprintf("http://%s:%s/", host, port)
+	port, err := getFreePort()
+	assert.NoError(t, err, "failed to find a free port on localhost")
 
-	restore := main.MockUnixSethostname(func([]byte) error {
-		return nil
-	})
-	t.Cleanup(restore)
+	buildBaseDir = t.TempDir()
+	baseURL = fmt.Sprintf("http://%s:%d/", host, port)
 
 	ctx, cancel := context.WithCancel(context.Background())
 	t.Cleanup(cancel)
@@ -68,14 +80,14 @@ func runTestServer(t *testing.T) (baseURL, buildBaseDir string, loggerHook *logr
 
 	args := []string{
 		"-host", host,
-		"-port", port,
+		"-port", strconv.Itoa(port),
 		"-build-path", buildBaseDir,
 	}
 	go func() {
 		_ = main.Run(ctx, args, os.Getenv, logger)
 	}()
 
-	err := waitReady(ctx, defaultTimeout, baseURL)
+	err = waitReady(ctx, defaultTimeout, baseURL)
 	assert.NoError(t, err)
 
 	return baseURL, buildBaseDir, loggerHook
diff --git a/internal/cloud/awscloud/secure-instance_test.go b/internal/cloud/awscloud/secure-instance_test.go
index 0a3c9548b..c67734486 100644
--- a/internal/cloud/awscloud/secure-instance_test.go
+++ b/internal/cloud/awscloud/secure-instance_test.go
@@ -8,6 +8,7 @@ import (
 func TestSecureInstanceUserData(t *testing.T) {
 	type testCase struct {
 		CloudWatchGroup  string
+		Hostname         string
 		ExpectedUserData string
 	}
 
@@ -21,6 +22,7 @@ write_files:
 		},
 		{
 			CloudWatchGroup: "test-group",
+			Hostname:        "test-hostname",
 			ExpectedUserData: `#cloud-config
 write_files:
   - path: /tmp/worker-run-executor-service
@@ -28,13 +30,14 @@ write_files:
   - path: /tmp/cloud_init_vars
     content: |
       OSBUILD_EXECUTOR_CLOUDWATCH_GROUP='test-group'
+      OSBUILD_EXECUTOR_HOSTNAME='test-hostname'
 `,
 		},
 	}
 
 	for idx, tc := range testCases {
 		t.Run(fmt.Sprintf("Test case %d", idx), func(t *testing.T) {
-			userData := SecureInstanceUserData(tc.CloudWatchGroup)
+			userData := SecureInstanceUserData(tc.CloudWatchGroup, tc.Hostname)
 			if userData != tc.ExpectedUserData {
 				t.Errorf("Expected: %s, got: %s", tc.ExpectedUserData, userData)
 			}
diff --git a/internal/osbuildexecutor/runner-impl-aws-ec2_test.go b/internal/osbuildexecutor/runner-impl-aws-ec2_test.go
index 4261f2160..ed36d6b89 100644
--- a/internal/osbuildexecutor/runner-impl-aws-ec2_test.go
+++ b/internal/osbuildexecutor/runner-impl-aws-ec2_test.go
@@ -46,7 +46,7 @@ func TestWriteInputArchive(t *testing.T) {
 	require.NoError(t, os.WriteFile(filepath.Join(storeDir, "contents"), []byte("storedata"), 0600))
 	require.NoError(t, os.WriteFile(filepath.Join(storeSubDir, "contents"), []byte("storedata"), 0600))
 
-	archive, err := osbuildexecutor.WriteInputArchive(cacheDir, storeDir, "some-job-id", []string{"image"}, []byte("{\"version\": 2}"))
+	archive, err := osbuildexecutor.WriteInputArchive(cacheDir, storeDir, []string{"image"}, []byte("{\"version\": 2}"))
 	require.NoError(t, err)
 
 	cmd := exec.Command("tar",
@@ -64,10 +64,6 @@ func TestWriteInputArchive(t *testing.T) {
 		"store/contents",
 		"",
 	}, strings.Split(string(out), "\n"))
-
-	output, err := exec.Command("tar", "xOf", archive, "control.json").CombinedOutput()
-	require.NoError(t, err)
-	require.Equal(t, `{"exports":["image"],"job-id":"some-job-id"}`, string(output))
 }
 
 func TestHandleBuild(t *testing.T) {