Execute External Build Command #996

otaviof · 2019-09-26T10:29:49Z

Extending s2i build command to build images using an external command, in this PR we are adding buildah, docker and podman. Therefore, users can run:

s2i build app-src nginx:latest app-dst --with-builder buildah
s2i build app-src nginx:latest app-dst --with-builder docker
s2i build app-src nginx:latest app-dst --with-builder podman

The idea is to improve user experience to in a single action, generate the Dockerfile and use a external command to build the container image, therefore the build environment would allow to bring OCI toolset.

openshift-ci-robot · 2019-09-26T10:30:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: otaviof
To complete the pull request process, please assign adambkaplan
You can assign the PR to them by writing /assign @adambkaplan in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

sbose78 · 2019-09-26T16:20:51Z

Good work, let's ensure that this is tested without the docker socket active on the host.

otaviof · 2019-09-26T16:54:21Z

Good work, let's ensure that this is tested without the docker socket active on the host.

Indeed, without Docker socket exposed it was not able to reach the point of invoking the external builder. I've made a few more changes, making sure it's only calling CreateDockerfile instead of Build, and the Docker integration is not enabled either when using --as-dockerfile or --with-builder options.

otaviof · 2019-09-27T14:52:33Z

/retest

sbose78 · 2019-09-30T15:30:58Z

If a user uses,

s2i build app-src nginx:latest app-dst --with-builder docker

Let's ensure we don't return an error.

It could internally take any of the following code paths:

invoke the docker API ( as it does today ). This touches less code.
invoke the external command. I see you have https://github.com/openshift/source-to-image/pull/996/files#diff-e4469a02d9eb983856b288f828175cb8R35 :)

sbose78 · 2019-09-30T15:50:38Z

Turn off docker daemon
user does an s2i build --with-builder=buildah 💚
s2i rebuild --with-builder=buildah 🔴

(3) attempts a Pull Image using the docker API, and would therefore fail.

source-to-image/pkg/cmd/cli/cmd/rebuild.go

Lines 48 to 54 in 0b950e4

    
           client, err := docker.NewEngineAPIClient(cfg.DockerConfig) 
        
           s2ierr.CheckError(err) 
        
           dkr := docker.New(client, cfg.PullAuthentication) 
        
           pr, err := docker.GetRebuildImage(dkr, cfg) 
        
           s2ierr.CheckError(err) 
        
           err = build.GenerateConfigFromLabels(cfg, pr) 
        
           s2ierr.CheckError(err)

@bparees Do you think we should refactor all docker API calls to start using external tool invocations wherever applicable?
In this case, we need to refactor to do an inspect using skopeo.

sbose78 · 2019-09-30T16:12:22Z

Another thing:

If s2i build --with-builder=docker takes the route of using the external docker command instead of the existing docker API route, then #973 gets exposed here.

s2i build ... and s2i build --with-builder=docker should not be taking two different code paths. That could cause inconsistent behavior.

In light of the above, my preference would be to invest in :

Refactor all workflows to make all invocations using external tools ( docker / podman / buildah). When the --with-builder is not specified, we use docker build .. for now.
Fix --as-dockerfile doesn't read builder image labels labels #973 as part of that. We might use the skopeo library to inspect images here.

otaviof · 2019-10-03T12:31:45Z

If a user uses,

s2i build app-src nginx:latest app-dst --with-builder docker

Let's ensure we don't return an error.

It could internally take any of the following code paths:

* invoke the docker API ( as it does today ). This touches less code.

* invoke the external command. I see you have https://github.com/openshift/source-to-image/pull/996/files#diff-e4469a02d9eb983856b288f828175cb8R35  :)

@sbose78 Please consider latest commit. Now it support --with-builder=docker triggering the original path, so using Docker API.

sbose78 · 2019-10-03T15:07:26Z

Summarizing the conversation on slack
https://coreos.slack.com/archives/CHK22LM6Y/p1569950216332100?thread_ts=1569948800.330600&cid=CHK22LM6Y

➡️ s2i build --with-builder=docker. We invoke external tool. That's what is being done now in the PR.
➡️ Let's not support --with-builder in s2i rebuild ...., it's going to bring up other issues with image inspection. rebuild would be a deprecated feature in the next minor release.
➡️ Removing all docker API calls in valuable codepaths isn't a priority but a nice goal to have.

sbose78 · 2019-10-03T15:09:17Z

@sbose78 Please consider latest commit. Now it support --with-builder=docker triggering the original path, so using Docker API.

Not that I don't like this ;) I like it very much. However, this is opposite to what we summarized the other day :) It is fine to fall back to your older approach of calling external docker since we are no longer going to invest in rebuild.

➡️ s2i build --with-builder=docker. We invoke external tool. That's what is being done now in the PR.

pkg/build/strategies/strategies.go

otaviof · 2019-10-04T11:36:31Z

Summarizing the conversation on slack
https://coreos.slack.com/archives/CHK22LM6Y/p1569950216332100?thread_ts=1569948800.330600&cid=CHK22LM6Y

➡️ s2i build --with-builder=docker. We invoke external tool. That's what is being done now in the PR.

Done. Reverting the changes we had before, where on using --with-builder you always invoke a external tool, including docker in this case.

➡️ Let's not support --with-builder in s2i rebuild ...., it's going to bring up other issues with image inspection. rebuild would be a deprecated feature in the next minor release.

Okay. The parameter --with-builder is not introduced over there, so it's "done".

➡️ Removing all docker API calls in valuable codepaths isn't a priority but a nice goal to have.

This topic I would like to tackle in a different PR.

otaviof · 2020-02-05T08:22:13Z

/retest

otaviof · 2020-02-05T16:51:01Z

/retest

otaviof · 2020-02-07T11:07:29Z

@adambkaplan, @sbose78 the build failures are addressed, please consider.

pradeepto · 2020-02-21T15:48:08Z

@sbose78 @adambkaplan @otaviof What is pending for this PR to be merged? There is no action on this since last 14 days. Can one of you please have a look and unblock this? Thanks.

adambkaplan · 2020-02-24T20:54:24Z

test/integration/docker/with_builder_test.go

+	// expect to find generated Dockerfile with s2i extension
+	expectedFiles := []string{filepath.Join(tempdir, "Dockerfile.s2i")}
+
+	dockerfile.RunDockerfileTest(t, config, expected, nil, expectedFiles, false)


@otaviof why run the Dockerfile test here? Since this is running on our Jeninks VM, why not run an actual Docker build (just like the other docker-based tests)?

In fact, it does run the actual Docker file build, but via the --with-builder external builder command-line. Therefore, we os/exec this builder command and wait for the outcome.

otaviof · 2020-07-15T07:16:45Z

/hold

openshift-bot · 2020-10-27T19:22:36Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

adambkaplan · 2020-11-24T13:27:01Z

/lifecycle frozen

We will want to resurrect this PR once we are ready to implement the node cache feature.

coreydaley · 2022-04-27T16:32:06Z

@otaviof Is this pull request still relevant?

otaviof · 2022-04-27T16:33:49Z

@otaviof Is this pull request still relevant?

Definitely not relevant. I'm closing it.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 26, 2019

openshift-ci-robot requested review from adambkaplan and coreydaley September 26, 2019 10:30

otaviof force-pushed the issue-966-with-builder branch from 6fc2045 to 49bb5c3 Compare September 26, 2019 16:51

otaviof force-pushed the issue-966-with-builder branch 3 times, most recently from 85b75d2 to 9c5ef9c Compare September 27, 2019 14:47

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 27, 2019

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 27, 2019

bparees mentioned this pull request Oct 1, 2019

Using redhat software collections container images with podman #997

Closed

otaviof force-pushed the issue-966-with-builder branch from 09bcd14 to 5361bf6 Compare October 3, 2019 12:41

sbose78 suggested changes Oct 3, 2019

View reviewed changes

pkg/build/strategies/strategies.go Show resolved Hide resolved

otaviof force-pushed the issue-966-with-builder branch from 5361bf6 to c09948a Compare October 4, 2019 11:34

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 4, 2019

openshift-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 4, 2019

fix(dockerfile): using build directly

5392fee

otaviof force-pushed the issue-966-with-builder branch from 2714531 to e17366f Compare February 4, 2020 21:59

otaviof force-pushed the issue-966-with-builder branch 2 times, most recently from d01e585 to 61be61a Compare February 5, 2020 16:32

otaviof force-pushed the issue-966-with-builder branch 3 times, most recently from 6e465a3 to e0bfbc7 Compare February 6, 2020 05:33

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2020

otaviof force-pushed the issue-966-with-builder branch from e0bfbc7 to 7588622 Compare February 6, 2020 05:45

otaviof added 2 commits February 6, 2020 10:05

chore(test): external builder integration test.

45b4f82

feat(with-builder): Handling build responses.

1285622

otaviof force-pushed the issue-966-with-builder branch from 7588622 to 1285622 Compare February 6, 2020 09:07

chre(test): Picking true/false from PATH.

1a252fa

adambkaplan requested changes Feb 24, 2020

View reviewed changes

LaVLaS mentioned this pull request May 25, 2020

Add Makefile to build and push the JupyterHub image opendatahub-io-contrib/jupyterhub-odh#52

Closed

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2020

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 27, 2020

openshift-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 24, 2020

otaviof closed this Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execute External Build Command #996

Execute External Build Command #996

otaviof commented Sep 26, 2019 •

edited

Loading

openshift-ci-robot commented Sep 26, 2019

sbose78 commented Sep 26, 2019

otaviof commented Sep 26, 2019

otaviof commented Sep 27, 2019

sbose78 commented Sep 30, 2019 •

edited

Loading

sbose78 commented Sep 30, 2019 •

edited

Loading

sbose78 commented Sep 30, 2019 •

edited

Loading

otaviof commented Oct 3, 2019

sbose78 commented Oct 3, 2019

sbose78 commented Oct 3, 2019

otaviof commented Oct 4, 2019

otaviof commented Feb 5, 2020

otaviof commented Feb 5, 2020

otaviof commented Feb 7, 2020

pradeepto commented Feb 21, 2020

adambkaplan Feb 24, 2020

otaviof Feb 25, 2020

otaviof commented Jul 15, 2020

openshift-bot commented Oct 27, 2020

adambkaplan commented Nov 24, 2020

coreydaley commented Apr 27, 2022

otaviof commented Apr 27, 2022

Execute External Build Command #996

Execute External Build Command #996

Conversation

otaviof commented Sep 26, 2019 • edited Loading

openshift-ci-robot commented Sep 26, 2019

sbose78 commented Sep 26, 2019

otaviof commented Sep 26, 2019

otaviof commented Sep 27, 2019

sbose78 commented Sep 30, 2019 • edited Loading

sbose78 commented Sep 30, 2019 • edited Loading

sbose78 commented Sep 30, 2019 • edited Loading

otaviof commented Oct 3, 2019

sbose78 commented Oct 3, 2019

sbose78 commented Oct 3, 2019

otaviof commented Oct 4, 2019

otaviof commented Feb 5, 2020

otaviof commented Feb 5, 2020

otaviof commented Feb 7, 2020

pradeepto commented Feb 21, 2020

adambkaplan Feb 24, 2020

Choose a reason for hiding this comment

otaviof Feb 25, 2020

Choose a reason for hiding this comment

otaviof commented Jul 15, 2020

openshift-bot commented Oct 27, 2020

adambkaplan commented Nov 24, 2020

coreydaley commented Apr 27, 2022

otaviof commented Apr 27, 2022

otaviof commented Sep 26, 2019 •

edited

Loading

sbose78 commented Sep 30, 2019 •

edited

Loading

sbose78 commented Sep 30, 2019 •

edited

Loading

sbose78 commented Sep 30, 2019 •

edited

Loading