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 podman farm build command #20050

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Conversation

umohnani8
Copy link
Member

@umohnani8 umohnani8 commented Sep 20, 2023

Add podman farm build command that sends out builds to
nodes defined in the farm, builds the images on the farm
nodes, and pulls them back to the local machine to create
a manifest list.

Signed-off-by: Urvashi Mohnani [email protected]

Does this PR introduce a user-facing change?

Add podman farm build command

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 20, 2023
@umohnani8
Copy link
Member Author

umohnani8 commented Sep 20, 2023

While I finalize the tests, opened the PR to get reviews started on it. Added no new tests needed so that the existing tests can run to ensure no regressions are happening.

Docs changes are quite a bit so opened another PR for it here: #20051

Demo of farm build: https://asciinema.org/a/V6WufoZX0Kglvlgq2YfmqZhH7

Adding hold label
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2023
@umohnani8 umohnani8 force-pushed the farm-build-2 branch 3 times, most recently from 5af332c to 3ab2a5c Compare September 20, 2023 15:30
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2023
cmd/podman/farm/build.go Outdated Show resolved Hide resolved
cmd/podman/farm/build.go Outdated Show resolved Hide resolved
pkg/farm/farm.go Outdated Show resolved Hide resolved
pkg/farm/list_builder.go Outdated Show resolved Hide resolved
pkg/farm/list_builder.go Outdated Show resolved Hide resolved
@cgwalters
Copy link
Contributor

Just happened to see this scroll by, but one thing I wanted to mention here is in the coreos pipeline we basically implement a version of this, see e.g. this code.

However, one recent pain point is that we actually want to orchestrate this process inside an OCP cluster, and the container-in-container flow needed STORAGE_DRIVER=vfs which is a well known thing. However, IMO there's no reason to actually use containers-storage for this, we should basically support using an oci directory and that has no special host requirements and should in general be a bit more efficient than deserializing the images only to reserialize them.

@cgwalters
Copy link
Contributor

I'm slightly surprised to see podman ship this FWIW, I would have said that multi-system orchestration is something we should defer to a higher level tool.

@rhatdan
Copy link
Member

rhatdan commented Sep 21, 2023

The goal is multi-arch builds. Assembling multi-arch images is a pain in the butt, we are attempting to make that easier. Especially on a MAC, where you could have two podman machines running one ARM and one x86.

@vrothberg
Copy link
Member

I'm slightly surprised to see podman ship this FWIW, I would have said that multi-system orchestration is something we should defer to a higher level tool.

Thanks for checking, @cgwalters !

As Dan mentioned, farm build is focusing on simplifying mutli-arch builds only. It's not meant to orchestrate beyond that. It boils down to maintaining a list of podman system connections along with the knowledge of the hardware platforms the machines run on.

@Luap99
Copy link
Member

Luap99 commented Oct 24, 2023

@umohnani8 I think the main problem is that you try to recreate a local engine for the local build instead of using the already existing engine interface in the client. I.e it should be possible to pass down registry.ImageEngine() to your farm package for the local build instead of trying to create your own interface instance.

@Luap99
Copy link
Member

Luap99 commented Oct 24, 2023

On the cli it should look something like this:

diff --git a/cmd/podman/farm/build.go b/cmd/podman/farm/build.go
index 6acf998fb..7ecb75baf 100644
--- a/cmd/podman/farm/build.go
+++ b/cmd/podman/farm/build.go
@@ -9,8 +9,8 @@ import (
        "github.com/containers/podman/v4/cmd/podman/common"
        "github.com/containers/podman/v4/cmd/podman/registry"
        "github.com/containers/podman/v4/cmd/podman/utils"
+       "github.com/containers/podman/v4/pkg/domain/entities"
        "github.com/containers/podman/v4/pkg/farm"
-       "github.com/containers/storage"
        "github.com/sirupsen/logrus"
        "github.com/spf13/cobra"
 )
@@ -106,17 +106,13 @@ func build(cmd *cobra.Command, args []string) error {
                defaultFarm = f
        }
 
-       var storeOptions *storage.StoreOptions
+       var localEngine entities.ImageEngine
        if buildOpts.local {
-               opts, err := storage.DefaultStoreOptionsAutoDetectUID()
-               if err != nil {
-                       return err
-               }
-               storeOptions = &opts
+               localEngine = registry.ImageEngine()
        }
 
        ctx := registry.Context()
-       farm, err := farm.NewFarm(ctx, defaultFarm, storeOptions, buildOpts.local)
+       farm, err := farm.NewFarm(ctx, defaultFarm, localEngine)
        if err != nil {
                return fmt.Errorf("initializing: %w", err)
        }

Then you need to make pkg/farm just pass around this interface instead of passing around c/storage options.
The main advantage of this is that it will actually respect the podman cli options such as --root/--runroot and not just work with the defaults.

Add emulation pkg to be used with farm build when
determining emulated platforms for the farm nodes.

Signed-off-by: Urvashi Mohnani <[email protected]>
Add podman farm build command that sends out builds to
nodes defined in the farm, builds the images on the farm
nodes, and pulls them back to the local machine to create
a manifest list.

Signed-off-by: Urvashi Mohnani <[email protected]>
Signed-off-by: Urvashi Mohnani <[email protected]>
@umohnani8
Copy link
Member Author

This is ready to be merge, can we please merge @rhatdan @nalind @vrothberg

Added remote test and the bloat for both podman and podman-remote is significantly less now.

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Test logs are disturbingly noisy. And it still bothers me that this can only be tested in CI, not by any developer in a local environment.

But enough already. It is impossible to keep reviewing this. I will take on the task of fixing the test bugs. I'm OK with merging this and then making smaller fix PRs in the future.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, umohnani8

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [edsantiago,umohnani8]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@TomSweeneyRedHat
Copy link
Member

LGTM
and I concur with @edsantiago . It's time to merge this and adjust as necessary afterwards. I'd say mark this as Tech Preview for Fedora v4.8, and then we'll do the same in RHEL 8.10/9.4 and Podman v4.9.
I'm going to let Paul, Valentin, Nalin, or Matt push the shiny red merge button.

@rhatdan
Copy link
Member

rhatdan commented Oct 25, 2023

/lgtm
/unhold

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 25, 2023
@openshift-ci openshift-ci bot merged commit 5a47b1e into containers:main Oct 25, 2023
99 checks passed
@vrothberg
Copy link
Member

vrothberg commented Oct 26, 2023

I'd say mark this as Tech Preview for Fedora v4.8, and then we'll do the same in RHEL 8.10/9.4 and Podman v4.9.

Please wait with advertising farm build. We had a meeting earlier this week and decided to merge the PR as is but there will be follow ups changing quite a bit how it's working. In other words, it's not yet ready for tech preview.

@afbjorklund
Copy link
Contributor

afbjorklund commented Oct 26, 2023

There were some questions about the new podman farm already in v4.7, where all it did was edit config files...

Usage:
  podman-remote-static farm [command]

Available Commands:
  create      Create a new farm
  list        List all existing farms
  remove      Remove one or more farms
  update      Update an existing farm

@TomSweeneyRedHat
Copy link
Member

@vrothberg I'm mostly concerned with putting the Tech Preview on Build Farm for the Podman 4.8/4.9 that lands in RHEL 8.10/9.4 in early February. Does that work for you? I'm fine with not marking it Tech Preview for Fedora at the moment while pieces/parts are worked on further.

edsantiago added a commit to edsantiago/libpod that referenced this pull request Oct 27, 2023
Followup from containers#20050. Lots of tiny problems in tests, all of
them adding up to significant maintainability problems.

These tests are currently impossible to run in a dev environment,
and super-painful to set up in 1mt, so I've just done a few hours
of cleanup and am giving up for the week.

This is ready for merge, in the sense that it's much better than
what exists now, but it still needs boatloads more work.

Signed-off-by: Ed Santiago <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Oct 27, 2023

It is not going to be ready so the commands should be hidden. We do not document it and will not ship this until podman 5.*

@TomSweeneyRedHat
Copy link
Member

@rhatdan @baude @mheon we have created Features/Epics saying Farm will be in RHEL 8.10/9.4 as Tech Preview, which would be late Jan/early Feb with Podman v4.9. Do we need to backtrack there?

@rhatdan
Copy link
Member

rhatdan commented Oct 31, 2023

I think it would be better to wait until 5.0

@cevich
Copy link
Member

cevich commented Nov 1, 2023

Respectfully, I'm going to disagree. I think this is an important feature that distinguishes podman. I could understand delaying if it's incomplete/not working in some way (is it?). Otherwise, is there really some important reason to delay getting it into the hands of users? Could we do a v4.10 for this?

@afbjorklund
Copy link
Contributor

afbjorklund commented Jan 25, 2024

Could we do a v4.10 for this?

It is now included in v4.9.0, but it is mostly undocumented...

"The podman farm suite of commands for multi-architecture builds is now fully enabled and documented."

https://docs.podman.io/en/v4.9.0/markdown/podman-farm.1.html

Especially the need to upgrade all the servers to 4.9 or above.

But also the need to deploy a registry, or configure an existing one.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Apr 28, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bloat_approved Approve a PR in which binary file size grows by over 50k lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.