Arm runner #1262

JunAr7112 · 2025-02-11T18:11:46Z

No description provided.

copy-pr-bot · 2025-02-11T18:11:49Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

cdesiniotis

@JunAr7112 thanks for raising this PR. Can you please squash your commits into one and sign off your commit? Our DCO check is failing https://github.com/NVIDIA/gpu-operator/pull/1262/checks

cdesiniotis · 2025-02-11T20:51:35Z

.github/workflows/ci.yaml

+      - name: Set up QEMU
+        uses: docker/setup-qemu-action@v3
+        with:
+          image: tonistiigi/binfmt:master


We no longer need QEMU since we are not cross-compiling anymore.

.github/workflows/ci.yaml

cdesiniotis · 2025-02-11T22:16:09Z

.github/workflows/ci.yaml

+          VERSION: ${COMMIT_SHORT_SHA}-arm
+        run: |
+          echo "${VERSION}"
+          make build-${{ matrix.dist }}


Question -- do we need to invoke the push target after this to ensure the tag gets pushed to the ghcr.io registry?

I think the push target gets invoked in the Makefile via its inclusion in native-only.mk.

cdesiniotis · 2025-02-11T22:19:05Z

.github/workflows/ci.yaml

+          IMAGE_ID_AMD: ghcr.io/${{ env.LOWERCASE_REPO_OWNER}}/gpu-operator:${{ env.COMMIT_SHORT_SHA }}-amd
+          IMAGE_ID_ARM_VAL: ghcr.io/${{ env.LOWERCASE_REPO_OWNER }}/gpu-operator/gpu-operator-validator:${{ env.COMMIT_SHORT_SHA }}-arm
+          IMAGE_ID_AMD_VAL: ghcr.io/${{ env.LOWERCASE_REPO_OWNER }}/gpu-operator/gpu-operator-validator:${{ env.COMMIT_SHORT_SHA }}-amd
+          MANIFEST: ghcr.io/${{ env.LOWERCASE_REPO_OWNER }}/gpu-operator:${{ env.COMMIT_SHORT_SHA }}-multiarch


Let's remove the -multiarch suffix

Suggested change

MANIFEST: ghcr.io/${{ env.LOWERCASE_REPO_OWNER }}/gpu-operator:${{ env.COMMIT_SHORT_SHA }}-multiarch

MANIFEST: ghcr.io/${{ env.LOWERCASE_REPO_OWNER }}/gpu-operator:${{ env.COMMIT_SHORT_SHA }}

cdesiniotis · 2025-02-11T22:19:37Z

.github/workflows/ci.yaml

+            ${MANIFEST_VAL} \
+            ${IMAGE_ID_AMD_VAL} \
+            ${IMAGE_ID_ARM_VAL}
+          docker manifest push ${MANIFEST}


Suggested change

docker manifest push ${MANIFEST}

docker manifest push ${MANIFEST_VAL}

cdesiniotis · 2025-02-11T22:23:39Z

native-only.mk

-DOCKER_BUILD_PLATFORM_OPTIONS = --platform=linux/amd64
-
+#DOCKER_BUILD_PLATFORM_OPTIONS = --platform ?= linux/amd64
+DOCKER_BUILD_OPTIONS = --output=type=image,push=$(PUSH_ON_BUILD) --provenance=$(ATTACH_ATTESTATIONS) --sbom=$(ATTACH_ATTESTATIONS)


What is the rationale behind this change?

So I commented out the DOCKER_BUILD_PLATFORM_OPTIONS because we were already specifying it in the workflow in ci.yaml. I added the DOCKER_BUILD_OPTIONS to store the image so we can build the Manifest in the next job.

cdesiniotis · 2025-02-11T22:23:52Z

validator/Makefile

+ifeq ($(BUILD_MULTI_ARCH_IMAGES),true)
+  include $(CURDIR)/multi-arch.mk
 else
-include $(CURDIR)/multi-arch.mk
+  include $(CURDIR)/native-only.mk


What was the rationale behind this change?

cdesiniotis · 2025-02-11T22:25:29Z

arm-only.mk

@@ -0,0 +1,20 @@
+# Copyright (c) 2022, NVIDIA CORPORATION.  All rights reserved.


I don't see this file being used as at all, and it shouldn't be needed. Can we remove it?

cdesiniotis · 2025-02-11T22:25:37Z

validator/arm-only.mk

@@ -0,0 +1,19 @@
+# Copyright (c) 2022, NVIDIA CORPORATION.  All rights reserved.


I don't see this file being used as at all, and it shouldn't be needed. Can we remove it?

Yes I will remove it.

cdesiniotis · 2025-02-11T22:26:29Z

.github/workflows/ci.yaml

@@ -19,6 +19,7 @@ on:
    branches:
      - "pull-request/[0-9]+"
      - main
+      - ARM_Runner


This should not be needed. Our GitHub actions run on PRs, so the changes you are making we can test via this PR.

@JunAr7112 let's remove this.

cdesiniotis · 2025-02-12T00:41:27Z

