[jira] [Comment Edited] (KAFKA-9323) Refactor Streams' upgrade system tests

["Sophie Blee-Goldman (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/KAFKA-9323?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17016420#comment-17016420 ] 

Sophie Blee-Goldman edited comment on KAFKA-9323 at 1/16/20 12:58 AM:
----------------------------------------------------------------------

Hey [~nizhikov], thanks for picking this up! The relevant files to this are streams_cooperative_rebalance_upgrade_test.py and streams_upgrade_test.py

We don't have this upgrade matrix written down anywhere, at the moment this information is pretty scattered around the system tests and the upgrade guide. This makes it difficult for maintainers to understand and update the tests correctly, and difficult for users to discover how to upgrade between specific versions. We should really add this to the docs in addition to the system tests... 

Besides compiling this matrix, we definitely need to add a new version probing test that tests upgrading TO the current version. And obviously, we should only run this test for upgrades that actually require version probing, but _don't_ require a cooperative upgrade (since that will include VP) – note this means we can probably just copy over much of the code from the first half of the cooperative test. I'd personally want to call this new test test_version_probing_upgrade and then rename the existing VP test to test_future_version_probing_upgrade (will refer to these like this going forward)

Then, we should refactor the tests to leverage a single source of truth (the upgrade matrix) to determine which upgrade path should be followed for a given to/from version. Each version combination should be run by _exactly one_ system test.

I think how exactly to go about this is up for some discussion (and I'm sure [~mjsax] has strong opinions on it :P ) but I would propose to have a single test_upgrade system test parametrized by all versions (with to > from) that just calls the appropriate upgrade path by looking it up in the upgrade matrix. This way it can share all the setup, etc and the actual individual upgrades can isolate and focus on the specific upgrade path it's testing. Each entry would point to one of the following functions, which contain just the upgrade logic extracted from the corresponding system test. 
 * existing test_simple_upgrade_downgrade -> do_simple_upgrade(to, from):
 * cooperative_rebalance_upgrade_test -> do_cooperative_upgrade(to, from):
 * test_metadata_upgrade -> do_metadata_upgrade(to, from):
 * test_version_probing_upgrade (the new one, not the "future" one) -> do_version_probing_upgrade(to, from):

We can list all the "rules" for determining which upgrade path above the upgrade matrix, so maintainers will only need to look at one place to update as new versions come out. This way we are guaranteed to test all possible upgrades, and do so according to the correct upgrade path. We also make it _much_ easier and faster for developers working on new features to add an upgrade path to the system tests, and be sure it will be followed (when appropriate)


was (Author: ableegoldman):
Hey [~nizhikov], thanks for picking this up! The relevant files to this are streams_cooperative_rebalance_upgrade_test.py and streams_upgrade_test.py

We don't have this upgrade matrix written down anywhere, at the moment this information is pretty scattered around the system tests and the upgrade guide. This makes it difficult for maintainers to understand and update the tests correctly, and difficult for users to discover how to upgrade between specific versions. We should really add this to the docs in addition to the system tests... 

 

Besides compiling this matrix, we definitely need to add a new version probing test that tests upgrading TO the current version. And obviously, we should only run this test for upgrades that actually require version probing, but _don't_ require a cooperative upgrade (since that will include VP) – note this means we can probably just copy over much of the code from the first half of the cooperative test. I'd personally want to call this new test test_version_probing_upgrade and then rename the existing VP test to test_future_version_probing_upgrade (will refer to these like this going forward)

Then, we should refactor the tests to leverage a single source of truth (the upgrade matrix) to determine which upgrade path should be followed for a given to/from version. Each version combination should be run by _exactly one_ system test.

I think how exactly to go about this is up for some discussion (and I'm sure [~mjsax] has strong opinions on it :P ) but I would propose to have a single test_upgrade system test parametrized by all versions (with to > from) that just calls the appropriate upgrade path by looking it up in the upgrade matrix. This way it can share all the setup, etc and the actual individual upgrades can isolate and focus on the specific upgrade path it's testing. Each entry would point to one of the following functions, which contain just the upgrade logic extracted from the corresponding system test. 
 * existing test_simple_upgrade_downgrade -> do_simple_upgrade(to, from):
 * cooperative_rebalance_upgrade_test -> do_cooperative_upgrade(to, from):
 * test_metadata_upgrade -> do_metadata_upgrade(to, from):
 * test_version_probing_upgrade (the new one, not the "future" one) -> do_version_probing_upgrade(to, from):

We can list all the "rules" for determining which upgrade path above the upgrade matrix, so maintainers will only need to look at one place to update as new versions come out. This way we are guaranteed to test all possible upgrades, and do so according to the correct upgrade path. We also make it _much_ easier and faster for developers working on new features to add an upgrade path to the system tests, and be sure it will be followed (when appropriate)

> Refactor Streams'  upgrade system tests
> ---------------------------------------
>
>                 Key: KAFKA-9323
>                 URL: https://issues.apache.org/jira/browse/KAFKA-9323
>             Project: Kafka
>          Issue Type: Improvement
>          Components: streams
>            Reporter: Sophie Blee-Goldman
>            Assignee: Nikolay Izhikov
>            Priority: Major
>
> With the introduction of version probing in 2.0 and cooperative rebalancing in 2.4, the specific upgrade path depends heavily on the to & from version. This can be a complex operation, and we should make sure to test a realistic upgrade scenario across all possible combinations. The current system tests have gaps however, which have allowed bugs in the upgrade path to slip by unnoticed for several versions. 
> Our current system tests include a "simple upgrade downgrade" test, a metadata upgrade test, a version probing test, and a cooperative upgrade test. This has a few drawbacks and current issues:
> a) only the version probing test tests "forwards compatibility" (upgrade from latest to future version)
> b) nothing tests version probing "backwards compatibility" (upgrade from older version to latest), except:
> c) the cooperative rebalancing test actually happens to involve a version probing step, and so coincidentally DOES test VP (but only starting with 2.4)
> d) each test itself tries to test the upgrade across different versions, meaning there may be overlap and/or unnecessary tests. For example, currently both the metadata_upgrade and cooperative_upgrade tests will test the upgrade of 1.0 -> 2.4
> e) worse, a number of (to, from) pairs are not tested according to the correct upgrade path at all, which has lead to easily reproducible bugs slipping past for several versions.
> f) we have a test_simple_upgrade_downgrade test which does not actually do a downgrade, and for some reason only tests upgrading within the range [0.10.1 - 1.1]
> g) as new versions are released, it is unclear to those not directly involved in these tests and/or projects whether and what needs to be updated (eg should this new version be added to the cooperative test? should the old version be aded to the metadata test?)
> We should definitely fill in the testing gap here, but how to do so is of course up for discussion.
> I would propose to refactor the upgrade tests, and rather than maintain different lists of versions to pass as input to each different test, we should have a single matrix that we update with each new version that specifies which upgrade path that version combination actually requires. We can then loop through each version combination and test only the actual upgrade path that users would actually need to follow. This way we can be sure we are not missing anything, as each and every possible upgrade would be tested.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)