-
Notifications
You must be signed in to change notification settings - Fork 111
improvement(kmip): change KMIP service from hytrust to PyKMIP #12079
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
base: master
Are you sure you want to change the base?
Conversation
Remove dependency on persistent HyTrust KMIP server This change replaces the persistent HyTrust KMIP server with a per-job PyKMIP server deployed on the monitoring node. This setup is similar to Scylla Manager, which is also deployed there. Additionally, this commit adds a wait step during Scylla startup in BaseScyllaCluster, since Scylla requires the KMIP server to be running before it can start.
|
Please note that this is just a proposal.
|
|
@scylladb/qa-maintainers could you please check if this solution looks correct and share your opinion on it? |
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.
why this file is being added into the source ?
| self.log.debug("Using round_robin for multiple Keyspaces...") | ||
| for i in range(1, keyspace_num + 1): | ||
| keyspace_name = self._get_keyspace_name(i) | ||
| keyspace_name = "10gb_sizetiered_2025_3" |
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.
is this for testing only, I guess ?, please remove (or put on it's own commit)
| @raise_event_on_failure | ||
| def node_startup(_node: BaseNode, task_queue: queue.Queue): | ||
| exception_details = None | ||
| node.remoter |
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.
???
| node.remoter.run("cd /home/ubuntu/pykmip && " | ||
| "curl -O https://raw.githubusercontent.com/jsmolar/PyKMIP/master/docker-compose.yaml") | ||
| node.remoter.sudo("chmod 777 /home/ubuntu/pykmip/data/logs") | ||
| node.remoter.run("cd /home/ubuntu/pykmip && docker compose up -d --scale pykmip=10") |
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.
10 ? do we really need that many instances ?
| self.install_scylla_manager(node) | ||
| self.install_pykmip(node) | ||
|
|
||
| def install_pykmip(self, node): |
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.
this one doesn't just install, but also run it
I'm not sure it is wise to set it up on the monitor node, I would suggest putting it on the test node (i.e. the SCT runner)
we already have one example of docker-compose based setup running (kafka-stack-docker-compose)
I would suggest align it with that implementation
fruch
left a comment
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.
- Move https://github.com/jsmolar/pykmip into scylla org.
- move installation into sct-runner
- why we need HA for it ? it's not that we gonna use this setup for multiple tests at the same time ?
- remove test related change out of the PR
|
|
||
| # 1GB dataset | ||
| prepare_write_cmd: ["cassandra-stress write cl=ONE n=1048576 -schema 'replication(strategy=NetworkTopologyStrategy,replication_factor=3) compaction(strategy=LeveledCompactionStrategy)' -mode cql3 native -rate threads=50 -col 'size=FIXED(1024) n=FIXED(1)' -pop seq=1..1048576"] | ||
| prepare_write_cmd: ["cassandra-stress write cl=ONE n=1048576 -schema 'keyspace=10gb_sizetiered_2025_3 replication(strategy=NetworkTopologyStrategy,replication_factor=3) compaction(strategy=LeveledCompactionStrategy)' -mode cql3 native -rate threads=50 -col 'size=FIXED(1024) n=FIXED(1)' -pop seq=1..1048576"] |
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.
Please avoid hardcoding the keyspace name in yaml test-case which is supposed to be used across several tests.
Also, there some issues with current implementation:
- C-S operation will write 1Gb of data only, not 10Gb as mentioned in keyspace name
- consecutive C-S read would fail as it doesn't expect
10gb_sizetiered_2025_3keyspace
If your idea was to prepare some backup snapshot for later utilization, please, take a look at the test cases yamls from here https://github.com/scylladb/scylla-cluster-tests/tree/master/test-cases/manager/prepare_snapshot
Why static keys at all ? Can we generate fresh key on each run like do for client encryption?
|
Fixes #8806
Remove dependency on the persistent HyTrust KMIP server
This change replaces the persistent HyTrust KMIP server with a per-job PyKMIP server deployed on the monitoring node. This setup is similar to Scylla Manager, which is also deployed there.
Additionally, this commit adds a wait step during Scylla startup in BaseScyllaCluster, since Scylla requires the KMIP server to be running before it can start.
I had to make some modifications to PyKMIP, since the upstream repository is no longer maintained: https://github.com/jsmolar/pykmip. These include adding PostgreSQL support and several minor adjustments.
On the monitoring node, PyKMIP is deployed via docker-compose, which launches 10 PyKMIP instances behind a single HAProxy load balancer, all backed by PostgreSQL.
Because PyKMIP is not designed for enterprise-scale load, this approach provides better scalability and reliability under high demand. It also eliminates the need for an expensive license and a permanently running HyTrust KMIP server.
Testing
PR pre-checks (self review)
backportlabelsReminders
sdcm/sct_config.py)unit-test/folder)