If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[PulseGuardian] Use docker for tests

RESOLVED FIXED

Status

Webtools
Pulse
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcote, Assigned: Ricardo Castro)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

47 bytes, text/x-github-pull-request
mcote
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
gps has done some really nice stuff with docker for external dependencies in the version-control-tools tests.  We already have a vagrant box in the mozillapulse package, so we should try to do something similar, but with docker so we can automate it even better.
(Reporter)

Updated

3 years ago
Blocks: 1089455
(Reporter)

Updated

3 years ago
No longer blocks: 1089455
(Assignee)

Comment 1

3 years ago
Picking this one up.
Assignee: nobody → mcc.ricardo
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
I'm making some progress on this.

So far, I managed to create Dockerfile that will create an image with Ubuntu 14.04 and RabbitMQ. I also managed to create a daemonized container running rabbitmq-server.

Next I'll configure ports, vhost and user.
(Reporter)

Comment 3

3 years ago
Awesome!  Remember that you need to enable the management plugin too; see the docs in the mozillapulse package or the .travis.yml file in pulseguardian.
(Assignee)

Comment 4

3 years ago
(In reply to Mark Côté [:mcote] from comment #3)
> Awesome!  Remember that you need to enable the management plugin too; see
> the docs in the mozillapulse package or the .travis.yml file in
> pulseguardian.

Exactly. We'll also need to extend the README file (or add a more convenient one for the purpose) to explain and help the setup of the container.
(Assignee)

Comment 5

3 years ago
I still have some testing to do on this, but I think I got this working. At least, I can now run PulseGuardian and RabitMQ server is running on a docker container :)

Do you want add anything else to the container?
(Assignee)

Comment 6

3 years ago
Another question:

Want me to add the Dockerfile along side the other files? Or do you think it's preferable to create a tests folder like in mozillapulse and put the Dockerfile in there? Maybe even put the runtests.py file in there too?

What are your thoughts on that?
Flags: needinfo?(mcote)
(Reporter)

Comment 7

3 years ago
(In reply to Ricardo Castro from comment #5)
> Do you want add anything else to the container?

I don't think so.  We really just need a working RabbitMQ (with management plugin) to develop/test PulseGuardian.

(In reply to Ricardo Castro from comment #6)
> Another question:
> 
> Want me to add the Dockerfile along side the other files? Or do you think
> it's preferable to create a tests folder like in mozillapulse and put the
> Dockerfile in there? Maybe even put the runtests.py file in there too?
> 
> What are your thoughts on that?

Hmmm good question.  Since this can be used for development as well as testing, perhaps it can be left in the root.  But seeing as how you've been working with PulseGuardian for a bit now, what's your opinion? :)
Flags: needinfo?(mcote)
(Assignee)

Comment 8

3 years ago
I'm with you on this one. Since we're going to use it for development, the root seems to be a more adequate place.
(Assignee)

Comment 9

3 years ago
Created attachment 8518992 [details] [review]
Pul request.
Attachment #8518992 - Flags: review?(mcote)
(Assignee)

Comment 10

3 years ago
Hey Mark,

Give it a look and tell me what you think.
(Reporter)

Comment 11

3 years ago
Comment on attachment 8518992 [details] [review]
Pul request.

Thanks, this is a good start.

I managed to get docker up on my Mac, but I could not execute the tests even after modifying the rabbit_host variable in config.py and running runtests.py and passing the boot2docker ip to --host.  It still keeps trying to connect to localhost.  Do you have a Mac to test on?

Also as noted in the pull request comments, we should automate this even more--automatically set up the docker container, and tear it down at the end.  I admit this might be tricky, but I think it's important if we expect people to run the tests themselves.
Attachment #8518992 - Flags: review?(mcote) → review-
(Assignee)

Comment 12

3 years ago
Hey Mark,

I'll test it on a Mac and figure out what's wrong. 

You make a good point. It might be a little bit tricky but I think it's worth it. It will make the test suite a lot cleaner easier to use.
(Assignee)

Comment 13

3 years ago
Starting to look it doesn't work on a Mac. This might be interesting:

> INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 192.168.59.103
> INFO:dbinit:Finished initializing database.
> INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): localhost
> ERROR:root:Failed to connect to the RabbitMQ server.
(Assignee)

Comment 14

3 years ago
>85: self.management_api = PulseManagementAPI()

I believe this defaults to 'localhost'.

From 'managment.py':

> DEFAULT_RABBIT_HOST = 'localhost'
> def __init__(self, host=DEFAULT_RABBIT_HOST,
>              management_port=DEFAULT_RABBIT_MANAGEMENT_PORT,
>              user=DEFAULT_RABBIT_USER,
>              password=DEFAULT_RABBIT_PASSWORD):
>        self.host = host

I'll test it :)
(Assignee)

Comment 15

3 years ago
That was it. Just to test, changed to something like:

> self.management_api = PulseManagementAPI(host=DEFAULT_RABBIT_HOST,
>                                       user=DEFAULT_RABBIT_USER,
>                                       password=DEFAULT_RABBIT_PASSWORD)

And it worked: tests are now running. Have to wait until they complete, but looks good.

'runtests.py' just needs some small adjustments :)
(Assignee)

Comment 16

3 years ago
Got it setup and running in Ubuntu but having some trouble with OS X. boot2docker seems to be blocking connections and although I've forwarded ports, still not working. I'll dig deeper.
(Assignee)

Comment 17

3 years ago
Created attachment 8522533 [details] [review]
Pull request.

Just a few points:

1) an image and a container are being crated each time a test is run. The image is only built the first time since subsequent builds identify that the image already exist and use it to build the container. On the other a container is built and destroyed each time. I decided not to destroy the image every time because building it takes some time (download and actually building it)and because we might end up removing the Ubuntu image that could happen to be used somewhere else. Nonetheless there's a function for that effect;

2) I added an extra argument to the runtests.py to invoke the use of docker. That way we can easily use the same script in Travis CI (I've updated .travis.yml and the tests are passing)
Attachment #8518992 - Attachment is obsolete: true
Attachment #8522533 - Flags: review?(mcote)
(Reporter)

Comment 18

3 years ago
Comment on attachment 8522533 [details] [review]
Pull request.

This is great!  My first run failed, but only because I had a pulse container left over from earlier; the second run (and third) ran great. \o/

I have a few comments, all related to making this even smoother, but this is pretty fantastic right now. :)
Attachment #8522533 - Flags: review?(mcote)
(Reporter)

Comment 19

3 years ago
I'm going to merge your branch over to the official mozilla repo (as a branch with the same name) just to get test results there.
(Assignee)

Comment 20

3 years ago
Cool :) 

I was struggling with the config file myself. I was thinking it was a bit unproductive to have to set the host every time we needed to run the test with the docker container.

The only issue here, is that the developer might already have a config.py set for it's own development.  My suggestion would be to check if that file exists and if it does, rename it to something line config.dev. Then have a config.docker ready to go, rename it config.py and after the tests run revert these changes.

What do you think?
Flags: needinfo?(mcote)
(Assignee)

Comment 21

3 years ago
Actually, the config file is already being imported in runtests.py. We can simply just update the rabbit_host with the docker ip.
(Assignee)

Comment 22

3 years ago
One other thing I just noticed: we're binding the contianer ports to the rabbitmq ports, using the default rabbitmq values . What that means is that if a developer is running it's own local instance of rabbitmq (most likely using the defaults) the container will fail because it wont be able to bind it's own rabbitmq instance to those ports.

In order to make this even more self contained, we should use a different set of ports, or we will have to force the developer to stop the local instance (in case it's running in those ports).
(Assignee)

Comment 23

3 years ago
In 'dbinit.py' we have:

> pulse_management = PulseManagementAPI(host=config.rabbit_host,
>                                      user=config.rabbit_user,
>                                      password=config.rabbit_password)

This makes that code tightly coupled with 'config.py' values. We either make the above option of renaming (and renaming back) config files for tests (probably not a good idea). Or we can decouple that code, by passing a PulseManagementAPI object to each function of 'dbinit.py'.
(Assignee)

Comment 24

3 years ago
Maybe alternatively, and to avoid big changes, we can import that 'dbinit.py' pulse_management object, assign it our PulseManagementAPI 'self.management_api' and call 'init_and_clear_db()' only after that.
(Assignee)

Comment 25

3 years ago
Created attachment 8524891 [details] [review]
Pull request.

The default now it's to use docker. A new argument '--use-local' was added to allow the developer to run tests against it's local setup.

Also an option for removing the image was added.

The test suite will try to connect to rabbitmq inside the container for a configurable  amount of time (20s as default) failing if it can't.

Checking if a $DOCKER_HOST env variable exists and using it as a host.
Attachment #8522533 - Attachment is obsolete: true
Flags: needinfo?(mcote)
Attachment #8524891 - Flags: review?(mcote)
(Reporter)

Comment 26

3 years ago
Comment on attachment 8524891 [details] [review]
Pull request.

This is awesome!  I just made a few comments on the PR (one of which rambly as I realized I misunderstood something; I'm also used to Bugzilla where you can't edit nor delete a comment after submitting it :).  Really minor points.  It ran perfectly on my OS X machine here.
Attachment #8524891 - Flags: review?(mcote) → review+
(Assignee)

Comment 27

3 years ago
Created attachment 8525517 [details] [review]
Pull request.

Changes:

- Fixed spelling errors
- Fixed new argument format
- Using urlparse to get DOCKER_HOST
- Removed unnecessary calls to 'init_db' and 'dbinit.init_and_clear_db'

I think we're almost there. I think those 'init_db' and 'dbinit.init_and_clear_db' I removed are not really necessary. I tested on both Ubuntu and OS X and tests ran smoothly :)
Attachment #8524891 - Attachment is obsolete: true
Attachment #8525517 - Flags: review?(mcote)
(Reporter)

Comment 28

3 years ago
Comment on attachment 8525517 [details] [review]
Pull request.

Great!  There was one tiny spelling mistake and an incorrect link; I fixed them, squashed your commits into one, and merged it into master.  Thanks!
Attachment #8525517 - Flags: review?(mcote) → review+
(Reporter)

Comment 29

3 years ago
https://github.com/mozilla/pulseguardian/commit/cf8cb7a7c290f43bd4d855e85439622791f37791
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.