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 remote-build to allow cosa builds remotely using podman remote #2887

Merged
merged 1 commit into from
Jun 21, 2022

Conversation

ravanelli
Copy link
Member

No description provided.

@dustymabe
Copy link
Member

Does this overlap at all with #2828 ?

@jlebon
Copy link
Member

jlebon commented Jun 3, 2022

It seems like it, though it looks like this PR additionally also builds the image via podman --remote. I do like that #2828 enhances the existing cosa push-container. How about making this PR add a new cosa build-container that only builds and then enhancing cosa push-container by finishing #2828 ?

@ravanelli
Copy link
Member Author

It seems like it, though it looks like this PR additionally also builds the image via podman --remote. I do like that #2828 enhances the existing cosa push-container. How about making this PR add a new cosa build-container that only builds and then enhancing cosa push-container by finishing #2828 ?

Yeah, I think we can make it. Looks the only thing we will need to do is slightly modify the manifest creation part to not use the archives.item that is looking for the builds itself.

#2828 has been there for a while though, is @rmohr still working on it?

@rmohr
Copy link
Member

rmohr commented Jun 7, 2022

@ravanelli I am no longer working on this. If you can reuse anything from my open PRs, feel free to do so :)

@ravanelli
Copy link
Member Author

@ravanelli I am no longer working on this. If you can reuse anything from my open PRs, feel free to do so :)

@rmohr Thanks! I will try to finish it 😃

@ravanelli
Copy link
Member Author

@jlebon and @dustymabe As we discussed, the cosa build (build cosa and push to quay) part is in this PR, and the manifest creation in #2828. I used remote-build but let me know if you have a better name in mind

@ravanelli ravanelli changed the title Add cmd-manifest to allow multi-arch manifest creation Add remote-build to allow cosa builds remotely using podman remote Jun 7, 2022
src/cmd-manifest Outdated Show resolved Hide resolved
src/cmd-manifest Outdated
Comment on lines 50 to 51
run_cmd("podman --remote push %s:%s-%s" % (repo, arch, commit))
tags = run_cmd("podman search --list-tags %s" % (repo), "output")
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use podman push --digestfile here as we do in other places to ensure we get the returned digest reliably - a "push then inspect" flow is more racy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just double checked it, and --digestfile doesn't work with podman remote according to the podman docs and my tests. I probably should add a comment about it there.

@ravanelli ravanelli force-pushed the podman branch 2 times, most recently from 9d7801a to f189360 Compare June 7, 2022 18:33
@ravanelli ravanelli requested a review from cgwalters June 7, 2022 18:35
@dustymabe
Copy link
Member

I used remote-build but let me know if you have a better name in mind

