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: Refactor vmware_cluster into several modules #58468

Open
wants to merge 6 commits into
base: devel
from

Conversation

@mariolenz
Copy link
Contributor

commented Jun 27, 2019

SUMMARY

Refactor vmware_cluster into several modules (vmware_cluster, vmware_cluster_drs, vmware_cluster_ha and vmware_cluster_vsan) as discussed in #58023.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

vmware_cluster

ADDITIONAL INFORMATION

vmware_cluster lacks a lot of configuration options for DRS, HA and vSAN. Implementing them all in vmware_cluster would make the module hard to maintain. Therefore, splitting it into several modules and implementing the missing configuration options in them seems a good idea to me.

This is step one, refactoring vmware_cluster into several modules. Step two, implementing more configuration options for DRS, HA and vSAN, will follow.

@mariolenz mariolenz force-pushed the mariolenz:issue58023 branch from e842e21 to a6dcae8 Jun 27, 2019

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@mariolenz this PR contains more than one new module.

Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@mariolenz, just so you are aware we have a dedicated Working Group for vmware.
You can find other people interested in this in #ansible-vmware on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@mariolenz mariolenz force-pushed the mariolenz:issue58023 branch from 55f29df to 54d6d6c Jun 27, 2019

@Akasurde Akasurde changed the title Refactor vmware_cluster into several modules VMware: Refactor vmware_cluster into several modules Jun 28, 2019

@Akasurde Akasurde requested review from goneri, jillr and Akasurde Jun 28, 2019

@Akasurde Akasurde removed the needs_triage label Jun 28, 2019

@Akasurde Akasurde self-assigned this Jun 28, 2019

@mariolenz mariolenz force-pushed the mariolenz:issue58023 branch from 54d6d6c to 570c22d Jul 3, 2019

@mariolenz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@Akasurde @goneri @jillr I rebased this PR to the current devel branch, still looks OK to me.

@jillr
Copy link
Contributor

left a comment

Thanks for this patch @mariolenz. On a first pass this looks generally reasonable, I'll try to give it a second pass next week.
We'll need integration tests for the new modules, and a porting guide entry in docs/docsite/rst/porting_guides/porting_guide_2.9.rst.

Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_cluster.py Outdated
@mariolenz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

We'll need integration tests for the new modules

I haven't written any tests yet... I'll have to work on this. As far as I know, you use govcsim(?) for your tests and I'm not sure if this really simulates the whole vCenter API. I generally test against the vCSA in our test environment... but I'll try.

porting guide entry in docs/docsite/rst/porting_guides/porting_guide_2.9.rst.

I'll have a look at this, too... give me a some time.

btw: In vmware_cluster_drs, vmware_cluster_ha and vmware_cluster_vsan there is the same code:

       try:
            self.datacenter = find_datacenter_by_name(self.content, self.datacenter_name)
            if self.datacenter is None:
                self.module.fail_json(msg="Datacenter %s does not exist." % self.datacenter_name)

            self.cluster = self.find_cluster_by_name(cluster_name=self.cluster_name)
            if self.cluster is None:
                self.module.fail_json(msg="Cluster %s does not exist." % self.cluster_name)

        except vmodl.RuntimeFault as runtime_fault:
            self.module.fail_json(msg=to_native(runtime_fault.msg))
        except vmodl.MethodFault as method_fault:
            self.module.fail_json(msg=to_native(method_fault.msg))
        except Exception as generic_exc:
            self.module.fail_json(msg="Failed to check configuration"
                                      " due to generic exception %s" % to_native(generic_exc))

I think this should be moved to lib/ansible/module_utils/vmware.py. Do you want me to do it in this PR or later on?

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@mariolenz this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@mariolenz mariolenz force-pushed the mariolenz:issue58023 branch from 8166b65 to fb8ccb4 Jul 5, 2019

@mariolenz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@jillr I've written (very) basic integration tests for vmware_cluster_drs and vmware_cluster_ha. Unfortunately, govcsim seems very limited. There are some things I'd like to have tested (like idempotency) that don't work, although they work fine with a real vCenter.

I couldn't write a test for vmware_cluster_vsan, either, because govcsim doesn't really know about vSAN. I wasn't even able to check if it's enabled or not. I see a problem here: You want me to write an integration test, but I can't because govcsim doesn't simulate the required functionality. I could write one with when: vcsim is not defined if you like. But you'll need a real vCenter to run them.

@jillr

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@mariolenz OK thanks for looking into that. We're actually in the process now of building a new CI environment that will test against a real VMware environment so we can skip the things vcsim doesn't cover now, or if you're willing to write them we can just leave those tests disabled until that comes fully online.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

@mariolenz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

@jillr I've added an integration test for vmware_cluster_vsan that runs only when: vcsim is not defined. Just a thought: Maybe checking for missing integration tests is something that ansibot could do automatically, so you don't have to?

I've also added a porting guide entry. I didn't really understand the syntax but maybe I'm lucky and got it right just by chance.

@jillr

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@goneri can you please take a look at the tests in this PR?

@goneri

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@mariolenz, this is a major improvement. Thanks for the contribution. The patch changes vmware_cluster behaviour and thus breaks test/integration/targets/prepare_vmware_tests/tasks/setup_cluster.yml. Could you refresh this file too.

This doc explains how to run the test-suite locally: https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/dev_guide/platforms/vmware_guidelines.rst

@mariolenz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@goneri Sorry, I havn't realized that I have broken something. I only tested against govcsim and setup_cluster.yml isn't executed in that case. I hope it's fixed now.

@mariolenz mariolenz force-pushed the mariolenz:issue58023 branch from b37a481 to b156be6 Jul 14, 2019

@mariolenz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@Akasurde @goneri @jillr Do you have any news for me on this PR? I hope I fixed all issues you've mentioned, but if you think otherwise or if there are some more don't hesitate to tell me. I'll do what I can.

@jillr
Copy link
Contributor

left a comment

Code LGTM, @acozine could you take a look at the porting guide here please? We're breaking up a module into several smaller modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.