Skip to content

Run tests for LHM with dev and shopify-build#32

Merged
bbuchalter merged 3 commits intomasterfrom
shopify-build-add-lhm
Jul 27, 2018
Merged

Run tests for LHM with dev and shopify-build#32
bbuchalter merged 3 commits intomasterfrom
shopify-build-add-lhm

Conversation

@shopify-admins
Copy link
Copy Markdown

@shopify-admins shopify-admins commented Jul 13, 2018

These tests are critical to exercising LHM. We need this to be as easy as
possible for developers to run locally and in CI, so let's automate it.

This also fixes dependencies:

Test with:

dev clone lhm
dev up
dev test

Hello Shopify Build 👋

This PR adds Shopify Build to this repository.

Please review this PR before merging it!

  • Head over to the Shopify Build documentation to learn more about configurating it.
  • Examine your shipit.yml configuration file as it may be configured to depend on the CircleCI status. You can reference a pipeline in the shipit.yml file by using buildkite/lhm as seen here.
  • Don't forget to replace your old CI badge with the Shopify Build one. You can get it here.
  • If you are using codecov.io for code coverage, you'll need to add the CODECOV_TOKEN environment variable to your Shopify Build pipeline. The token can be found at codecov and should be placed in the environment section of the .shopify-build/secrets.ejson file. More info.
  • Rebase and squash the commits into one and don't forget to remove the [ci skip] part from the commit message:
    git rebase -i 65d89a437ead5d4b8548dbfcb74bcfe42dffd2c3

Cleanup After Merging

  • We added a branch filter to your pipeline to prevent other branches from building while you are setting up Shopify Build. Remove the branch filter after the PR is merged by running:
    spy build branch filter clear lhm.
  • After merging the changes to set up Shopify Build you can remove CircleCI from your project by running spy circle remove Shopify/lhm.
  • Let people know with branches that are behind the master branch that their PRs are failing, because they are missing the necessary configuration files. You can use the following reply: “We migrated this repository from CircleCI to Shopify Build. Your build is failing, because your branch doesn't include the necessary configuration files. The configuration files are missing because your branch is behind the master branch. You should rebase your branch to make the build pass with git pull --rebase origin master

Questions about this PR?

Join the #shopify-build Slack channel.

Annoyed by how much you have to do yourself? Make it better in spy.

* Do not specify dependencies in Gemfile (this is a gem)
  * rake was already in gemspec causing bundler to complain
  * added package_cloud to gemspec
* Change `mysql` to `mysql2` because `mysql` is so ancient it won't even compile
* Add Gemfile.lock - this is now considered best practice: rubygems/bundler#6184
@bbuchalter bbuchalter force-pushed the shopify-build-add-lhm branch 15 times, most recently from e638ed1 to ca73bb7 Compare July 13, 2018 21:38
@bbuchalter bbuchalter force-pushed the shopify-build-add-lhm branch from ca73bb7 to d78548d Compare July 16, 2018 13:01
@bbuchalter
Copy link
Copy Markdown

@sroysen I've added the proper commands so dev down works as you would expect. Thanks for catching that oversight.

@bbuchalter bbuchalter changed the title Hello Shopify Build 👋 Run tests for LHM with dev and shopify-build Jul 16, 2018

describe 'with MySQL stopped on the slave' do
it 'assumes 0 slave lag' do
skip "Failing test that needs review"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Backtrace:

NoMethodError: undefined method `exclude?' for []:Array
    /Users/brianb/src/github.com/Shopify/lhm/lib/lhm/throttler/slave_lag.rb:65:in `get_slaves'
    /Users/brianb/src/github.com/Shopify/lhm/lib/lhm/throttler/slave_lag.rb:55:in `slaves'
    /Users/brianb/src/github.com/Shopify/lhm/lib/lhm/throttler/slave_lag.rb:82:in `max_current_slave_lag'
    /Users/brianb/src/github.com/Shopify/lhm/spec/unit/throttler/slave_lag_spec.rb:217:in `block (4 levels) in <top (required)>'

Original commit: a0df0e9

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So, I think we have a hidden dependency on ActiveSupport?

https://github.com/rails/rails/blob/379d98dcd40f28fd959f42958d662b5416171eea/activesupport/lib/active_support/core_ext/enumerable.rb#L102

We could always make the dependency explicit, given that LHM is usually only ran in a Rails context.

@bbuchalter bbuchalter requested a review from shuhaowu July 16, 2018 15:26
@@ -1,4 +1,5 @@
# Large Hadron Migrator [![Build Status][5]][4]
# Large Hadron Migrator
[![Build status](https://badge.buildkite.com/6ed04595f04c5cf6f9f1453afd3705046f6c83088bd29cecf7.svg)](https://buildkite.com/shopify/lhm)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May want to add a note about how this LHM is a downstream fork for our usages as this is a public repository.

install.sh Outdated
wget -q $origin/$filename.tar.gz
tar -xzf $filename.tar.gz
chmod +x $filename
sudo mv $filename /usr/local/bin/dbdeployer
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there a reason why we don't use docker-compose like we do in Ghostferry? This would save all this code of having to download a custom tool and mysql.

I'm somewhat sketched out that this script would install something into my /usr and that this is a hard requirement for running integration tests. If we need to stick with this setup for some requirement and can't use docker-compose, we could unpack this into a vendor directory or even just distribute the binary with this repo and call the executable via relative paths.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have to take responsibility for using dbdeployer in this use case. I suggested Brian to go with it.

IIRC, configuring a master-slave replication arrangement with docker compose was not solved out of the box.

Docker compose may be good enough for test suites, but I want us to get familiar with a tool that is going to allow us to start and stop mysql replicated clusters at will and easily in our machines.

Doing that with docker compose just to test something, is far from trivial.

README.md Outdated
### dbdeployer
The integration tests rely on a master/slave replication setup of MySQL.
We're using [dbdeployer](https://github.com/datacharmer/dbdeployer) to set this up via `./install.sh`.
`dbdeployer` provides scripts for operating and accessing the nodes in `$HOME/sandboxes/rsandbox_5_7_22`.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not the biggest fan that the test for this project will interfere with my $HOME. A directory with a name like sandboxes is fairly generic and could contain other stuff unrelated to mysql. This would mess with that organization.

Maybe it's better to have this install the mysql instances directly in this directory, or under some sort of ~/.directory.

Copy link
Copy Markdown

@insom insom left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! It's going to help with our confidence to make changes to LHM (and to ship them to production in future).

I have some of the same concerns as Shuhao about what this will do to the users machine, and also the use of dbdeployer instead of docker-compose.

I can see that dbdeployer is the more convenient tool, from a database perspective, but there's lots of test environments in Shopify (and even in datastores!) using Docker Compose and Dockerized MySQL -- I wonder how much it's worth using a database specific tool for this, vs. tools other teams are using.

I'm still basically 👍 on having this -- and maybe this is all fine for LHM -- it's written and passing -- but as feedback for future testing environments, I think as a team we should pick one method of doing this, and stick with it.

run:
- bundle check || bundle install
- sudo apt-get update
- sudo apt-get install numactl libaio-dev
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is numactl needed for? Seems like an odd dependency.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Answering my own question, in case others have it: looks like dbdeployer


describe 'with MySQL stopped on the slave' do
it 'assumes 0 slave lag' do
skip "Failing test that needs review"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So, I think we have a hidden dependency on ActiveSupport?

https://github.com/rails/rails/blob/379d98dcd40f28fd959f42958d662b5416171eea/activesupport/lib/active_support/core_ext/enumerable.rb#L102

We could always make the dependency explicit, given that LHM is usually only ran in a Rails context.

@bbuchalter
Copy link
Copy Markdown

@shuhaowu I've addressed your feedback. No mess in $HOME or /usr/local/bin.

@insom I'll see if I can add that dependency to make the test pass. Good catch.

@bbuchalter bbuchalter force-pushed the shopify-build-add-lhm branch from ed0e860 to 7c30e25 Compare July 27, 2018 13:29
These tests are critical to exercising LHM. We need this to be as
easy as possible for developers to run, so let's automate it.
The exclude method on enumerable is provided by activesupport,
which this gem does not currently include its gemspec. Although we
could add it, it's simpler to just prefer the built in `include?`
@bbuchalter bbuchalter force-pushed the shopify-build-add-lhm branch from 7c30e25 to accdd06 Compare July 27, 2018 13:34
@bbuchalter bbuchalter merged commit 17e142a into master Jul 27, 2018
@bbuchalter bbuchalter deleted the shopify-build-add-lhm branch July 27, 2018 13:40
@bbuchalter bbuchalter deployed to production July 30, 2018 16:06 Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants