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

Adds Open vSwitch bond management module #54177

Open
wants to merge 4 commits into
base: devel
from

Conversation

@busterswt
Copy link

commented Mar 21, 2019

SUMMARY

Adds a new Open vSwitch bond management module with support for multiple other_config and set commands necessary for multiple interfaces, including DPDK-enabled interfaces.

Based on the existing openvswitch_port module.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

openvswitch_bond

ADDITIONAL INFORMATION

The existing Open vSwitch modules do not address the configuration and management of bonded interfaces. This module allows users to create and add bonded interfaces to existing bridges by leveraging the ovs-vsctl add-bond command and associated configuration options.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

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

click here for bot help

@gundalow
Copy link
Contributor

left a comment

Hi,
I see from GitHub this is your first PR, congratulations and welcome to the Ansible Community!

I don't know anything about ovs, though I've reviewed the docs and got a few very minor suggestions.

DOCUMENTATION = '''
---
module: openvswitch_bond
version_added: 2.8

This comment has been minimized.

Copy link
@gundalow

gundalow Mar 21, 2019

Contributor
Suggested change
version_added: 2.8
version_added: '2.8'
requirements: [ ovs-vsctl ]
description:
- Manage Open vSwitch bonds and associated options.
options:

This comment has been minimized.

Copy link
@gundalow

gundalow Mar 21, 2019

Contributor

In Ansible 2.8 we are trying to improve the module docs by including type: could you please add that to all the options

This comment has been minimized.

Copy link
@busterswt

busterswt Mar 21, 2019

Author

Sure can, thanks for the suggestions.

- Manage Open vSwitch bonds and associated options.
options:
bridge:
required: true

This comment has been minimized.

Copy link
@gundalow

gundalow Mar 21, 2019

Contributor
Suggested change
required: true
required: true
type: str
'''

EXAMPLES = '''
# Creates an active-backup bond using eth4 and eth5 on bridge br-ex

This comment has been minimized.

Copy link
@gundalow

gundalow Mar 21, 2019

Contributor

We suggest that people always use a - name: when writing Playbooks, doing the same in the EXAMPLES makes it easier for people to copy&paste:

Suggested change
# Creates an active-backup bond using eth4 and eth5 on bridge br-ex
- name: Creates an active-backup bond using eth4 and eth5 on bridge br-ex

EXAMPLES = '''
# Creates an active-backup bond using eth4 and eth5 on bridge br-ex
- openvswitch_bond:

This comment has been minimized.

Copy link
@gundalow

gundalow Mar 21, 2019

Contributor
Suggested change
- openvswitch_bond:
openvswitch_bond:
timeout:
default: 5
description:
- How long to wait for ovs-vswitchd to respond

This comment has been minimized.

Copy link
@gundalow

gundalow Mar 21, 2019

Contributor

Is this seconds?

Suggested change
- How long to wait for ovs-vswitchd to respond
- How long to wait for ovs-vswitchd to respond in seconds.
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/network/ovs/openvswitch_bond.py:0:0: E312 No RETURN provided
lib/ansible/modules/network/ovs/openvswitch_bond.py:0:0: E326 Argument 'lacp' in argument_spec defines choices as (['active', 'passive', 'off']) but documentation defines choices as (['active', 'passive', 'False'])

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/network/ovs/openvswitch_bond.py:0:0: E312 No RETURN provided
lib/ansible/modules/network/ovs/openvswitch_bond.py:0:0: E335 Argument 'bond_downdelay' in argument_spec implies type as 'str' but documentation defines as 'int'
lib/ansible/modules/network/ovs/openvswitch_bond.py:0:0: E335 Argument 'bond_updelay' in argument_spec implies type as 'str' but documentation defines as 'int'

click here for bot help

@ansibot ansibot removed the ci_verified label Mar 21, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/network/ovs/openvswitch_bond.py:0:0: E312 No RETURN provided

click here for bot help

@ansibot ansibot added the ci_verified label Mar 21, 2019

@ansibot ansibot removed the ci_verified label Mar 21, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@markleehamilton

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

@odyssey4me
Copy link
Contributor

left a comment

Looks good to me, although I've not tested it. In an ideal world I'd like to see this have a functional test of all its features and functions, perhaps using all the examples. It'd also be double-plus good to see check mode supported so that users can see what will be changed without enacting the changes.

@kbreit

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Would you be able to paste an output with debug statements so we can see the data structures from the integration tests? I don't have OVS so I can't test it. Overall it looks good but I'd like to see the output.

def main():
""" Entry point. """
argument_spec = {
'bridge': {'required': True},

This comment has been minimized.

Copy link
@kbreit

kbreit Mar 29, 2019

Contributor

My preference is to be explicit about the types for parameters.

}

module = AnsibleModule(argument_spec=argument_spec,
supports_check_mode=True)

This comment has been minimized.

Copy link
@kbreit

kbreit Mar 29, 2019

Contributor

Check mode is preferred (I'm implementing it in my modules now, should have from day 1). If you don't support it, change this to False.

@ansibot ansibot added the stale_ci label Apr 6, 2019

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.