Distgen - support for image generation #38

dhodovsk · 2017-11-21T15:26:13Z

This PR was created as reaction to suggestions in sclorg/postgresql-container#203. It should be an initial effort in using distgen in image repositories.

Notes:

The image repository that uses make distgen should contain:
- specs/multispec.yml file
- manifest.sh - lists of files to generate, files to copy (including desired mode) and list of symlinks
Results are stored in current directory when not specified in DESTDIR in manifest

praiskup · 2017-11-21T15:43:57Z

distgen.sh

+cd templates
+OUTPUT_DIR=$(mktemp -d ${WORKDIR}/${OS}-${VERSION}-XXXX)
+echo "Copying files to ${OUTPUT_DIR}:"
+for file in ${FILES_TO_COPY}; do


I would really prefer having this solved properly through Makefile targets, so
everything is fully parallelized (even this cp && chmod thingy, but especially
the $FILES_TO_GEN bellow).

Thanks for the chmod call btw.! Could we made that "optional" with some sane
default (say 644)? Btw., this could solve #3 pretty nicely :-).

praiskup · 2017-11-21T15:50:13Z

distgen.sh

+#   FILES_TO_GEN in form similar to FILES_TO_COPY
+#   INTERNAL_SYMLINKS in form "name1->target1 name1->target1..."
+if [ -f "./dg_manifest" ]; then
+    source ./dg_manifest


Since manifest file is shell script, could we rename it to dg_manifest.sh (or rather remove the dg_ prefix, as that's not related to distgen) and generate some Makefile snippet from this automatically? The snippet could be then included.

I sort of miss the possibility to relocate the file (e.g you can not copy/symlink one source file onto two places), note that the quite old proposal here https://github.com/devexp-db/cont-postgresql/blob/master/cl-manifest allows this to be specified.

praiskup · 2017-11-21T15:53:34Z

distgen.sh

+# expecting file dg_manifest that contains variables:
+#   FILES_TO_COPY in form "path1:mode1 path2:mode2..."
+#   FILES_TO_GEN in form similar to FILES_TO_COPY
+#   INTERNAL_SYMLINKS in form "name1->target1 name1->target1..."


Nit, but wouldn't be better to use colon as a separator, too?

praiskup · 2017-11-21T15:57:05Z

distgen.sh

+
+echo "Generating files:"
+for file in ${FILES_TO_GEN}; do
+    split=(${file//:/ })


This is not safe for spaces, better would be "old_IFS=$IFS ; IFS=: ; set -- $file; ... use $1/$2 ... IFS=$old_IFS.

praiskup · 2017-11-21T15:57:27Z

distgen.sh

+    mode=${split[1]}
+
+    if [ ! -d $(dirname ${OUTPUT_DIR}/$path) ]; then
+        mkdir -p dirname ${OUTPUT_DIR}/$path


shell quotes missing

praiskup · 2017-11-21T16:00:09Z

distgen.sh

+
+    # With max-passes > 1 is this directive non-effective
+    if grep "{%- raw %}" $path > /dev/null; then
+        max_passes=1


Can you elaborate on this :-), I don't understand much what you try to achieve here.... The grep eats another fork, and is incomplete (one could use {% raw %} or {%- raw -%}).

Problem description:

some template files could contain some chars that are in fact jinja directives - e.g bash variable length vs. jinja comment - ${#myvar}

the directive {% raw %}/{% endraw %} fixes this problem, but the file can't be processed more than once

while using make distgen we don't know whether template needs to be processed multiple times or just once.

Possible solutions
1,Add max-passes to manifest in form

# path:max-passes:mode ./Dockerfile:2:644 ./script.sh::755

with 1 as default - this could add complexity to UX

2, Expect jinja2.exceptions.TemplateSyntaxError - could be problem when there is an actual error

3, ... (please add your suggestion)

@praiskup @bkabrda what do you think?

I actually already changed the mechanism of rerendering to not rerender the whole template, but just the spec values [1], which means this shouldn't be an issue going forward. I can do the release today and that would effectively make this problem go away, IIUC. @dhodovsk WDYT?

[1] devexp-db/distgen#63

@bkabrda I think it is great, would solve this issue nicely and also speed up the generation.

I released distgen 0.20 which should fix this.

praiskup · 2017-12-01T11:27:37Z

[test]

dhodovsk · 2017-12-08T14:55:49Z

@praiskup After our discussion I changed a bit the desired look of manifest file, so it is more readable. Please, let me know if you think it is ok.
Example can be found here: https://github.com/dhodovsk/postgresql-container/blob/ecd036610006bf9af4b0878e145273bfee51a15d/manifest.sh

hhorak · 2018-01-04T10:59:05Z

@praiskup I don't see any other issues with the current status, but I never paid big attention into this distgen, so I'll keep this to you.. WDYT?

praiskup · 2018-01-04T11:26:00Z

IMO we got much more talkative version of the manifest file than originally proposed variant https://github.com/devexp-db/cont-postgresql/blob/master/cl-manifest, but I don't mind in particular - hopefully that manifest format can be attractive enough, and we don't know until we try that..

So more importantly - as this particular PR is just implementation detail - could we vote in sclorg/postgresql-container#203 whether the format is generally OK?

TomasTomecek · 2018-01-08T13:24:32Z

common.mk

 else ifeq ($(TARGET),fedora)
 	OS := fedora
 	DOCKERFILE ?= Dockerfile.fedora
+	DG_CONF := fedora-26-x86_64.yaml


nit: Fedora 27 would be lovely as a start

hhorak · 2018-01-17T07:29:49Z

I've taken a look again and found nothing suspicious. So, I'd say let's give it a try and see how it flies.

dhodovsk · 2018-01-17T07:54:47Z

The implementation generates Dockerfile for desired TARGET without postfix. There has been requirement to generate Dockerfiles for all targets (Dockerfile, Dockerfile.rhel, Dockerfile.fedora) no matter what is in TARGET. That would also fix incorrect behaviour with two subsequent generations and change of target:

$ make generate TARGET=centos7 -> correctly generates centos7 Dockerfile
$ make generate TARGET=rhel7 -> doesn't do anything due to Dockerfile created in previous run (and no change in manifest or source files)

I postponed working on this due to change of priorities. In case of need I can change it back.

praiskup · 2018-01-17T07:57:48Z

[test]

pkubatrh · 2018-01-19T06:36:17Z

There has been requirement to generate Dockerfiles for all targets (Dockerfile, Dockerfile.rhel, Dockerfile.fedora) no matter what is in TARGET.

I say keep the functionality of generating only a single version of Dockerfile when TARGET is set. Adding a new makefile target (generate-all?) that creates all versions (TARGETs) available would be nice.

Both use cases still need the postfixed Dockerfiles though.

pkubatrh · 2018-01-19T06:45:10Z

[test]

omron93 · 2018-01-22T10:42:41Z

generate.sh

+for version in ${VERSIONS}; do
+    # copy targets
+    rules="$COPY_RULES"
+    core="cp \$< \$@ ; \\"


So it won't be possible to copy directories?

@omron93 It is not possible now, one has to specify all files that are being generated.

dhodovsk · 2018-02-14T13:41:57Z

@pkubatrh Can we expect that all files in directories except of Dockerfile are not affected by distribution change? When running generate-all for one version, one directory with all Dockerfiles would be present?

omron93 · 2018-02-14T14:30:13Z

Can we expect that all files in directories except of Dockerfile are not affected by distribution change? When running generate-all for one version, one directory with all Dockerfiles would be present?

README ?

dhodovsk · 2018-02-14T14:54:43Z

@omron93 I am asking because AFAIK e.g for posgres there is one set of files for every version and they are os agnostic. Based on your reaction there are distro specific files. How do you handle these kinds of files, do you have specific directories for version and distro, e.g rhel-nginx-1.2?

omron93 · 2018-02-15T13:18:42Z

Based on your reaction there are distro specific files. How do you handle these kinds of files, do you have specific directories for version and distro, e.g rhel-nginx-1.2?

Name of the image is distro specific. So for examples in README could be distro specific...

Currently we don't do this. So rhel based names are used in examples... But it would be useful to be able to generate right names. But it is nothing which have to be implemented now.

dhodovsk · 2018-02-15T14:27:47Z

So now the desired state is for example in postgres:

make generate VERSIONS=9.6 TARGET=rhel7 -> generates one directory: 9.6 with one Dockerfile, called Dockerfile.rhel (+ all other files)
make generate -> generates 3 directories and in each of them is one Dockerfile without postfix - since the target is default - centos
make generate-all VERSIONS=9.6 -> generates one directory containing Dockerfile, Dockerfile.rhel and Dockerfile.fedora

Is that correct, @omron93, @pkubatrh ?

pkubatrh · 2018-02-16T07:16:16Z

Also make generate-all should generate directories for all versions containing all Dockerfiles.

It should be easy to configure which suffix each of the Dockerfiles will have. For example right now we are using rhel7 for our rhel images (since they are based on rhel7) but we might want to have a different extension for future rhel images.

dhodovsk · 2018-02-20T14:08:18Z

Requested changes are now applied. What has been changed for image repository maintainer is that desired distro specific Dockerfile dest needs to be added to manifest.

Before:

...
DISTGEN_RULES="
    src=src/Dockerfile
    dest=Dockerfile;

    src=src/cccp.yml
    ...

After:

...
DISTGEN_RULES="
    src=src/Dockerfile
    dest=Dockerfile;

    src=src/Dockerfile
    dest=Dockerfile.rhel7;

    src=src/cccp.yml
    ...

The Dockerfile dest will be generated only when related TARGET is provided or by using generate-all.

praiskup · 2018-02-22T12:07:21Z

[test]

phracek · 2018-02-22T12:12:53Z

thx, please merge it. CI will be setup by the other issue, so we do not block the issue.

pvalena · 2018-02-22T12:24:58Z

common.mk

@@ -41,6 +44,7 @@ script_env = \
 	OPENSHIFT_NAMESPACES="$(OPENSHIFT_NAMESPACES)"  \
 	CUSTOM_REPO="$(CUSTOM_REPO)"

+


Formatting change, just saying....

pvalena · 2018-02-22T12:26:17Z

gen.mk

+	rm auto_targets.mk
+
+
+auto_targets.mk: $(generator) $(MANIFEST_FILE)


Why this one has .mk at the end? Please forgive my ignorance...

Np, auto_targets.mk is generated makefile containing rules for all targets to be created.

In this case it's not referring the file though, right? Sure, it gets included, but where this is called from?

It is. gen depends on auto_targets.mk which in turn depends on generator.sh in which the actual file is generated.

pkubatrh · 2018-02-22T12:26:59Z

Would it be possible to remove the auto_targets.mk file in case of a failure? Or maybe just clean it up inside make clean.

dhodovsk · 2018-02-22T12:29:25Z

@pkubatrh ofc, on it :)

pvalena · 2018-02-22T12:33:55Z

gen.mk

+endif
+
+generator = $(common_dir)/generate.sh
+DISTGEN_BIN ?= /usr/bin/dg


Could you somewhere fail, or warn at least if user does not have this binary? Where does one install it from?

dnf install distgen :D

pvalena · 2018-02-22T12:37:27Z

common.mk

 else ifeq ($(TARGET),fedora)
 	OS := fedora
 	DOCKERFILE ?= Dockerfile.fedora
+	DG_CONF := fedora-27-x86_64.yaml


Whe is there := when in other cases we use ?= ?
Same for rhel7.

You are right, this should be consistent.

pvalena · 2018-02-22T12:59:53Z

Otherwise LGTM :). Good job.

dhodovsk · 2018-02-26T10:52:04Z

[test]

TomasTomecek · 2018-02-26T12:28:16Z

Hm, tests are failing :/ is the failure related to this PR?

pkubatrh · 2018-02-26T12:36:41Z

Hm, tests are failing :/ is the failure related to this PR?

Looks like the fail happens during checking for an expected failure, so does not seem related.

pkubatrh · 2018-02-26T12:46:18Z

Actually, it does not fail on the expected failure but on make clean when it tests for the existence of the auto_targets.mk file ...

pkubatrh · 2018-02-26T12:51:27Z

common.mk

@@ -79,9 +82,17 @@ tag: build

 .PHONY: clean
 clean:
+	test -f auto_targets.mk && rm auto_targets.mk


@dhodovsk Moving this line directly into the clean.sh script should get rid of the test failures.

yeas, sorry about that :)

dhodovsk · 2018-02-26T14:52:40Z

[test]

dhodovsk · 2018-02-26T16:19:59Z

[test]

omron93 · 2018-02-27T09:54:41Z

LGTM. Thanks for this PR.

praiskup · 2018-02-27T10:19:17Z

Thanks!

TomasTomecek · 2018-02-27T10:31:58Z

praiskup reviewed Nov 21, 2017

View reviewed changes

dhodovsk force-pushed the distgen branch 3 times, most recently from dfa60ca to 38b12b8 Compare December 8, 2017 14:42

dhodovsk mentioned this pull request Dec 14, 2017

More distgen - directory generation sclorg/postgresql-container#203

Merged

TomasTomecek reviewed Jan 8, 2018

View reviewed changes

dhodovsk force-pushed the distgen branch from 38b12b8 to 6545ac2 Compare January 8, 2018 14:27

omron93 reviewed Jan 22, 2018

View reviewed changes

support for image generation

6236673

dhodovsk force-pushed the distgen branch from 6545ac2 to c43101c Compare February 20, 2018 12:51

dhodovsk force-pushed the distgen branch from c43101c to 81db4d6 Compare February 20, 2018 12:57

dhodovsk force-pushed the distgen branch 2 times, most recently from d9ded5b to 683bfec Compare February 20, 2018 15:09

pvalena reviewed Feb 22, 2018

View reviewed changes

dhodovsk force-pushed the distgen branch from 683bfec to 2ad301e Compare February 22, 2018 14:04

pkubatrh reviewed Feb 26, 2018

View reviewed changes

dhodovsk force-pushed the distgen branch from 2ad301e to 917b0a1 Compare February 26, 2018 14:51

Add generate-all capability

f63dc9d

dhodovsk force-pushed the distgen branch from 917b0a1 to f63dc9d Compare February 26, 2018 16:06

praiskup merged commit cef6837 into sclorg:master Feb 27, 2018

		@@ -41,6 +44,7 @@ script_env = \
		OPENSHIFT_NAMESPACES="$(OPENSHIFT_NAMESPACES)" \
		CUSTOM_REPO="$(CUSTOM_REPO)"

		rm auto_targets.mk


		auto_targets.mk: $(generator) $(MANIFEST_FILE)

Distgen - support for image generation #38

Distgen - support for image generation #38

Conversation

dhodovsk commented Nov 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup commented Dec 1, 2017

dhodovsk commented Dec 8, 2017

hhorak commented Jan 4, 2018

praiskup commented Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

hhorak commented Jan 17, 2018

dhodovsk commented Jan 17, 2018

praiskup commented Jan 17, 2018

pkubatrh commented Jan 19, 2018 • edited Loading

pkubatrh commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhodovsk commented Feb 14, 2018

omron93 commented Feb 14, 2018

dhodovsk commented Feb 14, 2018

omron93 commented Feb 15, 2018

dhodovsk commented Feb 15, 2018

pkubatrh commented Feb 16, 2018

dhodovsk commented Feb 20, 2018

praiskup commented Feb 22, 2018

phracek commented Feb 22, 2018 • 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

pkubatrh commented Feb 22, 2018

dhodovsk commented Feb 22, 2018

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

pvalena commented Feb 22, 2018

dhodovsk commented Feb 26, 2018

TomasTomecek commented Feb 26, 2018

pkubatrh commented Feb 26, 2018

pkubatrh commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhodovsk commented Feb 26, 2018

dhodovsk commented Feb 26, 2018

omron93 commented Feb 27, 2018

praiskup commented Feb 27, 2018

TomasTomecek commented Feb 27, 2018

dhodovsk commented Nov 21, 2017 •

edited

Loading

praiskup commented Jan 4, 2018 •

edited

Loading

pkubatrh commented Jan 19, 2018 •

edited

Loading

phracek commented Feb 22, 2018 •

edited

Loading