Fix v5 yum repo url #24

cmclaughlin · 2017-01-06T00:00:55Z

Based on the latest docs...

https://www.elastic.co/guide/en/elasticsearch/reference/current/rpm.html

The yum repo url should end with /yum

However, based on the 2.1 docs...

https://www.elastic.co/guide/en/elasticsearch/reference/2.1/setup-repositories.html

It should end with /centos

Also don't try to install apt-transport-https on rpm systems.

cmclaughlin · 2017-01-19T20:47:06Z

@blbradley I see you've merged previous PRs... can you review/merge this one too?

blbradley · 2017-01-19T21:04:33Z

Sure! Have you ran your changes with success?

blbradley · 2017-01-19T21:09:10Z

Addition: Would you like to add CentOS to the test suite? Not a requirement or anything.

cmclaughlin · 2017-01-23T21:51:19Z

@blbradley I am running with the proposed changes. I'd also like to write some tests, but running the suite fails for me:

=================================== FAILURES ===================================
 test_service_is_running_and_enabled[docker://872b6e362b81c138df4996f2dedc2450e44f77d5a252625ed0d34378143ad259]

Service = <class 'testinfra.modules.base.SystemdService'>

    def test_service_is_running_and_enabled(Service):
        elasticsearch = Service('elasticsearch')
>       assert elasticsearch.is_running
E       assert False
E        +  where False = <service elasticsearch>.is_running

Maybe it's something particular to my workstation since they ran cleanly via travis. I'll keep trying to get them running...

cmclaughlin · 2017-01-23T23:14:29Z

I got the tests running on an EC2 instance, so I should be able to add CentOS next...

blbradley · 2017-01-24T00:29:04Z

I used an Ubuntu-specific provision command because I deferred making a choice.

This formula could use an OS-agnostic way of installing Java. Would using sun-java-formula be acceptable?

blbradley · 2017-01-24T00:31:18Z

I propose that we leave the CentOS testing and Java formula discussion to another PR/issue.

cmclaughlin · 2017-01-24T06:18:24Z

@blbradley I just saw your recent comments, but I'm already done adding centos support and new tests for the repo URL. I think distro specific provision commands are necessary, but I'm new to Test Kitchen so please advise if there's a better way to do that.

I'm not sure about the best practice for Java and the tests. Currently elasticsearch-formula leaves Java installation up to the end user... at my company we already have some Salt code for installing Java, so that works well for me. But if you want me to leverage sun-java-formula in the formula that really goes beyond the scope of fixing the repo url. I don't see any problem installing openjdk for the tests. If you want to switch the Java install or want something more complicated for it that could be done separately.

blbradley

In addition, I think the tests you added should be covered by using testinfra's Package.is_installed. I've been trying to keep the tests OS-agnostic.

blbradley · 2017-01-24T14:19:33Z

.kitchen.yml

@@ -22,10 +14,31 @@ provisioner:
 - elasticsearch

 platforms:
+ # update base ubuntu image and install
+ # software-properties-common for add-apt-repository
+ <% @ubuntu_provision_commands = [


I think it may be best to move each OS-specific provision command to a bash script outside of this file.

blbradley · 2017-01-24T14:20:35Z

.kitchen.yml

+ <% end %>
+ - name: centos-6
+ driver_config:
+ box: rchrd/centos-6-x64-salt


Vagrant is not used by the test suite. As such, this line doesn't do anything.

Doh... I copied that from another salt-formula. Didn't realize they were using Vagrant instead of Docker. I'll work on this more today

No worries! You used the right name. So, an appropriate image was used. It will be fine if you get rid of the driver_config here.

blbradley · 2017-01-24T14:53:27Z

I do appreciate your work and want to get it merged! I'm not sure how to use a bash script as a provision command. So, let me know if you need help with that.

cmclaughlin · 2017-01-25T00:47:01Z

I think the tests you added should be covered by using testinfra's Package.is_installed. I've been trying to keep the tests OS-agnostic.

I'm unsure about this... as you noticed, I added tests to cover my repo URL change. Not sure how checking if a package is installed is relevant....?

blbradley · 2017-01-25T02:30:03Z

If the package installed correctly from a state run, then the repo URL must have been configured correctly. So, if you test the package being installed, then the repo URL has been tested.

blbradley · 2017-01-25T05:05:56Z

I tried to use relative paths and bash scripts with no success. It does not look like the entire repo root directory is copied to built Docker images. Just the directory specified in provisioner:formula. Looked for a way to copy to the repo root to the image. There could be a few ways.

cmclaughlin · 2017-01-25T06:38:39Z

I see your point about the package installation test being good enough for the repo URL.

Hmmm last run on Travis didn't work, but it passed locally. I'll try to figure that out...

blbradley · 2017-01-25T07:00:36Z

I think it would be worth using sun-java-formula in the test suite (and not in the formula) vs trying to run bash scripts to install Java. Please give that a try soon.

cmclaughlin · 2017-01-25T07:46:39Z

I think it would be worth using sun-java-formula in the test suite (and not in the formula) vs trying to run bash scripts to install Java. Please give that a try soon.

Before I started, your provision commands included:

apt-get install -y openjdk-8-jre-headless

Therefore, I think it's reasonable to do this for centos:

yum install -y java-1.8.0-openjdk

If you want to redo the tests with sun-java-formula, let's merge this and perhaps open an issue for future follow-up.

I think I have everything working at this point... just waiting on Travis to verify.

cmclaughlin · 2017-01-25T17:15:53Z

Hmmm pretty sure everything was working locally. I'll retry from scratch to verify. At some point I think my testing was faulty because of cached Docker images, but I think it was working before going to bed last night. Not sure what to make of the NotImplementedError failure on Travis at this point...

cmclaughlin · 2017-01-25T18:02:29Z

Yeah it works locally. Here's my last run:

https://gist.github.com/cmclaughlin/e709e6f3aac66945a2214fa7743269a2

cmclaughlin · 2017-01-25T18:50:49Z

It appears Travis is running off the master branch, not my PR. I guess the KitchenCI config is running off my branch? But the job definition has this:

$ git clone --depth=50 https://github.com/saltstack-formulas/elasticsearch-formula.git saltstack-formulas/elasticsearch-formula

I don't see any mention of changing branches.

Am I missing something?

myoung34 · 2017-01-25T20:32:31Z

@cmclaughlin FWIW if you expand that git clone it does a git fetch +refs/pulls/24 then does git checkout -qf FETCH_HEAD, which:

FETCH_HEAD is a short-lived ref, to keep track of what has just been fetched from the remote repository. 
git pull first invokes git fetch , in normal cases fetching a branch from the remote; 
FETCH_HEAD points to the tip of this branch (it stores the SHA1 of the commit, just as branches do).

Also per the last travis run (36) line 4082 shows

Step 16 : RUN yum install -y java-1.8.0-openjdk

so it's definitely running the right SHA

myoung34 · 2017-01-25T20:36:46Z

As for the failure:

                 ID: elasticsearch_service
           Function: service.running
               Name: elasticsearch
             Result: False
            Comment: Service elasticsearch has been enabled, and is dead
            Started: 19:10:27.019052
           Duration: 6409.7 ms
            Changes:

If you look above this (hidden as it's not red):

       [ERROR   ] output: Starting elasticsearch: OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x000000008a660000, 1973026816, 0) failed; error='Cannot allocate memory' (errno=12)
       #
       # There is insufficient memory for the Java Runtime Environment to continue.
       # Native memory allocation (mmap) failed to map 1973026816 bytes for committing reserved memory.
       # An error report file with more information is saved as:
       # /tmp/hs_err_pid609.log

Looks like travis is more aggressive about memory limits and is SIGKILL'ing the elasticsearch process whereas your docker is more hefty

blbradley · 2017-01-25T21:04:44Z

I got sun-java-formula working with just the test suite. I'll push a fix soon. We can revert or change if needed.

cmclaughlin · 2017-01-25T21:25:37Z

Thanks for investigating that. And thanks for integrating sun-java-formula for the tests... I'm curious how you'll do that.

blbradley · 2017-01-29T16:22:10Z

Test case for ES 5 on CentOS is failing on a memory error. Here is the important log output:

       [ERROR   ] Command '/sbin/service elasticsearch start' failed with return code: 1
       [ERROR   ] output: Starting elasticsearch: Java HotSpot(TM) 64-Bit Server VM warning: INFO: os::commit_memory(0x000000008a660000, 1973026816, 0) failed; error='Cannot allocate memory' (errno=12)
       #
       # There is insufficient memory for the Java Runtime Environment to continue.
       # Native memory allocation (mmap) failed to map 1973026816 bytes for committing reserved memory.
       # An error report file with more information is saved as:
       # /tmp/hs_err_pid645.log
       FAILED]

I recall this being a new safety check in ES 5.1. May need to manage another parameter to get the tests to work.

blbradley · 2017-01-29T16:24:13Z

In addition, the yaml that was intended to be imported should be removed.

blbradley · 2017-01-29T16:32:37Z

I reported this type of failure in #17.

cmclaughlin · 2017-02-02T22:45:43Z

@blbradley I think I'm done here, but Travis CI still fails. However, the tests pass for me locally. I don't think it's what you saw in issue #17... it's something about SSH'ing into the Docker containers. Have you seen that before?

blbradley · 2017-02-02T22:50:53Z

No. I restarted the build just in case.

blbradley · 2017-02-02T23:47:10Z

Travis has been acting slow the past few days, and the test suite currently takes a really long time.

We need to defer the CentOS testing to another PR and get this bug fixed now. In order to keep the commits from this PR for your usage, I'm going to revert to the first commit using git revert and merge just the bugfix. After I do that, feel free to submit another PR with the testing changes.

On another note, we will only be able to test three OSes before optimization on the test suite is required. See saltstack-formulas/docker-formula#95. It might be only two with how slow Travis has been acting this week.

Based on the latest docs... https://www.elastic.co/guide/en/elasticsearch/reference/current/rpm.html The yum repo url should end with /yum However, based on the 2.1 docs... https://www.elastic.co/guide/en/elasticsearch/reference/2.1/setup-repositories.html It should end with /centos Also don't try to install apt-transport-https on rpm systems.

cmclaughlin · 2017-02-03T00:11:15Z

@blbradley sounds good... I just removed the centos test and squashed, so this should be OK to merge.

blbradley · 2017-02-03T00:23:35Z

Thank you so much, Charles! I do appreciate you giving the test suite a good ole' one-two and for reporting/fixing the bug that I caused (I think).

blbradley suggested changes Jan 24, 2017

View reviewed changes

blbradley added the bug label Feb 1, 2017

cmclaughlin force-pushed the charles-v5repo branch from 6337e78 to 73a30ce Compare February 2, 2017 20:35

cmclaughlin force-pushed the charles-v5repo branch from 73a30ce to ff0f08b Compare February 3, 2017 00:10

blbradley merged commit 8dbf4b3 into saltstack-formulas:master Feb 3, 2017

cmclaughlin deleted the charles-v5repo branch February 3, 2017 19:45

Fix v5 yum repo url #24

Fix v5 yum repo url #24

Conversation

cmclaughlin commented Jan 6, 2017

cmclaughlin commented Jan 19, 2017

blbradley commented Jan 19, 2017

blbradley commented Jan 19, 2017

cmclaughlin commented Jan 23, 2017

cmclaughlin commented Jan 23, 2017

blbradley commented Jan 24, 2017

blbradley commented Jan 24, 2017

cmclaughlin commented Jan 24, 2017

blbradley left a comment

Choose a reason for hiding this comment

blbradley Jan 24, 2017

Choose a reason for hiding this comment

blbradley Jan 24, 2017

Choose a reason for hiding this comment

cmclaughlin Jan 24, 2017

Choose a reason for hiding this comment

blbradley Jan 24, 2017

Choose a reason for hiding this comment

blbradley commented Jan 24, 2017

cmclaughlin commented Jan 25, 2017

blbradley commented Jan 25, 2017 • edited Loading

blbradley commented Jan 25, 2017

cmclaughlin commented Jan 25, 2017

blbradley commented Jan 25, 2017

cmclaughlin commented Jan 25, 2017

cmclaughlin commented Jan 25, 2017

cmclaughlin commented Jan 25, 2017

cmclaughlin commented Jan 25, 2017

myoung34 commented Jan 25, 2017 • edited Loading

myoung34 commented Jan 25, 2017 • edited Loading

blbradley commented Jan 25, 2017

cmclaughlin commented Jan 25, 2017

blbradley commented Jan 29, 2017 • edited Loading

blbradley commented Jan 29, 2017

blbradley commented Jan 29, 2017

cmclaughlin commented Feb 2, 2017

blbradley commented Feb 2, 2017

blbradley commented Feb 2, 2017

cmclaughlin commented Feb 3, 2017

blbradley commented Feb 3, 2017

blbradley commented Jan 25, 2017 •

edited

Loading

myoung34 commented Jan 25, 2017 •

edited

Loading

myoung34 commented Jan 25, 2017 •

edited

Loading

blbradley commented Jan 29, 2017 •

edited

Loading