-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
Conversation
4ca6987
to
67efcc3
Compare
--tag docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}-arm64 | ||
--push | ||
./dist | ||
- docker manifest create docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
--file operator/Dockerfile | ||
--target manager | ||
--tag docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}-arm64 | ||
--push |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
--push | ||
./dist | ||
- docker buildx build | ||
--provenance false --sbom false --platform linux/arm64 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
67efcc3
to
c4c9ccb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks fine to me, one question about the use of --push
in the updated docker build step, but aside from that 👍
@@ -8,6 +8,8 @@ spec: | |||
nodePools: | |||
- name: first | |||
replicas: 3 | |||
storage: {} |
There was a problem hiding this comment.
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.
--file operator/Dockerfile | ||
--target manager | ||
--tag docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}} | ||
--tag docker.io/redpandadata/{{.OPERATOR_REPO}}:{{.TAG_NAME}}-amd64 | ||
--push |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
f3f8179
to
51f3fe1
Compare
In the CI the following error poped up when helm chart was pushed to docker hub container registry: ``` [+] Building 0.0s (0/0) docker:default ERROR: Multi-platform build is not supported for the docker driver. Switch to a different driver, or turn on the containerd image store, and try again. Learn more at https://docs.docker.com/go/build-multi-platform/ ``` https://buildkite.com/redpanda/redpanda-operator/builds/3741#0193d2e2-385c-4b84-9a23-8b047814b3fe/332-333
51f3fe1
to
99c2fad
Compare
Based on:
Fix nightly container build
In the CI the following error poped up when helm chart was pushed to docker hub
container registry:
https://buildkite.com/redpanda/redpanda-operator/builds/3741#0193d2e2-385c-4b84-9a23-8b047814b3fe/332-333
As machine in
k8s-builder
buildkite queue couldn't finish container build within 10 minutes change the buildkite queue name tov6-amd64-builders-m6id
.