Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multi arch nightly build #369

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Fix multi arch nightly build #369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4df1ff
Fix nightly container build
RafalKorepta Jan 3, 2025
eb02560
Change buildkite agent queue to bigger/faster machines
RafalKorepta Jan 3, 2025
99c2fad
Add default empty storage and cloud cache storage spec
RafalKorepta Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .buildkite/pipeline.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ steps:
label: ":gandalf: Nightly Operator release"
timeout_in_minutes: 10
agents:
queue: k8s-builders
queue: v6-amd64-builders-m6id
commands:
- |
TAG_NAME=$(ci/scripts/tag-check.sh) ./ci/scripts/run-in-nix-docker.sh task ci:configure ci:publish-k8s-nightly-artifacts
Expand Down
2 changes: 2 additions & 0 deletions operator/tests/e2e-with-flags/nodepools-delete/00-one-nodepool.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ spec:
nodePools:
- name: first
replicas: 3
storage: {}
cloudCacheStorage: {}
resources:
requests:
cpu: "100m"
Expand Down
4 changes: 4 additions & 0 deletions operator/tests/e2e-with-flags/nodepools-delete/01-add-second-pool.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ spec:
nodePools:
- name: first
replicas: 3
storage: {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem odd. I get that they were blocking this passing CI all of a sudden (wondering if we have something wrong with the tests/CI that is making them occasionally pass even without the change?), so I'm fine with adding these as a fix for now... that said, @birdayz is there a reason why these are marked with required? It looks like all of the corresponding struct fields are optional...

redpanda-operator/operator/api/vectorized/v1alpha1/cluster_types.go

Lines 213 to 215 in 0312468

// Storage spec for cluster
// +required
Storage StorageSpec `json:"storage"`

Any chance that could be changed to omitempty? No need to change to a pointer or anything, but I don't like the idea of hacking in an empty json map just for the sake of passing field validation.

cloudCacheStorage: {}
resources:
requests:
cpu: "100m"
Expand All @@ -17,6 +19,8 @@ spec:
memory: 256Mi
- name: second
replicas: 3
storage: {}
cloudCacheStorage: {}
resources:
requests:
cpu: "101m"
Expand Down
2 changes: 2 additions & 0 deletions operator/tests/e2e-with-flags/nodepools-delete/02-delete-first-nodepool.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ spec:
nodePools:
- name: second
replicas: 3
storage: {}
cloudCacheStorage: {}
resources:
requests:
cpu: "101m"
Expand Down
2 changes: 2 additions & 0 deletions operator/tests/e2e-with-flags/nodepools/00-one-nodepool.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ spec:
nodePools:
- name: nodepool1
replicas: 3
storage: {}
cloudCacheStorage: {}
resources:
requests:
cpu: "100m"
Expand Down
4 changes: 4 additions & 0 deletions operator/tests/e2e-with-flags/nodepools/01-two-nodepools.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ spec:
nodePools:
- name: nodepool1
replicas: 3
storage: {}
cloudCacheStorage: {}
resources:
requests:
cpu: "100m"
Expand All @@ -17,6 +19,8 @@ spec:
memory: 256Mi
- name: nodepool2
replicas: 3
storage: {}
cloudCacheStorage: {}
resources:
requests:
cpu: "100m"
Expand Down
4 changes: 4 additions & 0 deletions operator/tests/e2e-with-flags/nodepools/02-scale-down-first.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ spec:
nodePools:
- name: nodepool1
replicas: 0
storage: {}
cloudCacheStorage: {}
resources:
requests:
cpu: "100m"
Expand All @@ -17,6 +19,8 @@ spec:
memory: 256Mi
- name: nodepool2
replicas: 3
storage: {}
cloudCacheStorage: {}
resources:
requests:
cpu: "100m"
Expand Down
15 changes: 13 additions & 2 deletions taskfiles/k8s.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,12 +156,23 @@ tasks:
silent: true
- 'echo "~~~ :docker: Build & Push operator container"'
- docker buildx build
--platform linux/arm64,linux/amd64
--provenance false --sbom false --platform linux/amd64
--file operator/Dockerfile
--target manager
--tag docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}
--tag docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}-amd64
--push
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to remove the --push here if we're manually joining the two image tags below? Won't this wind up with us pushing 3 image tags?

Copy link
Contributor Author

@RafalKorepta RafalKorepta Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in docker hub nor container registry, but if you are pushing manifest you are not uploading/sending/providing actual container layers. Sole manifest is useless without actual image, with its layers, in container registry.

https://docs.docker.com/reference/cli/docker/manifest/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the docs imply that the listed images will be automatically resolved to their sha256 and pushed. I'm in favor of removing the --push.

 docker manifest push 45.55.81.106:5000/coolapp:v1
Pushed manifest 45.55.81.106:5000/coolapp@sha256:9701edc932223a66e49dd6c894a11db8c2cf4eccd1414f1ec105a623bf16b426 with digest: sha256:f67dcc5fc786f04f0743abfe0ee5dae9bd8caf8efa6c8144f7f2a43889dc513b
Pushed manifest 45.55.81.106:5000/coolapp@sha256:f3b3b28a45160805bb16542c9531888519430e9e6d6ffc09d72261b0d26ff74f with digest: sha256:b64ca0b60356a30971f098c92200b1271257f100a55b351e6bbe985638352f3a
Pushed manifest 45.55.81.106:5000/coolapp@sha256:39dc41c658cf25f33681a41310372f02728925a54aac3598310bfb1770615fc9 with digest: sha256:df436846483aff62bad830b730a0d3b77731bcf98ba5e470a8bbb8e9e346e4e8
Pushed manifest 45.55.81.106:5000/coolapp@sha256:f91b1145cd4ac800b28122313ae9e88ac340bb3f1e3a4cd3e59a3648650f3275 with digest: sha256:5bb8e50aa2edd408bdf3ddf61efb7338ff34a07b762992c9432f1c02fc0e5e62
sha256:050b213d49d7673ba35014f21454c573dcbec75254a08f4a7c34f66a47c06aba

I wonder if the docker manifest command might have a way to bundle the helm-chart into the same tag 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the docker manifest command might have a way to bundle the helm-chart into the same tag 🤔

It actually seems likely we could get this to work though I'm not sure if it's entirely worth the effort. It would just be awesome to bundle the chart into the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can test it, but my intuition tells me manifest (metadata) would not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work. I'm reverting.

https://buildkite.com/redpanda/redpanda-operator/builds/3948#019446d5-4701-446b-af7c-91a4d581ba92/344-437

As I said and you can see in the article https://containers.gitbook.io/build-containers-the-hard-way#manifest manifest is just a reference or I would say a metadata. If manifest can not find particular manifest in the registry it will stop you from creating a manifest.

./dist
- docker buildx build
--provenance false --sbom false --platform linux/arm64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning for disabling provenance and sbom? Not strong feelings, just curious if it was necessary for something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I borrowed the steps from @birdayz script.

For nightly I personally wouldn't care that much, but for official container image registry it would be wise to generate sbom and sign that image. Yet it is not our standard software delivery practise as far as I know.

--file operator/Dockerfile
--target manager
--tag docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}-arm64
--push
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we want to push the intermediate tags? Seems like it would be cleaner to just have the multi-arch image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cleaner, but I don't know how to perform such push without changes to the docker engine that runs in our buildkite runner.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we just need to remove the --push argument or does that do something other than pushing the built image to dockerhub?

./dist
- docker manifest create docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may also be possible to make the original command work if we make a buildkit builder or change the default docker settings. Is there a reason to pick one option over the other here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason of this PR is to speed up the fix nightly release process. The JIRA issue was opened for 1 month with 0 progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better the code that was already merged in this repo worked with multi architecture images, but it stopped when I pushed helm artifact. It might be related to the tag overlap which turns off ability to push and build multi arch images.

Yet I'm able to over come that limitation with manifest which is weird for me.

--amend docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}-amd64
--amend docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}-arm64
- docker manifest push docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}
- cmd: docker logout
preconditions:
- test -n "$DOCKERHUB_USER"
Expand Down