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

aci_domain: Manage phys, vmm, l2ext, l3ext and FC domain profiles #34011

Merged
merged 5 commits into from
Jan 11, 2018

Conversation

dagwieers
Copy link
Contributor

SUMMARY

A new ACI module to manage physical, virtual, external bridged, external routed and fibre channel domain profiles.

This module still needs integration tests once it is formally review and accepted.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

aci_domain

ANSIBLE VERSION

v2.5

@dagwieers dagwieers added the aci Cisco ACI community label Dec 19, 2017
@dagwieers dagwieers added this to the 2.5.0 milestone Dec 19, 2017
@ansibot
Copy link
Contributor

ansibot commented Dec 19, 2017

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.5 This issue/PR affects Ansible v2.5 module This issue/PR relates to a module. networking Network category new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Dec 19, 2017
@dagwieers dagwieers force-pushed the aci_domain branch 4 times, most recently from 7a9773e to b5865f0 Compare December 19, 2017 01:19
@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Dec 19, 2017
@dagwieers
Copy link
Contributor Author

cc @rsmeyers @nilloBE Please test and review !

@dagwieers dagwieers changed the title WIP: aci_domain: Manage phys, vmm, l2ext, l3ext and FC domain profiles aci_domain: Manage phys, vmm, l2ext, l3ext and FC domain profiles Dec 19, 2017
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Dec 19, 2017
@ansible ansible deleted a comment from ansibot Dec 19, 2017
@ansibot
Copy link
Contributor

ansibot commented Dec 19, 2017

@brunocalogero @jmcgill298 @schunduri

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 19, 2017
@gundalow
Copy link
Contributor

I don't see anything obviously wrong. If people are happy then will need EXAMPLES, RETURNS and tests.

@dagwieers dagwieers force-pushed the aci_domain branch 2 times, most recently from 1eca204 to 84d18b3 Compare December 21, 2017 03:42
@rsmeyers
Copy link

Not sure if we should provide the option via this way to create a VMM domain, as it opens up a certain complexity. I would only add vmm here when we covered the full use-case.

@dagwieers
Copy link
Contributor Author

dagwieers commented Dec 27, 2017

Can you elaborate about the full use case ?

If we don't do it this way, it means having 5 aci_*_domain modules here, and another 5 for aci_eap_to_*_domain , another 5 for aci_epg_to_*_domain and 5 more aci_*_domain_to_vlan_pool. I prefer adding domain_type and vm_provider and leave it at one module each

@rsmeyers
Copy link

I agree that having it in 1 module is great, but I would currently remove the option to create a vmm domain our of this module, the reason is that when you create a VMM domain, we need to create 10 more objects for which we need to create more ansible modules..., which is currently not the plan, so it won't be feasible anyway to use this module to create a vmm domain...

@dagwieers dagwieers added new_module This PR includes a new module. and removed gce inventory Inventory category module This issue/PR relates to a module. needs_maintainer Ansibot is unable to identify maintainers for this PR. (Check `author` in docs or BOTMETA.yml) needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html openstack virt Virt community (incl. QEMU, KVM, libvirt, ovirt, RHV and Proxmox) support:certified This issue/PR relates to certified code. support:core This issue/PR relates to code supported by the Ansible Engineering Team. vmware VMware community WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Jan 3, 2018
@ansibot ansibot added the bot_broken The bot is misbehaving. NOT for failing CI. A staff member will investigate. label Jan 3, 2018
@dagwieers
Copy link
Contributor Author

Merging, integration tests and examples follow...

@nilloBE
Copy link

nilloBE commented Jan 12, 2018

I do not know if there is enough to test at the moment. For physical and external domains this is fine. For VMM domains we need to be able to specify all required parameters. This is dependent on the domain type (vmware, redhat, microsoft). I believe it would be great to add to the module.

For VMware:

  • domain_name
  • vswitch_type: [vds, avs, ave]
  • access_mode: [ro, rw]
  • vcenter_name
  • vcenter_ip: [ip or FQDN]
  • vds_version: [5.1, 5.5, 6.0, 6.5, default]
  • stats_collection: [disabled, enabled]
  • datacenter_name
  • mgmt_epg
  • credential_name
  • port_channel_mode: [on, lacp-active,lacp-passive,mac-pining,mac-pinning-plus]
  • vswitch_policy: [cdp, lldp, none]

For RHV:

  • domain_name
  • rhvm_name
  • rhvm_ip: [ip or FQDN]
  • datacenter_name
  • mgmt_epg
  • credential_name

For Microsoft:

  • domain_name
  • scvmm_name
  • scvmm_ip: [ip or FQDN]
  • scvmm_cloud_name
  • port_channel_mode: [on, lacp-active,lacp-passive,mac-pining,mac-pinning-plus]

let me know if you need more info or disagree :)

@rsmeyers
Copy link

Due to the amount of modules we had to make, we limited this module and didn’t include VMM.
In a next phase for sure we need to add this.
Let’s docment the limitation of this module currently.

@dagwieers
Copy link
Contributor Author

So, it's not easy to add conditional parameters, another solution may be needed.

PS I would like to see a new open issue rather than comments in a closed PR which is not tracked.

@nilloBE
Copy link

nilloBE commented Jan 15, 2018

Fair enough. I will add a new issue to track this. We can then decide whether we want to keep it as a single module or not.

@dagwieers dagwieers added the cisco Cisco technologies label Feb 23, 2019
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
aci Cisco ACI community affects_2.5 This issue/PR affects Ansible v2.5 bot_broken The bot is misbehaving. NOT for failing CI. A staff member will investigate. cisco Cisco technologies community_review In order to be merged, this PR must follow the community review workflow. networking Network category new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants