(feat): s2i build using local code #452

surajssd · 2017-11-15T10:33:21Z

Now user can provide just the image name and code directory and kedge
build will create the configs and trigger binary build using openshift
s2i source build strategy.

surajssd · 2017-11-15T10:38:41Z

Try out running following command in https://github.com/surajssd/localbuild

$ kedge build --s2i --image mypythonapp
INFO[0000] Creating buildconfig and imagestream         
    * A Docker build using binary input will be created
      * The resulting image will be pushed to image stream "mypythonapp:latest"
      * A binary build was created, use 'start-build --from-dir' to trigger a new build

--> Creating resources with label build=mypythonapp ...
    imagestream "mypythonapp" created
    buildconfig "mypythonapp" created
--> Success
INFO[0000] Starting build for "mypythonapp"             
build "mypythonapp-1" started
INFO[0020] To trigger the build again, just run:
oc start-build mypythonapp --from-dir=.

surajssd · 2017-11-15T10:48:43Z

@kadel for kedge I tried using the builder image you have built for golang but it did not work, so ran following command:

$ kedge build --image kedge --s2i --builder-image tomkral/go-17-centos7 
INFO[0000] Creating buildconfig and imagestream         
    * A Docker build using binary input will be created
      * The resulting image will be pushed to image stream "kedge:latest"
      * A binary build was created, use 'start-build --from-dir' to trigger a new build

--> Creating resources with label build=kedge ...
    imagestream "kedge" created
    buildconfig "kedge" created
--> Success
INFO[0004] Starting build for "kedge"                   
build "kedge-1" started
INFO[0055] To trigger the build again, just run:
oc start-build kedge --from-dir=.

but the build failed as:

$ oc logs kedge-1-build
Receiving source from STDIN as archive ...
error: build error: open /tmp/docker-build203164960/Dockerfile: no such file or directory

kadel

Try out running following command in https://github.com/surajssd/localbuild

@surajssd look at the output of that command, it says * A Docker build using binary input will be created. This is a Docker build and not S2I build

$ kedge build --image kedge --s2i --builder-image tomkral/go-17-centos7

that image doesn't exist. it should be quay.io/tomkral/go-17-centos7:skip-download-when-vendored

kadel · 2017-11-15T18:08:21Z

cmd/build.go

@@ -51,6 +59,8 @@ func init() {
 	buildCmd.Flags().StringVarP(&DockerImage, "image", "i", "", "Image name and tag of resulting image")
 	buildCmd.Flags().StringVarP(&DockerContext, "context", "c", ".", "Path to a directory containing a Dockerfile, it is build context that is sent to the Docker daemon")
 	buildCmd.Flags().BoolVarP(&PushImage, "push", "p", false, "Add this flag if you want to push the image")
+	buildCmd.Flags().BoolVarP(&s2iBuild, "s2i", "", false, "If this is enabled then BuildConfig and Imagestream is created")
+	buildCmd.Flags().StringVarP(&BuilderImage, "builder-image", "b", "", "Name of a Docker image to use as a builder")


this should say that it is just s2i option

can you please update description to note that this is only s2i option

kadel · 2017-11-15T18:09:37Z

cmd/build.go

@@ -51,6 +59,8 @@ func init() {
 	buildCmd.Flags().StringVarP(&DockerImage, "image", "i", "", "Image name and tag of resulting image")
 	buildCmd.Flags().StringVarP(&DockerContext, "context", "c", ".", "Path to a directory containing a Dockerfile, it is build context that is sent to the Docker daemon")
 	buildCmd.Flags().BoolVarP(&PushImage, "push", "p", false, "Add this flag if you want to push the image")
+	buildCmd.Flags().BoolVarP(&s2iBuild, "s2i", "", false, "If this is enabled then BuildConfig and Imagestream is created")


This should say that it will use S2I, If BuildConfing is created it doesn't mean that S2I will be used for building

please update description. It should mention that it will create BuildConfig with SourceStragegy

kadel · 2017-11-15T18:24:32Z

pkg/build/build.go

+	// Generate the buildconfig and imagestream
+	log.Info("Creating buildconfig and imagestream")
+	if builderImage == "" {
+		cmd = []string{"oc", "new-build", "--binary", "--name=" + image}


This command will create BuildConfig with DockerStrategy so no S2I :-(

It might be good to do a small research first.

If you do oc new-build --binary without builder image (--docker-image) that it will always create BC with DockerStrategy, at least this is what I saw.

You can provide a directory to oc new-build and it might crate BC with SourceStrategy (s2i) in some cases, or even jenkinsPipelineStrategy if it detects Jenkinsfile in the directory. If directory is in git that it will add git remote as source :-( So far I couldn't figure out the way to force binary build with SourceStrategy using oc new-build

there is a --strategy option that allows you to force SourceStragy, but than you can't use binary build

If we can't find a way to do builder image autodetection with binary builds than I think that we can require manually selecting builder image. It will be the first step for S2I and than we can figure out autodetection later

@kadel we can do one thing, just start the normal build and immediately trigger it using the source in local directory:

oc new-build . --name=pythonapp --strategy=source oc start-build pythonapp --from-dir=.

So we are achieving similar results as the binary build, we are triggering the build using the code provided locally.

Not sure that this is a good idea. BuildConfig will be created with git as a source.
If someone triggers BuildConfig build without source-dir it will build from git repository :-( and that might not be what they intended. Also when I tried that the first build from git was still triggered and finished, so we would have to cancel it before it finished. Only the build form local dir should finish successfully.

I'm not sure if there is a good solution if we want autodetection of builder image :-(
Have you checked how that code looks like in oc? Maybe we could just use autodetection part from it, and create BC ourselves.

surajssd · 2017-11-20T16:03:25Z

Now there is a weird error that I am facing with this one:

cd /tmp && \
git clone https://github.com/surajssd/localbuild && \
cd localbuild && \
rm -rf .git Dockerfile && \
kedge build --s2i --image pyappth -b centos/python-35-centos7:3.5

For some reason it says that the imagestream is not valid, but the same thing if I try with raw oc commands it works just fine.

for cleanup run:

oc delete all --all && \
rm -rf /tmp/localbuild

kadel · 2017-11-21T09:29:25Z

That is strange. It worked for me and I did exactly the same steps.

kadel · 2017-11-21T09:35:12Z

That is strange. It worked for me and I did exactly the same steps.

ok, I got the following error when I tried to do the second build:

--> Failed
error: imagestreams.image.openshift.io "python-35-centos7" already exists
    error: imagestreams.image.openshift.io "pyappth" already exists
    error: buildconfigs.build.openshift.io "pyappth" already exists, exit status 1

It looks like it is because ImageStream for builder image already exists, it should be possible to refer to the IS somehow.

Users should be able to trigger build again without requiring to delete builder image IS

surajssd · 2017-11-21T10:04:28Z

@containscafeine can you PTAL ?

surajssd · 2017-11-21T10:06:11Z

@kadel

It looks like it is because ImageStream for builder image already exists, it should be possible to refer to the IS somehow.

the IS thatt is shown to be existing was generated by previously run kedge build what do you mean by refer to existing imagestream here?

kadel · 2017-11-21T10:21:16Z

the IS thatt is shown to be existing was generated by previously run kedge build what do you mean by refer to existing imagestream here?

Case where I want build another project, but with the same builder image, that is broken right now :-(

kadel · 2017-11-21T10:25:40Z

the IS thatt is shown to be existing was generated by previously run kedge build what do you mean by refer to existing imagestream here?
Case where I want build another project, but with the same builder image, that is broken right now :-(

If builder image was already used for another project that it fails with error: imagestreams.image.openshift.io "python-35-centos7" already exists

it that case you have to use --image-stream instead of --builder-image when running oc new-build command (oc new-build --binary --name=pyapp --image-stream=myproject/python-35-centos7:3.5)

We have to solve this somehow.

concaf · 2017-11-21T10:27:29Z

@surajssd running the steps in #452 (comment), I'm getting the same error -
The ImageStreamTag "python-35-centos7:3.5" is invalid: from: Error resolving ImageStreamTag python-35-centos7:3.5 in namespace stest: unable to find latest tagged image, exit status 1

kadel · 2017-11-21T12:47:14Z

@surajssd running the steps in #452 (comment), I'm getting the same error -
The ImageStreamTag "python-35-centos7:3.5" is invalid: from: Error resolving ImageStreamTag python-35-centos7:3.5 in namespace stest: unable to find latest tagged image, exit status 1

I can't reproduce it :-(

kadel · 2017-11-21T12:55:23Z

cmd/build.go

-			fmt.Println(err)
-			os.Exit(-1)
+
+		if s2iBuild {


There should be a check for --push option. Push doesn't make sense with s2i

Yeah --push does not make any sense because it is happening anyway. Can we not just ignore it?

Because there is no point in erroring out or anything?

We can't just silently ignore it. Kedge should tell the user that he is trying to do something that doesn't make sense.

I would error out, but showing a warning that --push flag will be ignored is also fine.

Help message for --push should say that it is just for Docker build

kadel · 2017-11-21T13:07:04Z

pkg/build/build.go

+
+	// start the first build since it won't be triggered automatically
+	log.Infof("Starting build for %q", image)
+	cmd = []string{"oc", "start-build", image, "--from-dir=" + context}


add --follow to show logs from build

kadel · 2017-11-21T14:49:39Z

It looks like the problem is that oc new-build is automatically creating ImageStram for builder image.

Lets ditch oc new-app and create BuildConfig manually as shown in #309 (comment)

Important thing is that builder docker image is defined with kind: DockerImage

surajssd · 2017-11-22T10:16:12Z

@kadel PTAL, ditched oc new-build now, generating configs by hand and then feeding it to oc!

surajssd · 2017-11-23T09:00:52Z

@cdrage @containscafeine can you give this PR a try once?

concaf · 2017-11-27T10:59:44Z

Instead of --s2i flag, I think a flag like --strategy s2i makes more sense. If this flag is not set, then we can default to the docker build strategy. Having a boolean flag for the strategy seems a bit unintuitive to me.
When a build fails, I expect to fix the error and run kedge build again with s2i, but I get an error that imagestream already exists in the cluster. I think, in case of an error, kedge should do a cleanup before exiting. Or, there should be a --retry option for kedge build which removes the existing artifacts in the cluster before trying to do a new build. Or, there should be a --cleanup for kedge build which cleans up the build artifacts from the cluster before coming out of it.
Basically, the following error should not occur -

$ kedge build -i testoutputimage --s2i -b testbuilderimage
imagestream "testoutputimage" created
buildconfig "testoutputimage" created
INFO[0000] Starting build for "testoutputimage"         
build "testoutputimage-1" started
Receiving source from STDIN as archive ...
pulling image error : Error: image library/testbuilderimage:latest not found
error: build error: unable to get testbuilderimage:latest
Uploading directory "." as binary input for the build ...
WARNING: the provided file may not be an archive (tar, tar.gz, or zip), use --from-file to prevent extraction
error: the build s2i/testoutputimage-1 status is "Failed", exit status 1

$ kedge build -i testoutputimage --s2i -b testbuilderimage
Error from server (AlreadyExists): error when creating "STDIN": imagestreams.image.openshift.io "testoutputimage" already exists
failed to execute command: exit status 1

Add an s2i example

kadel · 2017-11-27T11:04:39Z

When a build fails, I expect to fix the error and run kedge build again with s2i, but I get an error that imagestream already exists in the cluster. I think, in case of an error, kedge should do a cleanup before exiting. Or, there should be a --retry option for kedge build which removes the existing artifacts in the cluster before trying to do a new build. Or, there should be a --cleanup for kedge build which cleans up the build artifacts from the cluster before coming out of it.

It has to be possible to run build multiple in the row, without providing any additional flags.

surajssd · 2017-11-28T11:38:52Z

@containscafeine I have added a cleanup function which is called when there are errors while creating those configs

@kadel

It has to be possible to run build multiple in the row, without providing any additional flags.

done

surajssd · 2017-11-28T11:41:05Z

@containscafeine

Instead of --s2i flag, I think a flag like --strategy s2i makes more sense. If this flag is not set, then we can default to the docker build strategy. Having a boolean flag for the strategy seems a bit unintuitive to me.

I am not sure about the whole fallback you are suggesting of how helpful it would be. Because how do you assume that someone who is trying to using s2i will have Dockerfile in the repo for Docker strategy to work?

surajssd · 2017-11-28T11:41:50Z

I have no comment about the whole conversion of s2i flag from boolean to string, if we agree I can change that, but let's agree on it.

@kadel WDYT? cc: @cdrage

kadel · 2017-11-28T11:49:41Z

cmd/build.go

-			fmt.Println(err)
-			os.Exit(-1)
+
+		if s2iBuild {


We can't just silently ignore it. Kedge should tell the user that he is trying to do something that doesn't make sense.

I would error out, but showing a warning that --push flag will be ignored is also fine.

kadel · 2017-11-28T11:53:49Z

pkg/build/build.go

+func BuildS2I(image, context, builderImage string) error {
+
+	labels := map[string]string{
+		"build": GetImageName(image),


it would be nice to have build as constant in spec package similar to other labels and annotations
(appLabelKey,appVersion)

surajssd · 2017-11-28T12:07:58Z

@kadel done

surajnarwade · 2017-11-30T09:34:32Z

cd /tmp &&
git clone https://github.com/surajssd/localbuild &&
cd localbuild &&
rm -rf .git Dockerfile &&
kedge build --s2i --image pyappth -b centos/python-35-centos7:3.5

@surajssd, these steps now works for me :)

surajssd · 2017-12-01T10:09:03Z

cc: @kadel @containscafeine can you guys PTAL ?

kadel · 2017-12-01T13:04:58Z

There is one situation that can happen that is not nice.
I have my code, I trigger build and everything goes well, build is successful.
Now I update my code, I trigger the build again, it fails due to some code error, and imageStream that was created by previous successful build is deleted :-(

surajssd · 2017-12-11T09:35:14Z

end to end tests passed on my machine running on minikube!

$ VERBOSE=yes PARALLEL=4 make test-e2e                                                                                                                                                                       
./scripts/run_e2e.sh -p 4                                                                                                                                                                                          
======================================                                                                                                                                                                             
| Running end-to-end cluster tests.  |
...
ok      github.com/kedgeproject/kedge/tests/e2e 532.522s

kadel · 2017-12-11T10:42:57Z

This still has the same problem with deleting resources as described in #452 (comment)

kadel

The code looks good.
It would be nice to add unit tests, at least for GetImageTag and GetImageName.

kadel · 2017-12-11T12:32:10Z

pkg/build/build.go

+
+// getImageTag get tag name from image name
+// if no tag is specified return 'latest'
+func GetImageTag(image string) string {


missing unit test :-(

kadel · 2017-12-11T12:32:16Z

pkg/build/build.go

+	return "latest"
+}
+
+func GetImageName(image string) string {


missing unit test :-(

Now user can provide just the image name and code directory and kedge build will create the configs and trigger binary build using openshift s2i source build strategy.

kadel

LGTM

surajssd force-pushed the start-s2i-build branch from 23d5aa9 to a9a2339 Compare November 15, 2017 10:46

surajssd added the status/needs-review label Nov 15, 2017

surajssd requested review from kadel, cdrage and concaf November 15, 2017 13:11

kadel reviewed Nov 15, 2017

View reviewed changes

kadel suggested changes Nov 15, 2017

View reviewed changes

surajssd force-pushed the start-s2i-build branch 2 times, most recently from af903d5 to 98c7291 Compare November 20, 2017 15:57

kadel reviewed Nov 21, 2017

View reviewed changes

surajssd force-pushed the start-s2i-build branch from b99a28f to 3bfc8ec Compare November 22, 2017 09:52

surajssd force-pushed the start-s2i-build branch from 3bfc8ec to 4748ced Compare November 22, 2017 11:31

surajssd force-pushed the start-s2i-build branch from 4748ced to 2b058e0 Compare November 28, 2017 11:37

kadel reviewed Nov 28, 2017

View reviewed changes

surajssd force-pushed the start-s2i-build branch 2 times, most recently from a13f261 to df704e0 Compare November 28, 2017 12:07

surajssd force-pushed the start-s2i-build branch from df704e0 to c842db8 Compare November 28, 2017 12:51

surajssd force-pushed the start-s2i-build branch from c842db8 to 180d33f Compare December 11, 2017 08:54

surajssd force-pushed the start-s2i-build branch from 180d33f to 09e521f Compare December 11, 2017 11:37

kadel reviewed Dec 11, 2017

View reviewed changes

(feat): s2i build using local code

65ad58e

Now user can provide just the image name and code directory and kedge build will create the configs and trigger binary build using openshift s2i source build strategy.

surajssd force-pushed the start-s2i-build branch from 09e521f to 65ad58e Compare December 11, 2017 13:33

kadel approved these changes Dec 11, 2017

View reviewed changes

kadel merged commit a91be10 into kedgeproject:master Dec 11, 2017

kadel mentioned this pull request Dec 11, 2017

Add build support of S2I build #309

Closed

(feat): s2i build using local code #452

(feat): s2i build using local code #452

Conversation

surajssd commented Nov 15, 2017

surajssd commented Nov 15, 2017

surajssd commented Nov 15, 2017

kadel left a comment • edited Loading

Choose a reason for hiding this comment

kadel Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

kadel Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd commented Nov 20, 2017

kadel commented Nov 21, 2017

kadel commented Nov 21, 2017 • edited Loading

surajssd commented Nov 21, 2017

surajssd commented Nov 21, 2017

kadel commented Nov 21, 2017

kadel commented Nov 21, 2017 • edited Loading

concaf commented Nov 21, 2017

kadel commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Nov 21, 2017 • edited Loading

surajssd commented Nov 22, 2017

surajssd commented Nov 23, 2017

concaf commented Nov 27, 2017 • edited Loading

kadel commented Nov 27, 2017

surajssd commented Nov 28, 2017

surajssd commented Nov 28, 2017

surajssd commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd commented Nov 28, 2017

surajnarwade commented Nov 30, 2017

surajssd commented Dec 1, 2017

kadel commented Dec 1, 2017

surajssd commented Dec 11, 2017

kadel commented Dec 11, 2017

kadel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel left a comment

Choose a reason for hiding this comment

kadel left a comment •

edited

Loading

kadel Nov 15, 2017 •

edited

Loading

kadel Nov 15, 2017 •

edited

Loading

kadel Nov 15, 2017 •

edited

Loading

kadel commented Nov 21, 2017 •

edited

Loading

kadel commented Nov 21, 2017 •

edited

Loading

kadel commented Nov 21, 2017 •

edited

Loading

concaf commented Nov 27, 2017 •

edited

Loading