how about cosa buildcosa? cosa remote-build could be confusing as one might think you are just trying to do cosa build on a remote system. cosa buildcosa is more reflective of the operation we are actually doing here. An implementation detail is that we do require you to be targetting a remote system for this (doesn't really make sense otherwise).

src/cmd-remote-build Outdated Show resolved Hide resolved
run_cmd("podman --remote build --tag %s:%s-%s -f %s" % (repo, arch, commit, file))
run_cmd("podman --remote push %s:%s-%s" % (repo, arch, commit))

# Podman remote doesn't allow push using digestfile. That's why the tag check is done
Copy link
Member

Choose a reason for hiding this comment

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

Podman remote doesn't allow push using digestfile. That's why the tag check is done

Is there an open issue/feature request for this? or is it documented somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about issue/features.

src/cmd-remote-build Outdated Show resolved Hide resolved
src/cmd-remote-build Outdated Show resolved Hide resolved
src/cmd-remote-build Outdated Show resolved Hide resolved
src/cmd-remote-build Outdated Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented Jun 7, 2022

I used remote-build but let me know if you have a better name in mind

how about cosa buildcosa? cosa remote-build could be confusing as one might think you are just trying to do cosa build on a remote system. cosa buildcosa is more reflective of the operation we are actually doing here. An implementation detail is that we do require you to be targetting a remote system for this (doesn't really make sense otherwise).

IMO what makes this useful is the remote and multi-arch parts of it, not really that it's building cosa. E.g. we could use this to build multi-arch images of ignition-validate and coreos-installer in the future.

How about cosa remote-build-container? Makes it closer in naming to the existing cosa push-container.

@dustymabe
Copy link
Member

This is a followup topic, but we should think here about how we could do tagged builds using this mechanism. i.e. v0.13.0.

@dustymabe
Copy link
Member

dustymabe commented Jun 7, 2022

I used remote-build but let me know if you have a better name in mind

how about cosa buildcosa? cosa remote-build could be confusing as one might think you are just trying to do cosa build on a remote system. cosa buildcosa is more reflective of the operation we are actually doing here. An implementation detail is that we do require you to be targetting a remote system for this (doesn't really make sense otherwise).

IMO what makes this useful is the remote and multi-arch parts of it, not really that it's building cosa. E.g. we could use this to build multi-arch images of ignition-validate and coreos-installer in the future.

How about cosa remote-build-container? Makes it closer in naming to the existing cosa push-container.

I guess I could see that assuming it's generic enough (which it seems like it is). I'll counter you and propose cosa build-container, which I guess could be confused with the OSTree container we create.

EDIT: Also wonder if we should change push-container to push-manifest or something.

@cgwalters
Copy link
Member

This is just a thin wrapper around podman --remote right? Why not cosa podman-remote-build? Having long names I think is fine for things that aren't frequently invoked locally/manually, but are only executed by CI pipelines normally.

But now that I look more...what about dropping the cosa prefix entirely because this doesn't actually relate to anything in coreos-assembler? It could just be a script that we happen to ship in the cosa codebase and resulting container image, like podman-remote-build-multiarch
or so?

We're effectively just creating a schema for build tag names on top of podman --remote build right?

@dustymabe
Copy link
Member

I'm supportive of a more verbose name (as @cgwalters suggests) but I'd still like it wrapped by cosa if possible. A lot of times cosa is the entrypoint (especially if you are running it as a container). It can also be a good way to discover what's possible. Random script in a directory doesn't really give you that "discoverability". i.e. cosa podman-remote-build is something that a user could want to execute directly, but create_disk.sh isn't something a user would want to call directly.

@cgwalters
Copy link
Member

I'm supportive of a more verbose name (as @cgwalters suggests) but I'd still like it wrapped by cosa if possible. A lot of times cosa is the entrypoint (especially if you are running it as a container). It can also be a good way to discover what's possible.

To me, a key aspect of cosa the verb is that it operates on the "cosa dir", which has things like builds/. The original design of the command was very much to match how git operates in this way. Almost all the other commands either read or mutate this.

Now, there are some git commands that don't operate on the current git directory; for example, git config --global. But right there, you can see the exception to the rule being called out.

We can solve the discoverability problem by giving tools that don't operate on a cosa dir a different entrypoint name...though bike-shedding would be needed for that. Perhaps casm as short for coreos-assembler?

I've been noodling on an alternative entrypoint name in the context of #2685 too...the current strawman there was cosa init --ostree but following my same argument, since that flow would not involve a cosa-style builds/ etc. schema, probably it should be a new command. I am not sure we need anything more than "build and push", so perhaps to start it could be csam ostree build and csam ostree push or so.

@cgwalters
Copy link
Member

Of course also we do have today already alternative entrypoints in kola and mantle (and for that matter of course there's gangplank). And we're also using https://github.com/coreos/fedora-coreos-releng-automation which really should be its own container instead of being dumped dynamically into a cosa dir.

Anyways...don't get me wrong, I am not objecting to this PR. If we ship it as is, OK.

But...I'd restate that we already have multiple entrypoints, and it makes more sense to me for things that have nothing to do with the "cosa dir" to have an entrypoint other than cosa.

src/cmd-remote-build Outdated Show resolved Hide resolved
@ravanelli
Copy link
Member Author

We didn't come to an agreement on the cmd name. I agree with @jlebon in using cosa remote-build-container and keeping it as part of cosa. Nonetheless, we can always change names if we decide too.
What do you think about it?

@cgwalters
Copy link
Member

We didn't come to an agreement on the cmd name. I agree with @jlebon in using cosa remote-build-container and keeping it as part of cosa. Nonetheless, we can always change names if we decide too.

This is totally fine by me for the record!

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

A few comments.

I think some of how this is used (i.e. how it's coordinated with the followup job that pushes the manifest) are still details that aren't 100% clear to me but I think we can get this in and then fixup anything else in a followup.

src/cmd-remote-build-container Outdated Show resolved Hide resolved
src/cmd-remote-build-container Outdated Show resolved Hide resolved
src/cmd-remote-build-container Outdated Show resolved Hide resolved
src/cmd-remote-build-container Outdated Show resolved Hide resolved
src/cmd-remote-build-container Outdated Show resolved Hide resolved
src/cmd-remote-build-container Outdated Show resolved Hide resolved
src/cmd-remote-build-container Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member

one thing I am interested in.. Can we squash these into one commit and add an empty line between the title and body of the commit?

dustymabe
dustymabe previously approved these changes Jun 14, 2022
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM - one suggestion

- cmd-remote-build-container allows cosa builds using podman remote

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM - we can do later fixups in a followup if the workflow changes.

@dustymabe dustymabe enabled auto-merge (rebase) June 15, 2022 15:48
@dustymabe
Copy link
Member

/retest

2 similar comments
@ravanelli
Copy link
Member Author

/retest

@ravanelli
Copy link
Member Author

/retest

@dustymabe dustymabe merged commit 7d74b25 into coreos:main Jun 21, 2022
@ravanelli ravanelli deleted the podman branch December 5, 2022 13:42
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

5 participants