Support multi-arch builds #3
Conversation
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM, tested with the docker images for arm and a arm cluster in cloudstack
Able to create PVC and pod were using the it for persistent storage
pearl1594/cloudstack-csi-driver:test
Able to deploy
Test manifest file
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the Docker build process by implementing multi-stage builds with multi-architecture support. The changes enable building container images for both linux/amd64 and linux/arm64 platforms directly in the CI/CD pipeline without requiring pre-built binaries.
Key changes:
- Dockerfiles now use multi-stage builds with Go compilation inside the builder stage
- GitHub Actions workflow updated to use Docker Buildx for multi-platform builds
- Build arguments (LDFLAGS, TARGETOS, TARGETARCH) are passed to Docker builds for cross-compilation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/cloudstack-csi-driver/Dockerfile | Added multi-stage build with builder stage for Go compilation and multi-arch support |
| cmd/cloudstack-csi-sc-syncer/Dockerfile | Added multi-stage build with builder stage for Go compilation and multi-arch support |
| .github/workflows/release.yaml | Updated to use Docker Buildx for multi-platform builds; removed make container step; added Go build step for syncer binary upload |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/release.yaml
Outdated
| - name: Build syncer binary for upload | ||
| if: startsWith(github.ref, 'refs/tags/v') | ||
| run: | | ||
| REV=$(git describe --long --tags --match='v*' --dirty 2>/dev/null || git rev-list -n1 HEAD) | ||
| GIT_COMMIT=$(git rev-parse HEAD) | ||
| BUILD_DATE=$(date -u -Iseconds) | ||
| PKG=github.com/cloudstack/cloudstack-csi-driver | ||
| LDFLAGS="-s -w -X ${PKG}/pkg/driver.driverVersion=${REV} -X ${PKG}/pkg/driver.gitCommit=${GIT_COMMIT} -X ${PKG}/pkg/driver.buildDate=${BUILD_DATE}" | ||
| mkdir -p bin | ||
| CGO_ENABLED=0 go build -ldflags "${LDFLAGS}" -o ./bin/cloudstack-csi-sc-syncer ./cmd/cloudstack-csi-sc-syncer |
There was a problem hiding this comment.
The 'Build syncer binary for upload' step attempts to run go build but there's no Go setup step in this workflow. This will fail because Go is not installed on the runner. Add a setup-go action before this step, similar to the PR check workflow which uses actions/setup-go@v5 with go-version: \"1.23\".
| ARG TARGETOS | ||
| ARG TARGETARCH |
There was a problem hiding this comment.
The build arguments TARGETOS and TARGETARCH are declared but Docker Buildx automatically provides these as built-in arguments when using --platform. You should add default values for these arguments to ensure the Dockerfile works correctly in non-buildx contexts. For example: ARG TARGETOS=linux and ARG TARGETARCH=amd64.
| ARG TARGETOS | |
| ARG TARGETARCH | |
| ARG TARGETOS=linux | |
| ARG TARGETARCH=amd64 |
| ARG TARGETOS | ||
| ARG TARGETARCH |
There was a problem hiding this comment.
The build arguments TARGETOS and TARGETARCH are declared but Docker Buildx automatically provides these as built-in arguments when using --platform. You should add default values for these arguments to ensure the Dockerfile works correctly in non-buildx contexts. For example: ARG TARGETOS=linux and ARG TARGETARCH=amd64.
| ARG TARGETOS | |
| ARG TARGETARCH | |
| ARG TARGETOS=linux | |
| ARG TARGETARCH=amd64 |
| REV=$(git describe --long --tags --match='v*' --dirty 2>/dev/null || git rev-list -n1 HEAD) | ||
| GIT_COMMIT=$(git rev-parse HEAD) | ||
| BUILD_DATE=$(date -u -Iseconds) | ||
| PKG=github.com/cloudstack/cloudstack-csi-driver | ||
| LDFLAGS="-s -w -X ${PKG}/pkg/driver.driverVersion=${REV} -X ${PKG}/pkg/driver.gitCommit=${GIT_COMMIT} -X ${PKG}/pkg/driver.buildDate=${BUILD_DATE}" |
There was a problem hiding this comment.
The LDFLAGS construction is duplicated across three separate steps (lines 47-51, 69-73, and 89-93). Consider extracting this logic into a reusable composite action or a separate script to reduce code duplication and ensure consistency.
.github/workflows/release.yaml
Outdated
| - name: Build syncer binary for upload | ||
| if: startsWith(github.ref, 'refs/tags/v') | ||
| run: | | ||
| REV=$(git describe --long --tags --match='v*' --dirty 2>/dev/null || git rev-list -n1 HEAD) |
There was a problem hiding this comment.
Can we move all these params to a single step? this will make the github actions cleaner.
.github/workflows/release.yaml
Outdated
| LDFLAGS="-s -w -X ${PKG}/pkg/driver.driverVersion=${REV} -X ${PKG}/pkg/driver.gitCommit=${GIT_COMMIT} -X ${PKG}/pkg/driver.buildDate=${BUILD_DATE}" | ||
| mkdir -p bin | ||
| CGO_ENABLED=0 go build -ldflags "${LDFLAGS}" -o ./bin/cloudstack-csi-sc-syncer ./cmd/cloudstack-csi-sc-syncer |
There was a problem hiding this comment.
Would this build binaries for all architectures?
Issue #, if available:
Description of changes:
Builds multi-arch docker images to enable running csi driver on both amd64 & arm64.
Testing performed:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.