HCD-267: Fix the node UP/DOWN detection for DSE nodes#3
HCD-267: Fix the node UP/DOWN detection for DSE nodes#3szymon-miezal wants to merge 11 commits intoconverged-cassandrafrom
Conversation
szymon-miezal
force-pushed
the
HCD-267
branch
from
6f49864 to
a822d8b
Compare
| self._update_config() | ||
|
|
||
| def enable_internode_ssl(self, node_ssl_path): | ||
| def enable_internode_ssl(self, node_ssl_path, enable_legacy_ssl_storage_port=False): |
There was a problem hiding this comment.
A convenient change needed for testing DSE upgrades
|
|
||
| if wait_other_notice: | ||
| marks = [(node, node.mark_log()) for node in list(self.cluster.nodes.values()) if node.is_live()] | ||
| marks = [(node, node.mark_log()) for node in list(self.cluster.nodes.values()) if node.is_live() and node is not self] |
There was a problem hiding this comment.
Is there are reason we should be parsing our own logs?
szymon-miezal
force-pushed
the
HCD-267
branch
4 times, most recently
from
e32b8c0 to
b62218f
Compare
ccmlib/cluster.py
Outdated
| 'truststore': os.path.join(self.get_path(), 'internode-truststore.jks'), | ||
| 'truststore_password': 'cassandra' | ||
| 'truststore_password': 'cassandra', | ||
| 'enable_legacy_ssl_storage_port': enable_legacy_ssl_storage_port |
There was a problem hiding this comment.
I think the addition of this flag should be conditional
michaelsembwever
force-pushed
the
converged-cassandra
branch
from
d3ce9f6 to
1d07651
Compare
…tion - Add support for enable_legacy_ssl_storage_port in internode SSL setup - Fix address_for_version() to return address:port format for DSE 4.0+ - Override watch_log_for_alive() for DSE-specific log format - Add watch_log_for_death() method for DSE nodes - Fix wait_other_notice to exclude self from notification marks
Add enable_legacy_ssl_storage_port only if the caller explicitly asked for it.
|
|
||
| if self.cassandra_version() >= '4.0': | ||
| # Guard against incompatible settings for <DSE_6.9 | ||
| if self.getNodeClass() is DseNode and self.version() >= '6.9': |
There was a problem hiding this comment.
instead of putting it here and using conditional logic, why not put it in the overridden method in DseCluster.py like
def enable_internode_ssl(self, node_ssl_path, enable_legacy_ssl_storage_port=False):
if self.version() >= '6.9' and enable_legacy_ssl_storage_port:
node_ssl_options['enable_legacy_ssl_storage_port'] = enable_legacy_ssl_storage_port
super().enable_internode_ssl(node_ssl_path, enable_legacy_ssl_storage_port)
(then remove the DseNode import – best this class knows nothing of specific subclasses)
…dk versions NEEDS TESTING
ccmlib/common.py
Outdated
| # if we detect DSE then we try to parse VERSION.txt file | ||
| if os.path.exists(os.path.join(install_dir, BIN_DIR, 'dse')): | ||
| path_to_version = os.path.join(install_dir, 'VERSION.txt') | ||
| if os.path.exists(path_to_version): | ||
| with open(path_to_version, 'r') as file: | ||
| v = file.readline().strip() | ||
| if LooseVersion(v) >= LooseVersion('6.9'): | ||
| # DSE 6.9 supports only Java 11 | ||
| return [11] | ||
| else: | ||
| # it's DSE 6.8 or earlier, only Java 8 is supported | ||
| return [8] |
There was a problem hiding this comment.
i'm not fond of putting dse specific code into common.py (given all the recent effort to modularise the codebase).
my suggested approach to this has been added as a SQUASH commit 4c43a80 . @szymon-miezal , you're feel to accept it or not, but i do think it's the cleaner (more modular) approach. it needs testing, i moved the VERSION.txt parsing to the existing method higher up and earlier in the call chain where i believe it should be done…
|
with the changes i've suggested (in the comment and in the added squash commit), to keep dse code out of the base package, i'd would be happy to merge this upstream. if we agree then i'll be happy to create the jira ticket and do the rebase so it's made available immediately. |
3 participants
The main motivation for this patch is changing the way logs are parsed for node UP/DOWN status.
DSE does not display nodes' addresses with port, thus the tests were failing to parse the logs correctly.
Opportunistically the patch does two more changes:
enable_legacy_ssl_storage_portwhile enabling the inter-node encryption.Test results: http://10.169.74.112:8081/job/ds-cassandra-build/2104/testReport/
Baseline test results: http://10.169.74.112:8081/job/ds-cassandra-build/10/