Add etcd release compatibility tests #14610
Conversation
Codecov Report
@@ Coverage Diff @@
## main #14610 +/- ##
==========================================
- Coverage 75.50% 75.45% -0.05%
==========================================
Files 457 457
Lines 37299 37299
==========================================
- Hits 28163 28145 -18
- Misses 7369 7380 +11
- Partials 1767 1774 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
It seems you are doing multiple things in one PR. Please try to break down this PR, and each PR should only do one thing. Such as renaming |
| @@ -34,4 +42,10 @@ type ClusterConfig struct { | |||
| DisableStrictReconfigCheck bool | |||
| AuthToken string | |||
| SnapshotCount int | |||
|
|
|||
| E2eConfig *E2eClusterConfig | |||
There was a problem hiding this comment.
It's weird to see e2e config in the common ClusterConfig, which shouldn't care about the test infra/env (e2e or integration) at all.
I would suggest to move all the code from common area to e2e packages.
There was a problem hiding this comment.
This PR maintains invariant that tests in common do not care about cluster setup. However this should not be misunderstood that common framework itself doesn't care about e2e configuration. Goal of common framework is to abstract cluster setup so we can write a single test that works can be used as integration, e2e or upgrade.
There was a problem hiding this comment.
I would suggest to move all the code from common area to e2e packages.
Have you considered this?
There was a problem hiding this comment.
Not sure if I understand what you mean by "all code". I think that putting E2eConfig into framework.Config config is a good decision as it makes it explicitly part of cluster configuration. I think it's more important that maintaining framework.Config generic.
If we want to create a cluster with different server binaries, it should be part of cluster config, even if we cannot setup such cluster during integration tests. That's why instrumentation tests will immidietly panic when they discover E2eConfig set.
As for other code added to package, I cannot move it to e2e because it would create an import cycle.
There was a problem hiding this comment.
Points:
- Just as the comment Add etcd release compatibility tests #14610 (comment) I raised previously, let's try to do one thing in one PR. If you get the change on
e2eClusterTestCases()in a separate PR, it might have already been approved and merged. - "all code" means all code related to
E2eConfig, which means it doesn't include the code related toe2eClusterTestCases()mentioned above. - Regarding to
it should be part of cluster config, why not to add whatever config specific to e2e test intoEtcdProcessClusterConfigand test cases specific to e2e intoe2epackage? It's really weird to addE2eConfiginto the commonClusterConfig. - With regard to "
However this should not be misunderstood that common framework itself doesn't care about e2e configuration", I don't agree this. The diagram I provided about 9 months ago is still valid. FYI. Unify testing framework #13637 (comment)
There was a problem hiding this comment.
Points:
- Just as the comment Add etcd release compatibility tests #14610 (comment) I raised previously, let's try to do one thing in one PR. If you get the change on
e2eClusterTestCases()in a separate PR, it might have already been approved and merged.
It doesn't make sense to move e2eClusterTestCases as splitting integration and e2e scenarios is not nesesery if we change implementation of this PR.
- "all code" means all code related to
E2eConfig, which means it doesn't include the code related toe2eClusterTestCases()mentioned above.
As mentioned above code for E2eConfig cannot be moved because of import cycles.
- Regarding to
it should be part of cluster config, why not to add whatever config specific to e2e test intoEtcdProcessClusterConfigand test cases specific to e2e intoe2epackage? It's really weird to addE2eConfiginto the commonClusterConfig.
When thinking about unifying testing via common framework we need to separate test scenario from cluster setup (need to work on naming to avoid confusion). By test scenario I mean a test like "TestPutKV" it just takes a cluster, puts a key and verifies that it's there. Test scenario itself doesn't care how cluster was created.
This brings me to testing clusters with servers having different versions. This is not a test scenario, but a cluster setup. It doesn't make sense to have a specific test scenarios just because it requires some specific e2e configuration. We can reuse all existing common test scenarios and run them against the specific e2e cluster configuration.
You keep saying that ClusterConfig is common, but it's not part of common package not it was meant to be generic. For now it only has properties shared by both e2e and integration tests, but that just because we haven't migrated any test that would require a e2e specific cluster setup.
- With regard to "
However this should not be misunderstood that common framework itself doesn't care about e2e configuration", I don't agree this. The diagram I provided about 9 months ago is still valid. FYI. Unify testing framework #13637 (comment)
Thank's for diagram, however it's not set in stone truth. Original proposal didn't set such limitations.
There was a problem hiding this comment.
Don't you think ClusterConfig is a common struct, which is used by both e2e and integration test, and it's an anti-pattern for integration checks something it shouldn't care about at all?
There was a problem hiding this comment.
There are two parts we need to agree on:
- Should hybrid version tests be part of common tests.
- How to integrate E2e config into common tests. Let's discuss this after we agree on the first point.
First, hybrid version clusters are expected to behave exactly like a normal cluster. This means that we can and should run exactly the same tests as we do for other tests. Common tests are just a blackbox tests that replay scenarios that should work no matter if we use client library, etcdctl, single cluster node or multi cluster cluster node. They should also work in hybrid clusters (minus new features but however we also test protection against inconsistency during upgrade due to usage of new features).
Alternative that you proposed to have those tests as e2e does not scale. We would again be in a situation that we need to copy each and every test just change one line in configuration. Instead of thinking of hybrid clusters as a separate special test we should think it as a normal cluster that just has a different setup.
Does it make sense?
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
cc @ahrtr