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

vmware_cluster_ha - add set of heartbeat datastores #1732

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Nina2244
Copy link
Contributor

SUMMARY

Add the possibility to set the heartbeat datastores of an cluster.

Is the new Pullrequest for #1663.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

vmware_cluster_ha

ADDITIONAL INFORMATION

@mariolenz
Copy link
Collaborator

recheck

@Nina2244
Copy link
Contributor Author

recheck

@mariolenz
Copy link
Collaborator

recheck

@mariolenz
Copy link
Collaborator

It looks like your changes break this test for idempotency:

- name: Change APD response to "restartAggressive" again (idempotency)
vmware_cluster_ha: *change_apd_response
register: change_apd_response_again
- assert:
that:
- not change_apd_response_again.changed

@Nina2244
Copy link
Contributor Author

recheck

@mariolenz
Copy link
Collaborator

@Nina2244 Finally, I found the time to test this. However, I'm not really sure if the module works as intended.

If a cluster already has a list of hearbeat datastores defined, it seems to work fine. That is, if the configured list of datastores differs from the one that is defined, this will be remediated.

But if no heartbeat datastores are configured, defining heartbeat_datastores doesn't have an effect.

On they other hand, if there is a list of heartbeat datastores configured but not defined in heartbeat_datastores, the module always reports a change. But there isn't really changed anything at all.

@Nina2244
Copy link
Contributor Author

@mariolenz really nice that you tested the other both cases. I will look for it why there is no change in both cases.

@mariolenz
Copy link
Collaborator

@Nina2244 Since I'm beginning to think that we should not introduce new parameters with default behavior (this might break existing playbooks) I wonder how this new parameter heartbeat_datastores should work. My suggestion:

  • If unset (None) just ignore the current list of heartbeat datastores and don't do any configuration there
  • If set to the empty list [] configure HA to use all datastores for heartbeat
  • If there are datastores configured, make sure this are the datastores HA considers to use for heartbeats

What do you think?

BTW, I think you can compare both lists directly. Or, since there's not really an order to the datastores, you might want to make a set out of them first. So you could check for set(das_config.heartbeatDatastore) == set(self.heartbeat_datastores). This way, if the vCenter reports datastores 1, 2 and 3 but the playbook defines 3, 2 and 1 there won't be a change. I'm not 100% sure, though...

@Nina2244
Copy link
Contributor Author

@mariolenz i added the thing with None, and with the empty list, but I not really understand what you mean with your third point.

@mariolenz
Copy link
Collaborator

recheck

Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

The CI is still making trouble, but I hope we'll get it into 3.8.

description:
- A list of the heartbeat datastores.
- If list ist [] then all datastores of the cluster will be set as heartbeat_datastores.
type: list
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type: list
version_added: '3.8.0'
type: list

- allFeasibleDs -> Automatically select datastores accessible from the hosts. Causes that C(heartbeat_datastores) will be ignored.
- userSelectedDs -> Use datastores only from the specified list.
- allFeasibleDsWithUserPreference -> Use datastores from the specified list and complement automatically if needed.
type: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type: str
version_added: '3.8.0'
type: str

@softwarefactory-project-zuul
Copy link

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/e1c3ff8cbf384ca781875bcc4fd0871b

✔️ ansible-tox-linters SUCCESS in 9m 23s
✔️ build-ansible-collection SUCCESS in 10m 21s
ansible-test-cloud-integration-vcenter7_only-stable215 NODE_FAILURE Node request 200-0006211408 failed in 0s
ansible-test-cloud-integration-vcenter7_2esxi-stable215 NODE_FAILURE Node request 200-0006211409 failed in 0s
ansible-test-cloud-integration-vcenter7_1esxi-stable215_1_of_2 NODE_FAILURE Node request 200-0006211410 failed in 0s
ansible-test-cloud-integration-vcenter7_1esxi-stable215_2_of_2 NODE_FAILURE Node request 200-0006211411 failed in 0s
✔️ ansible-galaxy-importer SUCCESS in 4m 52s

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