-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: determine platform details from local docker installation for jibDockerBuild #4249
Conversation
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/StepsRunner.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/api/DockerClient.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/StepsRunner.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/api/DockerInfoDetails.java
Show resolved
Hide resolved
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 test was passing for this pom that contains multi-platfom?
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.
Reverted the change to this pom in favor of more unit tests in StepsRunnerTest and integration testing in jib-core.
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 think it still makes sense to have a happy path end-to-end test, so that it is easier for us to reproduce any multi-platform issues in the future. Specifically for this file though, I see that it exists for a few years, and why the test using this file was passing?
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.
That makes sense. I'll bring back the happy path.
Specifically for this file though, I see that it exists for a few years, and why the test using this file was passing?
This test is expected to pass because it is verifying multi-platform registry pushes through the mvn compile jib:build
command. While we don't have multi-platform support for builds to the local docker daemon, pushing to the registry is supported. It verifies the contents of the manifest list as well as the two platform specific images pushed. The pom defines busybox
as the base image which contains support for amd64/linux
and arm64/linux
which matches the platforms specified under the configuration block. This can be verified through docker manifest inspect busybox@sha256:4f47c01fa91355af2865ac10fef5bf6ec9c7f42ad2321377c21e844427972977
which shows:
{
"schemaVersion": 2,
"mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
"manifests": [
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 527,
"digest": "sha256:400ee2ed939df769d4681023810d2e4fb9479b8401d97003c710d0e20f7c49c6",
"platform": {
"architecture": "amd64",
"os": "linux"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 527,
"digest": "sha256:00466b7923ac6cb7bdfd211101c09917e0d416212ca058740954d7adb0aac4f6",
"platform": {
"architecture": "arm",
"os": "linux",
"variant": "v5"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 527,
"digest": "sha256:8fcef70caa67710d8cd568d611b1b23a61886ce5ffab267c5ffa34136999b023",
"platform": {
"architecture": "arm",
"os": "linux",
"variant": "v6"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 527,
"digest": "sha256:6f84685399b2b834d1bc230d2b7f47bed1169b058a338f1b4d05997bb2293a2d",
"platform": {
"architecture": "arm",
"os": "linux",
"variant": "v7"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 527,
"digest": "sha256:759af0b171cc3742032232b5562c1a6d5deb29e487efa2d0e425b2df3a8d5521",
"platform": {
"architecture": "arm64",
"os": "linux",
"variant": "v8"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 527,
"digest": "sha256:ab680d33f94b93e4c3383faad99a9d0195a73d50d4bb039acea5ae7f0ae6691d",
"platform": {
"architecture": "386",
"os": "linux"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 527,
"digest": "sha256:3d96083d0e7c8fee34cd7207880cf838f404d14349cb5e2f78085e05441e8576",
"platform": {
"architecture": "mips64le",
"os": "linux"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 528,
"digest": "sha256:ad1ceba3580dfde5e97fd3dc0fdb56b508ab4cb5ea1ff1dd688982c4eff20357",
"platform": {
"architecture": "ppc64le",
"os": "linux"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"size": 528,
"digest": "sha256:ab8e9b6566a776d442e6d20415a5a73124e2f5bd20dd179fb11ca079de1c13d3",
"platform": {
"architecture": "s390x",
"os": "linux"
}
}
]
}
Let me know if you're looking for any additional details!
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.
Got it, thanks! I guess we were pushing images to GCR?
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 think in this case (and many other integration tests) is verifying registry pushes against a local registry hosted on localhost:5000 (reference)
...in/src/integration-test/resources/gradle/projects/simple/build-multi-platform-invalid.gradle
Outdated
Show resolved
Hide resolved
@@ -647,4 +656,29 @@ private void writeTarFile( | |||
private <E> List<Future<E>> scheduleCallables(ImmutableList<? extends Callable<E>> callables) { | |||
return callables.stream().map(executorService::submit).collect(Collectors.toList()); | |||
} | |||
|
|||
private String computeArchitecture(String architecture) { | |||
if (architecture.equals("x86_64")) { |
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.
Where do we get the mapping between the architecture from docker info and the architecture needed for docker build? Are x86_64
and aarch64
all we need? Also please add unit tests for all the cases.
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 mapping was retrieved from https://docs.docker.com/engine/install/#supported-platforms. From the documented architectures, "x86_64" and "aarch64" appear to be the only ones with alternative naming? I've also added a source code comment.
Done, added unit testing in StepsRunnerTest.
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 see, so the root cause is that the architecture returned from docker info
is different from the architecture used by docker build
. If that's the case, maybe we can change the method name to something like mapDockerInfoArchitechtureToDockerBuildArchitechture
? Or toDockerBuildArchitecture(String dockerInfoArchitecture)
if we think the previous one is too verbose?
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.
That's right, the naming doesn't seem to be standardized everywhere. While docker info
results in x86_64
on my machine, docker manifest inspect
on images uses amd64
which is supposed to be equivalent (docker/docs#4295).
Renaming the method is a good idea. Alternatively, what are your thoughts on normalizeArchitecture()
or standardizeArchitecture()
? I found a couple of references that do something similar:
(1)https://github.com/containerd/platforms/blob/c1438e911ac7596426105350652fe267d0fb8a03/database.go#L76
(2)https://github.com/openzipkin/zipkin/blob/2f0a26fd4e3f3ea24e9fb30f55a84ebbc194129c/build-bin/docker/docker_arch#L7
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.
Thanks for the suggestions! I like normalizeArchitecture()
.
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/StepsRunner.java
Show resolved
Hide resolved
I think this feature does not work with podman, did you test or consider this? When using podman the {
"host": {
"arch": "amd64",
"buildahVersion": "1.23.0",
"cgroupManager": "systemd",
"cgroupVersion": "v2",
"cgroupControllers": [],
Please also see: https://docs.podman.io/en/latest/markdown/podman-info.1.html So determining os and architecture fails and you get |
In addition this feature is also enabled when having just one single platform specified. Even when specifying <platforms>
<platform>
<architecture>amd64</architecture>
<os>linux</os>
</platform>
</platforms> there is no need for this new feature. But it kicks in and fails the build when using podman. How can i disable this feature? |
This code has been tested manually with
jib/examples/helloworld/
.Notes:
docker info
). Otherwise, it throws an error when no matching os and architecture are found.For example, if the platform configurations are as follows then the image with os of
linux
and architecture ofarm64
will be pushed to the docker engine: