Integration tests by compiling and calling Ghostferry from Ruby #64

shuhaowu · 2018-09-17T15:28:55Z

Instead of running integration tests directly in Go, this PR enables us to run integration tests by calling Ghostferry from Ruby as a subprocess. This is needed because it is not possible for the the Go code to interrupt and resume itself.

Given that the Ruby integration test code is also much cleaner than the Go equivalent and suffer less issues such as goroutine leakage, we should prefer writing integration tests in Ruby from now on.

The Ghostferry subprocess right now is a separate binary from copydb/sharding. This decision was made because the tests needs to perform some actions (such as interrupting Ghostferry or injecting some data) in the middle of the run at strategic and reproducible times(1) and the base Ghostferry framework doesn't support it for now. In order to accomplish this, bidirectional communication needs to exists between the Ruby and Go code and this is done via a WEBrick::HTTPServer on the local machine.

Using this server, Ghostferry can send a message and pause until a reply is sent from the server. The pause allows race injection and other test code to execute.

All of this is wrapped up in the class GhostferryHelper::Ghostferry. To use it in tests, one can extend from GhostferryTestCase, where other helpers like the database connection, a parallel data writer, also exists.

A test will look something like the following:

  def test_copy_data_with_writes_to_source
    seed_simple_database_with_single_table

    datawriter = new_source_datawriter
    ghostferry = new_ghostferry(MINIMAL_GHOSTFERRY)

    start_datawriter_with_ghostferry(datawriter, ghostferry)
    stop_datawriter_during_cutover(datawriter, ghostferry)

    ghostferry.run
    assert_test_table_is_identical
  end

shuhaowu · 2019-01-08T21:43:17Z

It would be great if I can get some reviews on this PR. It's a fairly independent piece that we'll need regardless what approach we take to implement the rest of #75 as we'll need a mechanism to test.

At least half of this PR is reasonably trivial: direct translation from Go (DataWriter, TrivialIntegrationTest, TestCase), glue code to work with database (DbManager). The other half is a simple implementation of a Ghostferry with unix socket for status reporting.

For future note: the integrationferry.Ferry.Main could be a method we'll eventually port back into ghostferry.Ferry, to simplify how Ghostferry application can be run as per an idea @pushrax had sometime ago.

fjordan

Agree with the overall approach of running integration tests using a separate (ruby) process. Questioning the necessity of the socket communication and being event-driven as it seems to introduce a good amount of complexity, but apart from that this looks good to me. I can't speak to the conventions or idioms of ruby testing as much as others, so I'll defer that to the other reviewers.

fjordan · 2019-01-09T15:14:37Z

test/integration/go/integrationferry/integrationferry.go

+	StatusDone                   string = "DONE"
+
+	// Could be sent by multiple goroutines in parallel
+	StatusBeforeRowCopy     string = "BEFORE_ROW_COPY"


Are all of these statuses actually needed and used?

They will be useful. For example, if we want to terminate Ghostferry in a test after copy has begun (in a later PR):

@ghostferry.on_status(Ghostferry::Status::AFTER_ROW_COPY) do batches_written += 1 if batches_written >= 2 @ghostferry.send_signal("TERM") end end

Furthermore it is the intention to eventually port all existing Ghostferry integration tests into this framework. Lots of those Go tests uses the same statuses.

fjordan · 2019-01-09T15:20:20Z

test/integration/go/integrationferry/integrationferry.go

+	*ghostferry.Ferry
+}
+
+// =========================================


Nice work setting all of this up. It clearly works and does what it needs to do. However, I'm not sure we really need to setup and manage a socket to communicate between ghostferry and the ruby master process. Could the same thing not have been achieved using signals (SIGSTOP, SIGCONTv, and SIGUSR1/2), ENV vars or even files? This just seems like significant overhead and complexity that could be simplified using other methods.

Also, I'm not sure we really need to make it event-driven (do we really need to know when each row is written?) as much as simply communicating overall state, which I again feel could be communicated using the other methods mentioned above.

I also don't understand the need for this custom communication protocol.

Another option is to use the already existing HTTPCallback facility, similar to how the sharding package uses it in sharding/testhelpers/unit_test_suite.go and how the shop mover integration works (see ghostferry.rb in core).

hkdsun

As discussed on slack, this ruby testing setup/structure is super weird to me. Can we follow some existing patterns instead of hacking together a bunch of binaries? I understand it works but please let's refactor to something that our colleagues can understand quickly.

What I expect:

bundle exec rake test in the root of the repo runs all ruby tests (see Rakefile in nginx-routing-modules)
require 'test_helper' instead of require 'ghostferry_integration'
All ruby test files follow the pattern *_test.rb and live under test/integration or test/unit
Avoid implicit assumptions in tests such as @dbs and @ghostferry. It's fine to have them in one file (the base test case) but not all over the rest of the test cases.

I can provide a more detailed review when some of these concerns are taken care of

hkdsun · 2019-01-09T15:52:13Z

By the way, you may want to check out and possibly port some of the shop mover tests from core

Makes room to make the tests more like ruby tests, which will be employed heavily.

Instead of running integration tests directly in Go, we run it in Ruby by calling Ghostferry as a subprocess. The idea is that we will be able to test the interrupt/resume work more easily as well as making integration tests easier to write. In order to allow the ruby tests to inject race conditions for testing purposes, we need a way for the ruby code to pause and restart the execution of Go code at strategic locations. This is done with a local Unix socket. At locations of interests, the Go code will send some string via the Unix socket to the Ruby code. The ruby code will perform some sort of callback (such as injecting data, or locking a row) and then send a command back to the Go code via the same Unix socket, allowing it to continue executing. Note that each time we send some status to the ruby server, we do it in a new connection. This is to avoid race conditions if one shared connection was used between all the goroutines we employ inside Ghostferry. Stdout and stderr are captured and they can be asserted against/examined, more helper methods can be created later to give better structures for the stdout/stderr output such as being able to recognize a panic, or recognize particular stages of execution from within the logs. Lastly, the intention is to replace all integration tests with this framework in the future as it would likely be more robust and cause less issues such as goroutine leaks during the go test run. However, for the time being we will have both integration tests in Go and Ruby.

The DataWriterHelper is a straight port from data_writer.go, which is a multithreaded data writer that writes random data to the database. The DbHelper helps connects to the database, seed it, and verify that the source matches the target so that tests don't have to implement it over and over again.

Added some trivial integration tests and some interrupt/resume tests. Note the interrupt and resume works on master as of this commit without reconciler because the schema didn't change.

shuhaowu · 2019-01-15T21:07:26Z

I've updated this PR to follow more of a Ruby convention:

Uses bundle exec rake test
require "test_helper"
Removed GhostferryIntegration module and moved functionality into *Helper modules that are included in GhostferryTestCase.
Avoided implicit instance variables like @ghostferry and force tests to create their own instances of ghostferry via ghostferry = new_ghostferry(minimal); ghostferry.run.
Avoided ambiguously named functions like use_datawriter in favour of start_datawriter_with_ghostferry(datawriter, ghostferry); stop_datawriter_during_cutover(datawriter, ghostferry)
Used WEBrick::HTTPServer instead of UNIX socket
Moved all go test code from test/*.go into test/go/*.go

shuhaowu · 2019-01-15T21:08:42Z

test/helpers/data_writer_helper.rb

+    end
+  end
+
+  class DataWriter


Much of this is translated directly from data_writer.go. The goal is eventually we can use Ruby tests only.

shuhaowu · 2019-01-15T21:09:14Z

test/helpers/db_helper.rb

+    [source_metrics, target_metrics]
+  end
+
+  def table_metric(conn, table, sample_id: nil)


Not a big fan of this name, any ideas?

hkdsun

So much better 👍 I found the Ghostferry helper very hard to read still but that can be massaged as we go along maybe.

Looks good to go functionally though!

hkdsun · 2019-01-16T18:38:40Z

test/test_helper.rb

+    assert_equal(
+      source[DEFAULT_FULL_TABLE_NAME][:sample_row],
+      target[DEFAULT_FULL_TABLE_NAME][:sample_row],
+    )


Isn't the checksum assertion sufficient? I'm having trouble understanding where you're going with the sample rows

The checksum is likely sufficient and the row count (especially since you can have a checksum of 0 if a table is empty), I'm just being defensive. I'll remove this line.

hkdsun · 2019-01-16T18:43:04Z

test/test_helper.rb

+    refute dumped_state["LastSuccessfulPrimaryKeys"].nil?
+    refute dumped_state["CompletedTables"].nil?
+    refute dumped_state["LastWrittenBinlogPosition"].nil?
+  end


This seems a bit odd to have as a global helper and could easily go out of date. You could make it a private method where it's being used right now for the first concern. For the going out of date... maybe we need to write some negative tests? i.e. "if i remove field X, then we can't resume correctly?" or something like that?

hkdsun · 2019-01-16T18:51:59Z

test/lib/go/minimal.go

+	if err != nil {
+		panic(err)
+	}
+}


Maybe keep IntegrationFerry in the same file for now? Or did you have a purpose in mind for having a separate IntegrationFerry abstraction ?

hkdsun · 2019-01-16T19:00:30Z

test/helpers/ghostferry_helper.rb

+    end
+
+    # When using this method, you need to call it within the block of
+    # GhostferryIntegration::TestCase#with_state_cleanup to ensure that the


with_state_cleanup doesn't exist?

hkdsun · 2019-01-16T20:36:54Z

test/integration/interrupt_resume_test.rb

+    end
+
+    dumped_state = ghostferry.run_expecting_interrupt
+    assert_basic_fields_exist_in_dumped_state(dumped_state)


can't we assert more than that at this point?

Do you mean to assert the state of the database? It's difficult to know exactly what's going on due to the semi-randomness of the data writer and binlog streamer.

I'll try to add a test without the datawriter where we can better check the state of the target database.

hkdsun · 2019-01-16T20:37:40Z

test/integration/interrupt_resume_test.rb

+
+    start_datawriter_with_ghostferry(datawriter, ghostferry)
+    stop_datawriter_during_cutover(datawriter, ghostferry)
+    ghostferry.run(dumped_state)


assert_test_table_is_identical ?

hkdsun · 2019-01-16T20:38:29Z

test/integration/interrupt_resume_test.rb

+
+class InterruptResumeTest < GhostferryTestCase
+  def test_interrupt_resume_with_writes_to_source
+    seed_simple_database_with_single_table


def setup?

hkdsun · 2019-01-16T20:48:22Z

test/lib/go/integrationferry/integrationferry.go

+	// These should be kept in sync with ghostferry.rb
+	portEnvName    string        = "GHOSTFERRY_INTEGRATION_PORT"
+	timeout        time.Duration = 30 * time.Second
+	maxMessageSize int           = 256


this is unused

hkdsun · 2019-01-16T20:48:59Z

test/helpers/ghostferry_helper.rb

+
+    # Keep these in sync with integrationferry.go
+    ENV_KEY_PORT = "GHOSTFERRY_INTEGRATION_PORT"
+    MAX_MESSAGE_SIZE = 256


unused as well

hkdsun · 2019-01-16T20:53:55Z

test/lib/go/integrationferry/integrationferry.go

+	StatusRowCopyCompleted       string = "ROW_COPY_COMPLETED"
+	StatusDone                   string = "DONE"
+
+	// Could be sent by multiple goroutines in parallel


does this distinction matter?

shuhaowu · 2019-01-21T16:40:51Z

I've addressed all the feedback as well as slightly reorganized the ghostferry_helper module with some comments to make it less all over the place. Hopefully that will help with some of the readability. We can improve it incrementally as we go forward.

shuhaowu force-pushed the ruby-integration-test branch from 558c297 to 080c76a Compare September 18, 2018 14:45

shuhaowu changed the title ~~Integration tests by compiling and calling Ghostferry from Ruby~~ [WIP] Integration tests by compiling and calling Ghostferry from Ruby Sep 25, 2018

shuhaowu force-pushed the ruby-integration-test branch from 080c76a to 78bdc7d Compare September 25, 2018 19:26

shuhaowu force-pushed the ruby-integration-test branch from 78bdc7d to 6198812 Compare October 2, 2018 20:43

shuhaowu force-pushed the ruby-integration-test branch 2 times, most recently from 68f4f0a to 23a602d Compare October 30, 2018 19:03

shuhaowu mentioned this pull request Nov 6, 2018

Ghostferry interrupt/resume meta issue #75

Closed

shuhaowu force-pushed the ruby-integration-test branch 5 times, most recently from a456cd5 to 2b25a1d Compare November 20, 2018 03:16

shuhaowu force-pushed the ruby-integration-test branch 2 times, most recently from 3f45878 to e4c88ce Compare November 20, 2018 19:12

shuhaowu changed the title ~~[WIP] Integration tests by compiling and calling Ghostferry from Ruby~~ Integration tests by compiling and calling Ghostferry from Ruby Nov 20, 2018

shuhaowu force-pushed the ruby-integration-test branch from e4c88ce to 2ef8987 Compare November 20, 2018 20:40

shuhaowu requested review from pushrax and hkdsun November 20, 2018 20:52

fjordan reviewed Jan 9, 2019

View reviewed changes

hkdsun reviewed Jan 9, 2019

View reviewed changes

shuhaowu force-pushed the ruby-integration-test branch 3 times, most recently from 15c373e to 3901412 Compare January 15, 2019 15:03

shuhaowu added 4 commits January 15, 2019 10:06

Moved Go test files from test/*.go to test/go/*.go

ea7428b

Makes room to make the tests more like ruby tests, which will be employed heavily.

Added some integration tests

f1465a7

Added some trivial integration tests and some interrupt/resume tests. Note the interrupt and resume works on master as of this commit without reconciler because the schema didn't change.

shuhaowu force-pushed the ruby-integration-test branch from 3901412 to f1465a7 Compare January 15, 2019 15:18

Switched to HTTP callbacks instead of socket

bd60675

shuhaowu force-pushed the ruby-integration-test branch from 0e2a271 to bd60675 Compare January 15, 2019 20:54

shuhaowu commented Jan 15, 2019

View reviewed changes

hkdsun approved these changes Jan 16, 2019

View reviewed changes

shuhaowu force-pushed the ruby-integration-test branch from d3b3280 to 652fdfd Compare January 21, 2019 16:31

Updated for comments

b8fb91a

shuhaowu force-pushed the ruby-integration-test branch from 652fdfd to b8fb91a Compare January 21, 2019 17:34

shuhaowu merged commit 07831b1 into Shopify:master Jan 21, 2019

shuhaowu deleted the ruby-integration-test branch January 21, 2019 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration tests by compiling and calling Ghostferry from Ruby #64

Integration tests by compiling and calling Ghostferry from Ruby #64

shuhaowu commented Sep 17, 2018 •

edited

shuhaowu commented Jan 8, 2019

fjordan left a comment •

edited

fjordan Jan 9, 2019

shuhaowu Jan 14, 2019

fjordan Jan 9, 2019 •

edited

hkdsun Jan 9, 2019

hkdsun left a comment

hkdsun commented Jan 9, 2019

shuhaowu commented Jan 15, 2019

shuhaowu Jan 15, 2019

shuhaowu Jan 15, 2019

hkdsun left a comment

hkdsun Jan 16, 2019

shuhaowu Jan 21, 2019

hkdsun Jan 16, 2019

hkdsun Jan 16, 2019

hkdsun Jan 16, 2019

hkdsun Jan 16, 2019

shuhaowu Jan 21, 2019

hkdsun Jan 16, 2019

hkdsun Jan 16, 2019

hkdsun Jan 16, 2019

hkdsun Jan 16, 2019

hkdsun Jan 16, 2019

shuhaowu commented Jan 21, 2019

Integration tests by compiling and calling Ghostferry from Ruby #64

Integration tests by compiling and calling Ghostferry from Ruby #64

Conversation

shuhaowu commented Sep 17, 2018 • edited

shuhaowu commented Jan 8, 2019

fjordan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjordan Jan 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkdsun left a comment

Choose a reason for hiding this comment

hkdsun commented Jan 9, 2019

shuhaowu commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkdsun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuhaowu commented Jan 21, 2019

shuhaowu commented Sep 17, 2018 •

edited

fjordan left a comment •

edited

fjordan Jan 9, 2019 •

edited