Skip to content
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

ansible-scylla-node: Remove run_once from io_properties block and fix CI #336

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

igorribeiroduarte
Copy link
Collaborator

@igorribeiroduarte igorribeiroduarte commented Feb 8, 2024

Assuming scylla_io_probe == true, this run_once had the following logic:

  • If scylla_io_probe_dc_aware was false, the block would run only once and copy the io_properties from the first node to all the others
  • If it was true the block would run for all the hosts

However, having scylla_io_probe_dc_aware set to false means that we don't know if the nodes configuration will be the same, meaning that we'd rather measure io_properties per node than copy it from the first node.

This PR fixes this problem by removing the run_once.

Now, we'll only copy io_properties to other nodes in the following scenarios:

  • if both, scylla_io_probe and scylla_io_probe_dc_aware are set to true, then we copy the io_properties from the first node of each DC to all the others of the same DC.
  • if io_properties is explicitly set via ansible params, its value will be copied to the nodes of all DCs.

Assuming `scylla_io_probe == true`, this `run_once` had the following logic:
  * If `scylla_io_probe_dc_aware` was false, the block would run only once and
    copy the io_properties from the first node to all the others
  * If it was true the block would run for all the hosts and every node
    would measure its own io_properties

However, having `scylla_io_probe_dc_aware` set to false means that we don't know
if the nodes configuration will be the same, meaning that we'd rather measure
io_properties per node than copy it from the first node.

This PR fixes this problem by removing the `run_once`.

Now, we'll only copy io_properties to other nodes in the following scenarios:
  * if both, `scylla_io_probe` and `scylla_io_probe_dc_aware` are set to `true`,
    then we copy the io_properties from the first node of each DC to all the
    others of the same DC.
  * if io_properties is explicitly set via ansible params, its value will be
    copied to the nodes of all DCs.
In the latest enterprise version, if we use the defaults from Scylla
we'll see the following error:
`init - Bad configuration: consistent_cluster_management requires schema commit log to be enabled`

This happens because consistent_cluster_management is set to true
and force_schema_commit_log is set to false by default.

This PR fixes this error in our CI by setting force_schema_commit_log
to true in scylla_yaml_params.
@igorribeiroduarte igorribeiroduarte changed the title ansible-scylla-node: Remove run_once from io_properties block ansible-scylla-node: Remove run_once from io_properties block and fix CI Feb 8, 2024
@@ -82,7 +82,6 @@
- set_fact:
io_conf: "{{ _io_conf.stdout_lines[0] }}"
when: io_properties is not defined and scylla_io_probe|bool == True and (scylla_io_probe_dc_aware|bool == False or dc_to_node_list[dc][0] == inventory_hostname)
run_once: "{{ not scylla_io_probe_dc_aware|bool }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was this run_once doing here indeed?...

Copy link
Collaborator

Choose a reason for hiding this comment

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

And this expression is strange too from Ansible syntax perspective.

@vladzcloudius vladzcloudius merged commit bfb894e into scylladb:master Feb 8, 2024
2 checks passed
@igorribeiroduarte igorribeiroduarte deleted the remove_run_once branch February 8, 2024 21:49
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.

2 participants