You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@metron.apache.org by jameslamb <gi...@git.apache.org> on 2018/07/03 04:28:23 UTC

[GitHub] metron pull request #1091: METRON-1650: Cut size of packaging docker contain...

GitHub user jameslamb opened a pull request:

    https://github.com/apache/metron/pull/1091

    METRON-1650: Cut size of packaging docker containers

    ## Contributor Comments
    
    I have been looking through the project source code, and found that the packaging docker containers are bigger than they need to be. 
    
    The containers' size could be cut substantially by:
    
    * Grouping as many commands as possible into as few RUN stages as possible
    * Taking advantage of `yum clean` and `apt-get clean`
    * Removing artifacts pulled with `wget`
    * Removing unused libraries or libraries only needed at build time (like `tar` and `wget`)
    
    In local testing, I found the following reductions in container size:
    
    `deb`: 700MB --> 672MB
    `rpm`: 1GB --> 566MB
    `ansible`: 2.48GB --> 1.21GB
    
    To test these changes, run the following on current master from the repo root:
    
    ```
    METRON=$(pwd)
    for pkgtype in ansible deb rpm; do
        cd ${METRON}/metron-deployment/packaging/docker/${pkgtype}-docker/
        docker build -t ${pkgtype}_orig -f Dockerfile .
    done
    ```
    
    Then run again on the PR branch:
    
    ```
    METRON=$(pwd)
    for pkgtype in ansible deb rpm; do
        cd ${METRON}/metron-deployment/packaging/docker/${pkgtype}-docker/
        docker build -t ${pkgtype}_pr -f Dockerfile .
    done
    ```
    
    You can then use `docker images` to compare the size of the original images to those generated by the Dockerfiles in this PR.
    
    Thanks for considering this change!
    
    ## Pull Request Checklist
    
    ### For all changes:
    - [y] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
    - [y] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
    - [y] Has your PR been rebased against the latest commit within the target branch (typically master)?
    
    
    ### For code changes:
    - [y] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
    - [y] Have you included steps or a guide to how the change may be verified and tested manually?
    - [] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
      ```
      mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh 
      ```
    
    - [ ] Have you written or updated unit tests and or integration tests to verify your changes?
    - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
    - [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
    
    ### For documentation related changes:
    - [ ] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
    
      ```
      cd site-book
      mvn site
      ```


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jameslamb/metron master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/metron/pull/1091.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1091
    
----
commit 64ece102875f827293814f3c4ac3a091f8c86df8
Author: James Lamb <ja...@...>
Date:   2018-07-03T04:17:20Z

    METRON-1650: Cut size of packaging docker containers

----


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @jameslamb 
    
    ```bash
    cd metron-deployment/development/centos6
    vagrant up
    ```
    
    To run it.  This will build, build the rpms in docker, create a vagrant machine and install metron in that machine with ambari.
    
    After it is done if you go to http://node1:8080 it is the vagrant ambari, where you can check that everything loaded and installed.



---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @mmiklavc Thank you for the help! Unfortunately, this did not work for me. Running `mvn clean install -DskipTests` fails with this error:
    
    ```
    [ERROR] Failed to execute goal on project metron-stellar: Could not resolve dependencies for project org.apache.metron:metron-stellar:pom:0.5.1: Could not find artifact sun.jdk:tools:jar:LATEST at specified path /Library/Java/JavaVirtualMachines/jdk-10.0.1.jdk/Contents/Home/../lib/tools.jar -> [Help 1]
    ```
    
    Env info from `mvn --version`:
    
    ```
    Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T13:33:14-05:00)
    Maven home: /Applications/apache-maven-3.5.4
    Java version: 10.0.1, vendor: Oracle Corporation, runtime: /Library/Java/JavaVirtualMachines/jdk-10.0.1.jdk/Contents/Home
    Default locale: en_US, platform encoding: UTF-8
    OS name: "mac os x", version: "10.13.5", arch: "x86_64", family: "mac"
    ```
    
    I did try googling around for this error (thinking maybe it's just something that Java developers understand and experience commonly), but I haven't had any luck.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    Great!  I will give this a try asap


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @jameslamb,
    
    The vagrant stuff is the general information on how to run it.  For the docker containers that you have worked on, it would be fine I think to say that you tested that the containers work for their purpose.
    
    It would be enough for you to say in the description:
    
    ```
    Testing:
    run ansible docker like this %THIS, see that it works without error
    run deb docker like this %THIS, see that it works without error
    run rpm docker like this %THIS, see that it works without error
    
    ```
    
    Our vagrant build for centos and ubuntu targets actually use the rpm and deb dockers so that is one way they get tests, but doesn't have to be the only way.
    
    the ansible docker must be tested explicitly
    
    



---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    thanks again @jameslamb!


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @merrimanr I'd like to get your sign off on this, now that @cestella and I have given a +1


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    Point taken.  The issue we have is that our builds take a long time so we have to prioritize what is included in travis.  There are several components that can't be tested in travis even if we wanted to so we test with our VM environment frequently (daily).  That's the world we live in :)
    
    This could be added to travis but I don't know that it's worth it because this part of Metron (setting up a Docker container to build in) doesn't change very often.  Changes that modify our packages (adding/removing scripts or other assets) need to be tested and demonstrated in full dev anyways.  Others might have a different opinion though.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @merrimanr thank you, understood. I will try to reproduce the issue on my end and see if I can find what is failing.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @merrimanr @ottobackwards Hey I think I got this working! See my recent commit. I had accidentally smashed together two commands, which is why node stuff was failing.
    
    I was able to get successful builds locally by running the following (after installing Java 8, per recommendations above):
    
    ```
    mvn clean install -DskipTests -Drat.skip=true
    cd metron-deployment/
    mvn clean package -DskipTests -Pbuild-rpms
    mvn clean package -DskipTests -Pbuild-debs
    ```



---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @jameslamb It looks like you are trying to build with Java 10.  You need to be running Java 8 to build the project.  
    
    You might be interested in the instructions [here](https://github.com/apache/metron/tree/master/metron-deployment/development/centos6#getting-started).  
    
    Also, sending the output of running `metron-deployment/scripts/platform-info.sh` is also helpful to ensure that you have all the dependencies setup to build Metron correctly.
    
    Feel free to reach out on the dev mailing list for help in getting your build environment setup.  It might make sense to carry the conversation forward there, at least until you can build master and deploy the dev environment.  Once you get past that hurdle we could restart the conversation here focusing on your specific contribution.
    
    Thanks for contributing!  


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @nickwallen Thank you so much!! I was able to get `master` building with your help and the advice of the other commenters.
    
    Now that I know I can build, I'll be able to test my changes. Will comment on here once I think I have something that works.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @merrimanr Oh I force-pushed over the old commit. I don't like having failing commits in the history. Apologies if that was not what you expected. In the future I can push new ones individually.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    You of course also have to make sure they work


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    thanks for helping me work through it! I will be back to Metron to contribute in the future 😄 


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    This gets a +1 from me too, great job!


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @ottobackwards I tried to get this running on my local but was hitting some weird networking errors, even on current master (without any of my changes).
    
    I'm not very familiar with vagrant and don't have the time to pick it up right now. If familiarity with vagrant is a prerequisite for contributing on this project then I probably won't be able to contribute.
    
    I will leave it to you and @merrimanr to do what you want with this PR.
    
    Cheers,
    
    -James


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    ok that's fair! And I do appreciate that you had an issue template saying "hey do these things before you make contributions". I'll update the PR when I've had a chance to re-run them.
    
    Thanks for the review!


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    I (and I expect most in our community) would prefer separate commits.  It makes it easier to see changes you make in response to feedback.  Also when I pulled your recent commits I got a merge conflict because of the forced push.  We squash the commits when we merge a PR to master so it doesn't really matter anyways.  In the end it's one commit.
    
    Running the command in my previous comment should reproduce the error.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    Unfortunately, I cannot even build the current `master` branch of this project (with none of my changes) successfully with that command. 
    
    When running `mvn clean package -DskipTests -Pbuild-rpms` from `metron-deployment`, the build dies with:
    
    ```
    + tar -xzf /root/SOURCES/metron-common-0.5.1-archive.tar.gz -C /root/BUILDROOT/metron-0.5.1-root/usr/metron/0.5.1
    tar (child): /root/SOURCES/metron-common-0.5.1-archive.tar.gz: Cannot open: No such file or directory
    tar (child): Error is not recoverable: exiting now
    tar: Child returned status 2
    tar: Error is not recoverable: exiting now
    
    
    RPM build errors:
    error: Bad exit status from /var/tmp/rpm-tmp.sAg9X3 (%install)
        Macro %_prerelease has empty body
        Macro %_prerelease has empty body
        Bad exit status from /var/tmp/rpm-tmp.sAg9X3 (%install)
    RPM build errors encountered
    [ERROR] Command execution failed.
    org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1)
        at org.apache.commons.exec.DefaultExecutor.executeInternal (DefaultExecutor.java:404)
    
    <big stack trace>
    ```
    
    I totally cleared out all my docker images before running, so it's not like this is being caused by docker finding one of the images built from my branch in its cache.
    
    I don't even know where to start with this error. It's disappointing, but you can just close this PR if you want. I hope you will borrow some of the ideas from it...there is a lot of excess stuff in these containers right now.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    just pushed the `scl` fix


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    Hey, awesome! Thanks to you and the rest of the metron community for your patience and support


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @jameslamb, this is still failing for me.  Just to be clear, the "build-rpms" and "build-debs" maven profiles should be used when testing this.  You don't necessarily have to build full dev.  When I run this command in metron-deployment:
    ```
    mvn clean package -DskipTests -Pbuild-rpms
    ```
    I am still getting this error:
    ```
    /var/tmp/rpm-tmp.VpikxG: line 60: npm: command not found
    error: Bad exit status from /var/tmp/rpm-tmp.VpikxG (%install)
        Macro %_prerelease has empty body
        Macro %_prerelease has empty body
        Bad exit status from /var/tmp/rpm-tmp.VpikxG (%install)
    ```
    
    I think your change may not have been pushed out correctly.  I still only see 1 commit in the PR when I would expect 2:  your original commit + the commit to fix this issue.  Because of this I'm also not able to see what you did to fix the error, I only see all changes.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    +1 from me.  Thank you so much @jameslamb for sticking with this.  Nice work!


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    can one of you ( @cestella or @merrimanr ) merge?  I can't right now


---

[GitHub] metron pull request #1091: METRON-1650: Cut size of packaging docker contain...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/metron/pull/1091


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    Thank you for the contribution @jameslamb!  This failed for me when I tried to run it up in full dev:
    ```
    + npm install --prefix=/root/BUILDROOT/metron-0.5.1-root/usr/metron/0.5.1/web/expressjs --only=production
    /var/tmp/rpm-tmp.tvLfXe: line 60: npm: command not found
    error: Bad exit status from /var/tmp/rpm-tmp.tvLfXe (%install)
        Macro %_prerelease has empty body
        Macro %_prerelease has empty body
        Bad exit status from /var/tmp/rpm-tmp.tvLfXe (%install)
    ```
    
    I think the "Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?" item in the PR checklist is a requirement for this one.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @merrimanr are you all set?


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    I THINK I found the issue with my original PR. The command to register the scl repo doesn't get picked up by `yum` until it exits.
    
    **this fails to find devtoolset-4**
    
    ```
    yum install -y \
        centos-release-scl \
        devtoolset-4-gcc-c++
    ```
    
    **this finds it**
    
    ```
    yum install -y centos-release-scl \
    && yum install -y devtoolset-4-gcc-c++
    ```
    
    Testing locally now, will update later tonight. I am testing locally by walking through the build instructions in each README in the `metron-deployment/packaging/docker` sub-folders. I've never used vagrant and don't actually know how to do "Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?"


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    +1 from me.  I was able to do the above, along with building metron from the instructions ansible-docker's readme.md.
    
    Thanks for sticking with it.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    Running `mvn clean package -DskipTests` inside the container built with this version of `ansible-docker` succeeds:
    
    ![image](https://user-images.githubusercontent.com/7608904/42256070-8da6a166-7f14-11e8-850c-788fa5d9eacf.png)



---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    I broke this into two steps (was previously one):
    
    ```
    # Install Software Collections repo (needs to be done in separate command) \
    +    && yum install -y \
    +        centos-release-scl \
    +    # newer cpp 11 support required for building node modules \
    +    && yum install -y \
    ```
    
    because the other deps installed need `scl` repository to be registered with `yum`


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @jameslamb Thank you for taking the time to contribute to the project. That command you're running is independent of the main build from root. It appears that it's probably failing because those tar.gz artifacts have not been built and thus will not have been copied to the SOURCES folder. Run the following from project root: `mvn clean install -DskipTests` followed by the command you posted above from `metron-deployment`. The `SOURCES` and `BUILDROOT` folders should typically exist in `<project-root>/metron-deployment/packaging/docker/rpm-docker/`. I'm not sure what the `/root` path business is all about, but I hope this helps.


---

[GitHub] metron issue #1091: METRON-1650: Cut size of packaging docker containers

Posted by jameslamb <gi...@git.apache.org>.
Github user jameslamb commented on the issue:

    https://github.com/apache/metron/pull/1091
  
    @merrimanr ok no problem! Yeah I can run those later and report back.
    
    IMHO it's a problem that your CI doesn't catch this though? Like ideally shouldn't these package builds should be done on Travis so reviewers don't need to remind contributors to run these tests locally?
    
    Making contributors run tests locally exposes you to either false positives or false negatives caused by differences between their local environment and whatever actual environment you use to build these.


---