-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update CETS node discovery and DB migrations #4255
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4255 +/- ##
==========================================
+ Coverage 84.42% 84.43% +0.01%
==========================================
Files 552 552
Lines 33546 33547 +1
==========================================
+ Hits 28322 28327 +5
+ Misses 5224 5220 -4 ☔ View full report in Codecov by Sentry. |
6eb50c8
to
aaace49
Compare
This comment was marked as outdated.
This comment was marked as outdated.
aaace49
to
bca629b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bca629b
to
7b1f409
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Otherwise, if you move the node to another cluster, old cluster would still connect to it, and the whole system would be in an inconsistent state. In particular, this change allows split-cluster rolling upgrade, where a new cluster is gradually formed from the restarted nodes.
This ensures consistency: each node appears only once.
- Add missing 'caps' table - Update primary key for 'discovery_nodes'
- Add missing 'caps' table - Update primary key for 'discovery_nodes'
- Add missing 'caps' table - Update primary key for 'discovery_nodes' It is necessary to use a dynamic query because primary key is unnamed. - Fix mistakes, e.g. missing 'NOT NULL', syntax error for 'ALTER TABLE'. All migrations tested manually.
7b1f409
to
674e769
Compare
This comment was marked as outdated.
This comment was marked as outdated.
- Add test for moving nodes to a new cluster - Make sure it's not possible to have duplicate node name - Reduce verbosity by using more helpers
They are flaky (at least locally) if you try to run many at once.
674e769
to
cb92b6a
Compare
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / cb92b6a small_tests_25 / small_tests / cb92b6a small_tests_26 / small_tests / cb92b6a small_tests_26_arm64 / small_tests / cb92b6a ldap_mnesia_25 / ldap_mnesia / cb92b6a dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / cb92b6a ldap_mnesia_26 / ldap_mnesia / cb92b6a dynamic_domains_mysql_redis_26 / mysql_redis / cb92b6a dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / cb92b6a internal_mnesia_26 / internal_mnesia / cb92b6a pgsql_cets_26 / pgsql_cets / cb92b6a graphql_SUITE:tls_enabled:tls_connect_admin_unknown_certificate{error,{{assertMatch,[{module,graphql_SUITE},
{line,253},
{expression,"Result"},
{pattern,"{ error , { tls_alert , { unknown_ca , _ } } }"},
{value,{error,connection_closed}}]},
[{graphql_SUITE,tls_connect_admin_unknown_certificate,1,
[{file,"/home/circleci/project/big_tests/tests/graphql_SUITE.erl"},
{line,253}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1793}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1302}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1234}]}]}} dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / cb92b6a pgsql_mnesia_25 / pgsql_mnesia / cb92b6a pgsql_mnesia_26 / pgsql_mnesia / cb92b6a mysql_redis_26 / mysql_redis / cb92b6a mssql_mnesia_26 / odbc_mssql_mnesia / cb92b6a pgsql_cets_26 / pgsql_cets / cb92b6a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
This PR makes sure that when you move a node to another cluster, it is not a part of the old cluster anymore.
Previously, it would be a part of both clusters in the
discovery_nodes
table until the next cleanup (1h by default).Additionally, this PR makes it possible to do a "split-cluster" rolling upgrade, keeping the new and old parts of the cluster disjoint.
Since I needed to test migrations, I figured out several missing and broken parts in them, so I fixed them as well.
It looked to me that the MSSQL migration was untested because of syntax errors.
After this PR, the migration guide should be ready for the next release - only the files will be renamed in the
rel-6.2
branch.