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.

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

Add oci-layout to platformSpecificSource #2971

Merged
merged 1 commit into from Jul 21, 2022

Conversation

giggsoff
Copy link
Contributor

oci-layout source is platform-scpecific, we should use provided
platform to resolve correct image.

Now I can see the problem with building for another arch and seems we reuse image with host arch for build: linuxkit/linuxkit#3797 (comment)

Copy link
Contributor

@deitch deitch left a comment

Choose a reason for hiding this comment

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

@giggsoff explained this in detail to me, and was able to replicate it.

Because it does not provide a platform (i.e. ignores it), it always looks for the one the builder is running on, even if you requested a different one via:

SolveOpt{
		FrontendAttrs: map[string]string{
                    "platform": "linux/arm64", // or whatever
                },
	}

@jedevc jedevc requested a review from tonistiigi July 20, 2022 13:55
@tonistiigi
Copy link
Member

Not a blocker for this PR, but could we make this case covered by integration tests?

@giggsoff
Copy link
Contributor Author

Not a blocker for this PR, but could we make this case covered by integration tests?

Sure, will try to add the test for this specific case.
It seems that not all current tests passed, not sure that failed TestClientGatewayIntegration/TestClientGatewaySlowCacheExecError/worker=containerd-rootless test is related with changes.

@tonistiigi
Copy link
Member

test is related with changes.

It's #2926 . Restarted

@deitch
Copy link
Contributor

deitch commented Jul 20, 2022

@giggsoff here is the client_test.go for the PR that created OCI source.

You should be able either to extend testOCILayoutSource() to include specific arch use case, or create a separate test.

I can see perhaps including arch-specific information in the built image, e.g. adding a line here where we check the results here. Might want to build for multiple platforms by putting this in a for loop, each one checking the expected platform in the file. That way you know you are pulling the right one.

oci-layout source is platform-scpecific, we should use provided
platform to resolve correct image.

Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
@giggsoff giggsoff force-pushed the fix-build-another-arch-oci-layout branch from 09af600 to 9447ace Compare July 21, 2022 15:29
@giggsoff
Copy link
Contributor Author

@tonistiigi test is ready.
@deitch thanks for recommendations. Decided to add another test for this case.

@sudo-bmitch
Copy link

oci-layout source is platform-scpecific, we should use provided platform to resolve correct image.

OCI Layout can include multiplatform images. I haven't dug through the changes here, but is this related to how the Layout is generated, and do we want to support other sources of a Layout?

More generally, we probably want to warn or fail when a single platform source is used and the platform does not match the requirements for the build. I think that's being looked at in #2946.

@deitch
Copy link
Contributor

deitch commented Jul 21, 2022

The oci layout just treats it like a registry. Give it the digest, it sends back that manifest. From that point on, it's the same actual code we use for the registry: resolve the arch, find the manifest, etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants