Skip to content
This repository was archived by the owner on Jun 2, 2023. It is now read-only.

Better builds + 9.0.0.3 + IHS shutdown fix #22

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

trajano
Copy link
Contributor

@trajano trajano commented May 23, 2017

Addresses #20

trajano added a commit to trajano/jee that referenced this pull request May 23, 2017
The code is written out, but mostly commented out to use static routing
until WASdev/ci.docker.ibm-http-server#22 is
merged
@davidcurrie
Copy link
Contributor

Many thanks for the PR. Firstly, we'd need you to signoff on the commits to indicate acceptance of the DCO. I'd happily take the v9.0.0.3 commit but I'm going to need help understanding the changes that you have made to the build process. For example, why does ilan/Dockerfile now contain a shebang? And the approach that you have taken seems to layer in the entire .tgz file before unpacking it which is going to create excessively large images. What are you trying to achieve with your better builds?

@trajano
Copy link
Contributor Author

trajano commented May 24, 2017

So how do I do this acceptance of the DCO?

@trajano
Copy link
Contributor Author

trajano commented May 24, 2017

If you can get the 9.0.0.3 deployed soon that would be awesome because it is blocking my builds for WebSphere Liberty dynamicRouting.

@trajano
Copy link
Contributor Author

trajano commented May 24, 2017

I added more details about the changes on the commit message.

ilan/build_all Outdated
@@ -25,4 +25,4 @@ while read line; do
if [[ $line == \#* ]]; then continue; fi
version=$(cut -d, -f1 <<< $line)
./build $version $1 $2
done < versions.csv
done < im/versions.csv
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not right will fix this first

@@ -79,4 +76,3 @@ function install_version() {


install_version
tar -zcf /host/ihs${VERSION}.tar.gz /opt/IBM/HTTPServer /opt/IBM/WebSphere/Plugins /opt/IBM/WebSphere/Toolbox
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was moved to Dockerfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidcurrie The installation-manager image is transient anyway it does not get pushed anywhere. But I did make most of these changes so I can do a build on Windows #20

@@ -1,31 +0,0 @@
#!/bin/bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A new Dockerfile gets generated per version.

@@ -21,15 +21,13 @@ if [ $# != 3 ]; then
exit 1
fi

docker build -t installation-manager im || exit $?
docker run --rm -v $(pwd):/host installation-manager /host/install_ihs $1 $2 $3 || exit $?
docker build -t installation-manager . --build-arg VERSION=$1 --build-arg IBM_ID=$2 --build-arg IBM_PASSWORD=$3|| exit $?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

install_ihs.sh is called within docker during image build. The images are "cached up to the point of line 47. so there's no need to concern about installing Installation Manager again.

@trajano trajano changed the title Better builds + 9.0.0.3 Better builds + 9.0.0.3 + IHS shutdown fix May 24, 2017
ilan/Dockerfile Outdated
# limitations under the License. #
# #
############################################################################
#!/bin/bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is weird I'll remove this

Moved the logic of building the images into docker.  Removed using any
absolute paths on the build system (i.e. $(pwd) which does not work
correctly on Windows)

Added .gitattributes to ensure line endings for files being used in the
Docker build containers are correct.

Please compare with whitespace checks turned off.

In order to support builds within the container bsdtar needs to be used
rather than the regular tar due to
docker/hub-feedback#727

The tar server was also removed in favor of copying from the container
to the "im" folder and imported back using Dockerfile for the final
image.

The step where the files are tarred in install_ihs was removed and put
into docker.  This makes it easier to debug issues with tar
@davidcurrie
Copy link
Contributor

Thanks for the updates. I'll see what we can do about getting a 9.0.0.3 image up tomorrow. I can't make any promises on reviewing the other part of the PR until next week though (heading in to a long weekend here in the UK). You need to specify the -s option on git commit to sign-off on the commits and indicate acceptance of the DCO.

No need for traps and shutdown hooks, simply run IHS in foreground and
it will handle the shutdowns properly.

Fixes WASdev#23

Signed-off-by: Archimedes Trajano <[email protected]>
@trajano
Copy link
Contributor Author

trajano commented May 24, 2017

NP this is a big change anyway. But as long as the 9.0.0.3 gets built tomorrow I'm okay. This was more to help others who needed to get dynamicRouting to work and needed to build on a Windows machine.

@trajano
Copy link
Contributor Author

trajano commented May 24, 2017

@davidcurrie the installation-manager docker image is transient (i.e. it does not get pushed anywhere) so even if it had the tgz file it will not be relevant later. Unless you realize that there was a problem in the second part of the build process which is to assemble the image.

However, now that I think about it I should change the order of one of the lines.

@trajano
Copy link
Contributor Author

trajano commented May 24, 2017

OK that last change will allow you to fix the Dockerfile.template image to do any other fixes after the tgz is created. So if you make changes to ihsstart which I did you don't have to wait as long because the tgz is still there.

RUN export tar='bsdtar'

COPY ihsstart.sh /work/
COPY im/ihs@[email protected] /ihs.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

This step means that you end up with the .tar.gz in a layer in the final image even though it is hidden by the layer above. That's why the current approach serves up the files so that they can be pulled in, unpacked and then deleted all as part of the same layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm you're right I should have an rm after that step to get rid of the ihs.tar.gz file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that's because I don't use --squash on the docker build stage https://docs.docker.com/engine/reference/commandline/build/#squash-an-images-layers---squash-experimental-only it's still experimental but the end users should only get the final image rather than the whole history no?


# Build image from hosted tar file
echo "Building image"
docker build -t ibm-http-server:$1 --build-arg TAR_URL=$tar_url . || exit $?
docker rm -f tar_server
docker build -t ibm-http-server:$1 -f im/DockerFile.$1 . || exit $?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add --squash here to get rid of the intermediate images but the feature is still experimental in Docker 1.13 so I opted not to for now. The end result for the end users will not have the ihs.tar.gz file regardless.

Do it after the tar is complete.  That way if there are any problems in
the Dockerfile.template it will not require a rebuild

Signed-off-by: Archimedes Trajano <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants