-
Notifications
You must be signed in to change notification settings - Fork 170
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 KubeVirt containerdisk build and upload #2750
Conversation
Hi @rmohr. Thanks for your PR. I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
e30b451
to
b9d79f1
Compare
/cc @miabbott Would be happy to get your feedback on this (@cgwalters due to the interest in publishing for fcos). The build and the push seems to work as expected. I plan to tackle the |
/cc @davidvossel FYI |
@rmohr: GitHub didn't allow me to request PR reviews from the following users: davidvossel. Note that only coreos members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
b9d79f1
to
ef90ea4
Compare
/retest |
ef90ea4
to
e7318d3
Compare
/retest |
/retest |
e7318d3
to
48913b9
Compare
177ea8a
to
2c330e8
Compare
Is there a link with more background on this? Is this part of an OCP epic? |
@jlebon This artifact is useful for OCP Virtualization + Hypershift: https://issues.redhat.com/browse/COS-1232 |
/hold
Edit: Need an official release tag, but referencing now the merged code already. |
4c046fa
to
a8ddadd
Compare
mantle/go.mod
Outdated
@@ -1,56 +1,107 @@ | |||
module github.com/coreos/mantle | |||
|
|||
go 1.12 | |||
go 1.17 |
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.
This is probably worth factoring out as a separate preparatory commit.
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 you mean a preparatory PR, or just creating a dedicated commit with the vendoring in a follow-up commit to make it more visible?
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.
Separate commit in the same PR should be fine.
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.
Done.
mantle/go.mod
Outdated
github.com/godbus/dbus/v5 v5.0.4 // indirect | ||
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect | ||
github.com/golang/protobuf v1.5.2 // indirect | ||
github.com/google/go-containerregistry v0.8.0 |
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.
Offhand this library looks sane, but it also duplicates a lot of what we already have vendored in the repositories from github.com/containers.
(Now personally, I would say we should stop vendoring all of podman in this repo, but that's a different discussion)
A possible alternative is to fork off buildah
; we already do that for the other containers.
One specific reason to do that instead of using this library is to ensure that we have a single code path for e.g. authentication, and fewer places we need to change for things like "upload in oci format" (xref #2726 ). I'm guessing go-containerregistry only does v2s2?
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.
But to be clear, I'm OK with this as is.
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.
(Now personally, I would say we should stop vendoring all of podman in this repo, but that's a different discussion)
A possible alternative is to fork off
buildah
; we already do that for the other containers.
If you do that already I can definitely change it. I missed that you already build containers in go in coreos-assembler.
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.
But to be clear, I'm OK with this as is.
Oh nice :)
stream-metadata-go update in #2754. |
f72977d
to
a87ed90
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.
LGTM!
(I restarted CI so that it builds on top of #2808, which will verify that the schema was updated correctly.) |
And it detected issues. Fixing. 👍 |
Build the containerdisk based on a qcow2 image and wrap it into an oci archive as final artifact. This artifact can be uploaded to an arbitrary container registry with the upload step. Signed-off-by: Roman Mohr <[email protected]>
Signed-off-by: Roman Mohr <[email protected]>
Signed-off-by: Roman Mohr <[email protected]>
Signed-off-by: Roman Mohr <[email protected]>
Head branch was pushed to by a user without write access
8cbfdec
to
808b53c
Compare
@jlebon can it be that the pipeline changes only get picked up after a merge? |
Ahh yup, indeed. Jenkins is wary of you. :)
I think we just need to add you to the org. Let me sort that out. |
Signed-off-by: Roman Mohr <[email protected]>
808b53c
to
6f7af75
Compare
OK right, this won't work. Hmm, but how does the existing buildah bits in the oscontainer code work in the RHCOS pipeline currently then? Ahhh OK, I think it's this bit: coreos-assembler/src/oscontainer.py Lines 119 to 121 in f6991e5
coreos-assembler/src/oscontainer.py Line 27 in f6991e5
@rmohr Can you try adding that to |
Ensure that buildah is called for kubevirt with the same base arguments like for the oscontainer. Signed-off-by: Roman Mohr <[email protected]>
That did the trick! 👍 |
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.
Nice work!
Create a KubeVirt containerdisk ociarchive and upload it to arbitrary container registries.
buildah
is used for building theociarchive
,skopeo
is used to push the image to a container registry.Build the ociarchive
Upload the OCI archive
Generate and inspect metadata