/ok to test

........... PR changes 2 Signed-off-by: Arjun <[email protected]>

cdesiniotis · 2025-02-12T19:17:17Z

.github/workflows/ci.yaml

@@ -19,6 +19,7 @@ on:
    branches:
      - "pull-request/[0-9]+"
      - main
+      - ARM_Runner


@JunAr7112 let's remove this.

cdesiniotis · 2025-02-12T19:20:22Z

.github/workflows/ci.yaml

@@ -133,6 +134,7 @@ jobs:
          echo "LABEL_IMAGE_SOURCE=https://github.com/${REPO_FULL_NAME}" >> $GITHUB_ENV

          GENERATE_ARTIFACTS="false"
+          #NOT_USING_ARM="false"


Please remove this.

cdesiniotis · 2025-02-12T19:24:51Z

Makefile

+
+

nit: unneeded newlines.

cdesiniotis · 2025-02-12T19:25:06Z

.github/workflows/ci.yaml

          SUBCOMPONENT: validator
        run: |
          echo "${VERSION}"
          make build-${{ matrix.dist }}

+
+  ### MULTI-ARCH-IMAGES test ###


nit: update the comment or remove it.

cdesiniotis · 2025-02-12T19:25:23Z

.github/workflows/ci.yaml

+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v3


We don't need docker buildx for this job.

cdesiniotis · 2025-02-12T19:26:33Z

.github/workflows/ci.yaml

+          LOWERCASE_REPO_OWNER: ${{ env.LOWERCASE_REPO_OWNER }}
+          COMMIT_SHORT_SHA: ${{ env.COMMIT_SHORT_SHA }}


Are these lines needed? It looks like you are already adding this variables to the environment in a prior step.

I will remove these lines

Signed-off-by: Arjun <[email protected]>

JunAr7112 requested review from ArangoGutierrez, cdesiniotis, elezar, shivamerla and tariq1890 as code owners February 11, 2025 18:11

cdesiniotis reviewed Feb 11, 2025

View reviewed changes

JunAr7112 force-pushed the ARM_Runner branch from 7f02169 to c8a5e54 Compare February 12, 2025 00:23

bump kubevirt-device-plugin to v1.3.0

ae0537c

........... PR changes 2 Signed-off-by: Arjun <[email protected]>

JunAr7112 force-pushed the ARM_Runner branch from c8a5e54 to ae0537c Compare February 12, 2025 18:41

cdesiniotis reviewed Feb 12, 2025

View reviewed changes

JunAr7112 force-pushed the ARM_Runner branch from b2eacc7 to 9de76aa Compare February 12, 2025 21:21

code review 2

1b9777b

Signed-off-by: Arjun <[email protected]>

JunAr7112 force-pushed the ARM_Runner branch from 9de76aa to 1b9777b Compare February 12, 2025 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arm runner #1262

Arm runner #1262

JunAr7112 commented Feb 11, 2025

copy-pr-bot bot commented Feb 11, 2025

cdesiniotis left a comment

cdesiniotis Feb 11, 2025

cdesiniotis Feb 11, 2025

JunAr7112 Feb 11, 2025

cdesiniotis Feb 11, 2025

cdesiniotis Feb 11, 2025

cdesiniotis Feb 11, 2025

JunAr7112 Feb 11, 2025

cdesiniotis Feb 11, 2025

cdesiniotis Feb 11, 2025

cdesiniotis Feb 11, 2025

JunAr7112 Feb 11, 2025

cdesiniotis Feb 11, 2025

cdesiniotis Feb 12, 2025

cdesiniotis commented Feb 12, 2025

cdesiniotis Feb 12, 2025

cdesiniotis Feb 12, 2025

cdesiniotis Feb 12, 2025

cdesiniotis Feb 12, 2025

cdesiniotis Feb 12, 2025

cdesiniotis Feb 12, 2025

JunAr7112 Feb 12, 2025

	MANIFEST: ghcr.io/${{ env.LOWERCASE_REPO_OWNER }}/gpu-operator:${{ env.COMMIT_SHORT_SHA }}-multiarch
	MANIFEST: ghcr.io/${{ env.LOWERCASE_REPO_OWNER }}/gpu-operator:${{ env.COMMIT_SHORT_SHA }}

	docker manifest push ${MANIFEST}
	docker manifest push ${MANIFEST_VAL}

		@@ -0,0 +1,20 @@
		# Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

		@@ -0,0 +1,19 @@
		# Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

		- name: Set up Docker Buildx
		uses: docker/setup-buildx-action@v3

		LOWERCASE_REPO_OWNER: ${{ env.LOWERCASE_REPO_OWNER }}
		COMMIT_SHORT_SHA: ${{ env.COMMIT_SHORT_SHA }}

Arm runner #1262

Are you sure you want to change the base?

Arm runner #1262

Conversation

JunAr7112 commented Feb 11, 2025

copy-pr-bot bot commented Feb 11, 2025

cdesiniotis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdesiniotis commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment