More logical buildsystem #19

praiskup · 2017-07-27T09:28:56Z

Use more standardized "make" workflow

From now we don't blindly squash image if there are no changes
(aka docker build was no-op). We also remove old (unreferenced
images if those are to be replaced by new images.

`make` or `make build`
    Builds the images.

`make tag`
    Tag built images (depends on `make build`).

`make test`
    Tests the :latest image (build if necessary).  This depends on
    `make tag` because the existing test-cases out in the wild
    depend on tagged images.  The most importantly the s2i tool
    doesn't seem to work with $IMAGE_ID only.

The make tag also tags the image with :squashed or :raw suffixes,
while the :latest image points (only) to one of them.

pkubatrh · 2017-07-31T11:19:53Z

All in all I really like the changes from the point of how everything is handled by make dependencies and I was considering to try writing something similar as this myself but unfortunately I am not a great make-magician as of yet.
+1 for build-serial! That is a really nice way to make sure we are not running in parallel by default and seems worthwhile to merge even by itself.

As for the qualms I have about this:
I think skipping the build stage when a build is already done can be omitted. It definitely is proper make work-flow but does not make much sense in the docker world since a build of an existing image is nearly instantaneous due to the docker cache. At least when the images are not squashed, which users will not do anyway if they are concerned for how much of time is spent on the build process.
Removing the build stage skip would make common.mk a bit easier to read as there would not be a reason to generate dependencies on each of the files inside the images' respective directories.
The issue then would be how to modify the makefile while making sure the work-flow (which is one of the best points about this change imo) stays the same, or at least very similar...

pkubatrh · 2017-07-31T11:33:38Z

One additional thing - using image's ID as the content of the IMAGE_NAME variable used inside tests might break said tests for some images that rely on it being a name string that can be parsed properly (I have implemented something like this in thermostat16* tests - sclorg/thermostat-container@a2c6dc3#diff-8c6c67279d694dec8dcd75f8e276c4d9R34)

omron93 · 2017-08-03T07:10:53Z

One additional thing - using image's ID as the content of the IMAGE_NAME variable used inside tests might break said tests for some images that rely on it being a name string that can be parsed properly (I have implemented something like this in thermostat16* tests - sclorg/thermostat-container@a2c6dc3#diff-8c6c67279d694dec8dcd75f8e276c4d9R34)

Also it is not possible for s2i based images. s2i build works only with tags and it is not possible to use image ID !

The permissions/ownership of the /dev/pts/* file (symlinked from /dev/fd/$fd) is uncertain, so we can not rely on the fact that we can write to this file (happened to me when I ssh as 'root' to my RHEL7 testing box and then I 'sudo su - foo'). Since we already depend on Bash features extensively, let's rather let the shell allocate a temporary fifo file with >(cmd) feature. While we are on it, separate the parsing method into self-standing method which can be easily reused, and allow parsing also standard error output (will be needed at least in sclorg#19).

The permissions/ownership of the /dev/pts/* file (symlinked from /dev/fd/$fd) is uncertain, so we can not rely on the fact that we can write to this file (happened to me when I ssh as 'root' to my RHEL7 testing box and then I 'sudo su - foo'). Since we already depend on Bash features extensively, let's rather let the shell allocate a temporary fifo file with >(cmd) feature and redirect to a shell file descriptor number by 'cat - >&$fd'. While we are on it, separate the parsing method into self-standing method which can be easily reused, and allow parsing also standard error output (will be needed at least in #19). Fixes #21.

praiskup · 2017-08-18T16:16:45Z

Thanks a lot for the comments. So I changed the semantics a bit to take your notes into account. Now, when doing make test the images are automatically tagged (TBH I don't claim that it is ideal approach, but at least for s2i tests we need IMAGE_NAME tag so that's sort of compromise). When doing plain make, images are still only built (not tagged).

IMO this is still beneficial:

most importantly, we don't squash unless really necessary (aka. there's new unsquashed image)
we don't spread untagged images (only if you interrupt the build in the middle of the build)
make clean removes both squashed/non-squashed image

omron93 · 2017-08-24T08:28:21Z

build.sh

+{
+  local base squashed_from squashed= unsquashed=$IMAGE_ID
+  test "$SKIP_SQUASH" = 1 && return 0
+
  # FIXME: We have to use the exact versions here to avoid Docker client
  #        compatibility issues
  local squash_version=1.0.5


@praiskup I vote for not checking for exact docker_squash version.

Imagine a situation when we decides to change this version. We/user will have to update docker_squash on testing machine to make similar PR tests to pass.
But since other images won't use newest version of container-common-scripts its PRs will fail.

So common-scripts in images will have to updated all in once and all other PRs will fail till that time.

@omron93 wrote:

I vote for not checking for exact docker_squash version.

+1. I was not brave enough to drop that ... since this hack was important enough to be fixed many, many times ... So the question is whether we really care about the version or not. Maybe there should be version >= 1.0.5, but IMO this deserves separate PR and discussion.

Imagine a situation when we decides to change this version. We/user will have to update docker_squash on testing machine to make similar PR tests to pass.

Right, but IMO it is not that huge issue, since we maintain the CI. I don't think user will be somewhat bothered by this.

But since other images won't use newest version of container-common-scripts its PRs will fail.

Agreed. IMO in such case it would be the right time to update common in the remote repos; and yes -> I don't claim that such hacks are not expensive.

So, since the version check was already merged and is not the part of this PR, what about to remove that later (and let others ack this?)?

So, since the version check was already merged and is not the part of this PR, what about to remove that later (and let others ack this?)?

You are right

omron93 · 2017-08-24T08:31:07Z

when doing make test the images are automatically tagged (TBH I don't claim that it is ideal approach, but at least for s2i tests we need IMAGE_NAME tag so that's sort of compromise)

@praiskup Or if you would like to avoid this, some "random" image name can be generated for tests only and removed after.

Overall, it looks very good. Thanks.

praiskup · 2017-08-24T10:15:31Z

@praiskup Or if you would like to avoid this, some "random" image name can be generated for tests only and removed after.

Can be, but it brings some complexity -- and I'm not good enough with s2i testing, so I would rather delay this for after-PR time :) if any. Even as-is, the PR somewhat improves the resource management (major motivation for this PR) so I'm less motivated to solve more problems now.

pkubatrh · 2017-08-28T12:15:10Z

common.mk

 	VERSIONS="$(VERSIONS)" $(script_env) $(test)
-	VERSIONS="$(VERSIONS)" $(script_env) $(tag)

 .PHONY: test-openshift
 test-openshift: script_env += TEST_OPENSHIFT_MODE=true
 test-openshift: $(VERSIONS)


imo test-openshift should depend on tag as well

right, good catch

pkubatrh · 2017-08-28T12:19:01Z

otherwise lgtm

From now we don't blindly squash image if there are no changes (aka docker build was no-op). We also remove old (unreferenced images if those are to be replaced by new images. `make` or `make build` Builds the images. `make tag` Tag built images (depends on `make build`). `make test` Tests the :latest image (build if necessary). This depends on `make tag` because the existing test-cases out in the wild depend on tagged images. The most importantly the s2i tool doesn't seem to work with $IMAGE_ID only.

This helps on builders where umask is e.g. 0077.

It is much more standard to use 'make check' than 'make test'; but let's keep them both (aliases) for compatibility.

praiskup · 2017-08-28T14:06:14Z

Fixed the test-openshift target, and build-alltarget so it doesn't print not-right-now-built images. Since we have basically 2xLGTM now, I'm merging this.

praiskup mentioned this pull request Aug 18, 2017

build.sh: fix /dev/pts/ permission issues #21

Closed

praiskup force-pushed the more-logical-buildsystem branch from 7355830 to 0ce4a5c Compare August 18, 2017 16:09

praiskup force-pushed the more-logical-buildsystem branch 2 times, most recently from 9c76d50 to 5aca0b5 Compare August 23, 2017 17:42

omron93 reviewed Aug 24, 2017

View reviewed changes

praiskup mentioned this pull request Aug 24, 2017

don't even check for docker_squash version #26

Closed

pkubatrh reviewed Aug 28, 2017

View reviewed changes

praiskup added 5 commits August 28, 2017 14:25

By default squash only RHEL image

4e44920

Ensure the generated help.1 is world-readable

649be82

This helps on builders where umask is e.g. 0077.

Drop TAG_ON_SUCCESS and *-candidate prefixes

b26a080

tests: add 'make check' target

cf4053a

It is much more standard to use 'make check' than 'make test'; but let's keep them both (aliases) for compatibility.

praiskup force-pushed the more-logical-buildsystem branch from 5aca0b5 to cf4053a Compare August 28, 2017 12:25

praiskup merged commit 92c8e45 into sclorg:master Aug 28, 2017

praiskup mentioned this pull request Sep 21, 2017

Image does not get tagged after build #34

Closed

praiskup deleted the more-logical-buildsystem branch March 28, 2018 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More logical buildsystem #19

More logical buildsystem #19

praiskup commented Jul 27, 2017 •

edited

Loading

pkubatrh commented Jul 31, 2017

pkubatrh commented Jul 31, 2017

omron93 commented Aug 3, 2017

praiskup commented Aug 18, 2017

omron93 Aug 24, 2017

praiskup Aug 24, 2017

omron93 Aug 24, 2017

omron93 commented Aug 24, 2017

praiskup commented Aug 24, 2017

pkubatrh Aug 28, 2017

praiskup Aug 28, 2017

pkubatrh commented Aug 28, 2017

praiskup commented Aug 28, 2017

More logical buildsystem #19

More logical buildsystem #19

Conversation

praiskup commented Jul 27, 2017 • edited Loading

pkubatrh commented Jul 31, 2017

pkubatrh commented Jul 31, 2017

omron93 commented Aug 3, 2017

praiskup commented Aug 18, 2017

omron93 Aug 24, 2017

Choose a reason for hiding this comment

praiskup Aug 24, 2017

Choose a reason for hiding this comment

omron93 Aug 24, 2017

Choose a reason for hiding this comment

omron93 commented Aug 24, 2017

praiskup commented Aug 24, 2017

pkubatrh Aug 28, 2017

Choose a reason for hiding this comment

praiskup Aug 28, 2017

Choose a reason for hiding this comment

pkubatrh commented Aug 28, 2017

praiskup commented Aug 28, 2017

praiskup commented Jul 27, 2017 •

edited

Loading