Skip to content

Conversation

petyaslavova
Copy link
Collaborator

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Please provide a description of the change here.

@petyaslavova petyaslavova force-pushed the ps_hitless_add_e2e_tests branch 4 times, most recently from 98d13c8 to 394c020 Compare September 1, 2025 17:32
…ons - second try + some test helpers improvements
@petyaslavova petyaslavova force-pushed the ps_hitless_add_e2e_tests branch from 394c020 to eaeedc1 Compare September 1, 2025 17:41
tuple: (target_node, empty_node) where target_node has master shards
and empty_node has no shards
"""
cluster_info = ClusterOperations.get_cluster_nodes_info(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there's a specific reason why you decided to use rladmin and not the REST API here? There's v1/shards endpoint that returns you information about shards so you can easier find the node with master shards and nodes without shards.

rladmin is more an internal tool, whereas REST API is public so I would suggest to use it for this purposes. Since, rladmin is a plain text we might encounter parsing issues if it will be changed whereas REST API returns standardised JSON object

https://redis.io/docs/latest/operate/rs/references/rest-api/requests/shards/#example-json-body

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reason.
We were using this for our testing for a long time + the demo, so thats why we stuck to that as we know it worked.
The API requests were less explored.
I am ok to switch to that approach instead, although it will take some time to refactor the code.

endpoint_id: str,
) -> str:
"""Execute rladmin bind endpoint command and wait for completion."""
command = f"bind endpoint {endpoint_id} policy single"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same achievable with REST API

curl -X PUT \
  -H "accept: application/json" \
  -H "Content-Type: application/json" \
  -u "[username]:[password]" \
  https://[host]:9443/v1/bdbs/[database_id] \
  -d '{
    "proxy_policy": "all-master-shards",
    "endpoint_ip": "[specific_ip]",
    "port": [port_number]
  }' \
  -k -i

empty_node: str,
) -> str:
"""Execute rladmin migrate command and wait for completion."""
command = f"migrate node {target_node} all_shards target_node {empty_node}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same achievable with REST API

curl -X POST \
  -H "accept: application/json" \
  -H "Content-Type: application/json" \
  -u "[username]:[password]" \
  https://[host]:9443/v1/shards/actions/migrate \
  -d '{
    "shard_uids": ["2","4","6"],
    "target_node_uid": 9,
    "override_rack_policy": false,
    "preserve_roles": false,
    "max_concurrent_bdb_migrations": 3
  }' \
  -k -i

@petyaslavova petyaslavova force-pushed the ps_hitless_add_e2e_tests branch from 961ebce to 015550b Compare September 3, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants