Repository: osbuild/osbuild-composer
Pull request #3321 (2023-03-03 13:39:49)

worker: allow configuring number of upload threads for Azure
The default number of threads (16) is OK for general use case. However, we are being asked by RH IT to lower the number of threads when uploading the image to Azure using proxy server.

Make the number of threads configurable in the worker configuration and default to the currently used value if it is not provided.


This pull request includes:

- [x] adequate testing for the new functionality or fixed issue
  - [ ] unit test for invalid value.
- [ ] adequate documentation informing people about the change such as
  - [ ] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

<!--
Thanks for proposing a change to osbuild-composer!

Please don't remove the above check list. These are things that each pull
request must have before it is merged. It helps maintainers to not forget
anything.

If the reason for ticking any of the boxes is ambiguous, please add a short
note explaining why.

In addition, if this pull request fixes a downstream issue, please refer to
test/README.md and add these additional items:

- [ ] 1st commit of any `rhbz#` related PR contains bug reproducer; CI reports FAIL or
- [ ] PR contains automated tests for new functionality and
- [ ] QE has approved reproducer/new tests and
- [ ] Subsequent commits provide bug fixes without modifying the reproducer; CI reports PASS and
- [ ] QE approves this PR; RHBZ status is set to `MODIFIED + Verified=Tested`

Information regarding our GitLab pipeline (Schutzbot):

CI will not be ran automatically if WIP label is applied or the PR is in DRAFT state, instead only
a link will be provided to the pipeline which can then be triggered manually if desired. To run the
CI automatically either switch the PR to ready or apply WIP+test label.

Outside contributors need manual approval from one of the osbuild-composer maintainers.

Schutzbot will only be triggered if all Tests jobs in GitHub workflow succeed.
-->



Patch:
diff --git a/cmd/osbuild-worker/config.go b/cmd/osbuild-worker/config.go
index b968f687f..2f0d9b76a 100644
--- a/cmd/osbuild-worker/config.go
+++ b/cmd/osbuild-worker/config.go
@@ -1,9 +1,11 @@
 package main
 
 import (
+	"fmt"
 	"os"
 
 	"github.com/BurntSushi/toml"
+	"github.com/osbuild/osbuild-composer/internal/upload/azure"
 	"github.com/sirupsen/logrus"
 )
 
@@ -27,7 +29,8 @@ type gcpConfig struct {
 }
 
 type azureConfig struct {
-	Credentials string `toml:"credentials"`
+	Credentials   string `toml:"credentials"`
+	UploadThreads int    `toml:"upload_threads"`
 }
 
 type awsConfig struct {
@@ -90,5 +93,14 @@ func parseConfig(file string) (*workerConfig, error) {
 		logrus.Info("Configuration file not found, using defaults")
 	}
 
+	// set defaults for Azure only if the config section is present
+	if config.Azure != nil {
+		if config.Azure.UploadThreads == 0 {
+			config.Azure.UploadThreads = azure.DefaultUploadThreads
+		} else if config.Azure.UploadThreads < 0 {
+			return nil, fmt.Errorf("invalid number of Azure upload threads: %d", config.Azure.UploadThreads)
+		}
+	}
+
 	return &config, nil
 }
diff --git a/cmd/osbuild-worker/jobimpl-osbuild.go b/cmd/osbuild-worker/jobimpl-osbuild.go
index 9c6124539..3114abd89 100644
--- a/cmd/osbuild-worker/jobimpl-osbuild.go
+++ b/cmd/osbuild-worker/jobimpl-osbuild.go
@@ -53,12 +53,17 @@ type ContainersConfiguration struct {
 	TLSVerify    *bool
 }
 
+type AzureConfiguration struct {
+	Creds         *azure.Credentials
+	UploadThreads int
+}
+
 type OSBuildJobImpl struct {
 	Store            string
 	Output           string
 	KojiServers      map[string]kojiServer
 	GCPConfig        GCPConfiguration
-	AzureCreds       *azure.Credentials
+	AzureConfig      AzureConfiguration
 	AWSCreds         string
 	AWSBucket        string
 	S3Config         S3Configuration
@@ -639,12 +644,12 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 			targetResult = target.NewAzureImageTargetResult(nil)
 			ctx := context.Background()
 
-			if impl.AzureCreds == nil {
+			if impl.AzureConfig.Creds == nil {
 				targetResult.TargetError = clienterrors.WorkerClientError(clienterrors.ErrorSharingTarget, "osbuild job has org.osbuild.azure.image target but this worker doesn't have azure credentials", nil)
 				break
 			}
 
-			c, err := azure.NewClient(*impl.AzureCreds, targetOptions.TenantID)
+			c, err := azure.NewClient(*impl.AzureConfig.Creds, targetOptions.TenantID)
 			if err != nil {
 				targetResult.TargetError = clienterrors.WorkerClientError(clienterrors.ErrorInvalidTargetConfig, err.Error(), nil)
 				break
@@ -738,7 +743,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 					BlobName:       blobName,
 				},
 				imagePath,
-				azure.DefaultUploadThreads,
+				impl.AzureConfig.UploadThreads,
 			)
 			if err != nil {
 				targetResult.TargetError = clienterrors.WorkerClientError(clienterrors.ErrorUploadingImage, fmt.Sprintf("uploading the image failed: %v", err), nil)
diff --git a/cmd/osbuild-worker/main.go b/cmd/osbuild-worker/main.go
index 51b989cf4..fa767db72 100644
--- a/cmd/osbuild-worker/main.go
+++ b/cmd/osbuild-worker/main.go
@@ -341,12 +341,13 @@ func main() {
 	// Load Azure credentials early. If the credentials file is malformed,
 	// we can report the issue early instead of waiting for the first osbuild
 	// job with the org.osbuild.azure.image target.
-	var azureCredentials *azure.Credentials
+	var azureConfig AzureConfiguration
 	if config.Azure != nil {
-		azureCredentials, err = azure.ParseAzureCredentialsFile(config.Azure.Credentials)
+		azureConfig.Creds, err = azure.ParseAzureCredentialsFile(config.Azure.Credentials)
 		if err != nil {
 			logrus.Fatalf("cannot load azure credentials: %v", err)
 		}
+		azureConfig.UploadThreads = config.Azure.UploadThreads
 	}
 
 	// If the credentials are not provided in the configuration, then the
@@ -437,7 +438,7 @@ func main() {
 			Output:      output,
 			KojiServers: kojiServers,
 			GCPConfig:   gcpConfig,
-			AzureCreds:  azureCredentials,
+			AzureConfig: azureConfig,
 			AWSCreds:    awsCredentials,
 			AWSBucket:   awsBucket,
 			S3Config: S3Configuration{
diff --git a/templates/packer/ansible/roles/common/tasks/main.yml b/templates/packer/ansible/roles/common/tasks/main.yml
index 9f1fccb1d..dbf762481 100644
--- a/templates/packer/ansible/roles/common/tasks/main.yml
+++ b/templates/packer/ansible/roles/common/tasks/main.yml
@@ -12,8 +12,8 @@
 # Configure the worker.
 - include_tasks: worker-config.yml
 
-# Unsubscribe
-- include_tasks: unsubscribe.yml
+# Unregister
+- include_tasks: unregister.yml
 
 - name: Ensure SELinux contexts are updated
   command: restorecon -Rv /etc
diff --git a/templates/packer/ansible/roles/common/tasks/subscribe.yml b/templates/packer/ansible/roles/common/tasks/subscribe.yml
index 04c0d0d69..06f0d2170 100644
--- a/templates/packer/ansible/roles/common/tasks/subscribe.yml
+++ b/templates/packer/ansible/roles/common/tasks/subscribe.yml
@@ -21,6 +21,6 @@
     - subscribe
   shell: >-
     subscription-manager repos \
-      --enable rhel-9-for-x86_64-appstream-rpms \
-      --enable rhel-9-for-x86_64-baseos-rpms \
-      --enable codeready-builder-for-rhel-9-x86_64-rpms
+      --enable rhel-9-for-{{ ansible_architecture }}-appstream-rpms \
+      --enable rhel-9-for-{{ ansible_architecture }}-baseos-rpms \
+      --enable codeready-builder-for-rhel-9-{{ ansible_architecture }}-rpms
diff --git a/templates/packer/ansible/roles/common/tasks/unregister.yml b/templates/packer/ansible/roles/common/tasks/unregister.yml
new file mode 100644
index 000000000..ba7acc23a
--- /dev/null
+++ b/templates/packer/ansible/roles/common/tasks/unregister.yml
@@ -0,0 +1,8 @@
+---
+
+- name: Unregister
+  become: yes
+  tags:
+    - subscribe
+  shell: >-
+    subscription-manager unregister
diff --git a/templates/packer/ansible/roles/common/tasks/unsubscribe.yml b/templates/packer/ansible/roles/common/tasks/unsubscribe.yml
deleted file mode 100644
index 90c2a3792..000000000
--- a/templates/packer/ansible/roles/common/tasks/unsubscribe.yml
+++ /dev/null
@@ -1,7 +0,0 @@
----
-
-- name: Unsubscribe
-  tags:
-    - subscribe
-  community.general.redhat_subscription:
-    state: absent


Test patch:
diff --git a/cmd/osbuild-worker/config_test.go b/cmd/osbuild-worker/config_test.go
index ebea302b8..aa2cb33b7 100644
--- a/cmd/osbuild-worker/config_test.go
+++ b/cmd/osbuild-worker/config_test.go
@@ -43,6 +43,7 @@ credentials = "/etc/osbuild-worker/gcp-creds"
 
 [azure]
 credentials = "/etc/osbuild-worker/azure-creds"
+upload_threads = 8
 
 [aws]
 credentials = "/etc/osbuild-worker/aws-creds"
@@ -88,7 +89,8 @@ offline_token = "/etc/osbuild-worker/offline_token"
 					Credentials: "/etc/osbuild-worker/gcp-creds",
 				},
 				Azure: &azureConfig{
-					Credentials: "/etc/osbuild-worker/azure-creds",
+					Credentials:   "/etc/osbuild-worker/azure-creds",
+					UploadThreads: 8,
 				},
 				AWS: &awsConfig{
 					Credentials: "/etc/osbuild-worker/aws-creds",
@@ -141,6 +143,17 @@ offline_token = "/etc/osbuild-worker/offline_token"
 		_, err := parseConfig(configFile)
 		require.Error(t, err)
 	})
+
+	t.Run("wrong Azure config", func(t *testing.T) {
+		configFile := prepareConfig(t, `
+[azure]
+credentials = "/etc/osbuild-worker/azure-creds"
+upload_threads = -5
+`)
+		_, err := parseConfig(configFile)
+		require.Error(t, err)
+	})
+
 }
 
 func prepareConfig(t *testing.T, config string) string {