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

New module ip_link_device_attribute #66108

Open
wants to merge 5 commits into
base: devel
from
Open

Conversation

@amarao
Copy link

amarao commented Dec 29, 2019

SUMMARY

This module implements subset of features of ip link set command in Linux. It allows to set and query link-level attributes for interfaces: MTU, administrative state, flags (ARP/PROMISC/MULTICAST), aliases and interface groups. It also supports move of interfaces between namespaces (netns attribute). It does so in idempotent way and supports the check mode.

As a side effect it allows to query interface attributes when run without list of attributes to change. It provides more information than a normal setup, moreover, it can gather them in a network namespace.

I plan to write more of those modules for other ip subcommands: ip link create, ip address, ip route.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

ip_link_device_attribute

ADDITIONAL INFORMATION

I found that in laboratory environment I have trouble to create 'integration mocks' - fake namespaces with routing and complicated topology (veth from one namespace into bridge to join it into another namespace). While this is possible with careful 'command'/when sequence, it's really hard to debug and become verbose very quickly. This module removes part of that verbosity.

ip utility (iproute2 package) is extremely widespread on Linux and it's obvious that it should have a module in Ansible. Currently Ansible have only ip_netns module, but I hope to fill this gap.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 29, 2019

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

lib/ansible/modules/net_tools/ip_link_set.py:0:0: module-incorrect-version-added: version_added should be '2.10'. Currently 2.1

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

test/units/modules/net_tools/test_ip_link_set.py:12:0: anomalous-backslash-in-string: Anomalous backslash in string: '\ '. String constant might be missing an r prefix.
test/units/modules/net_tools/test_ip_link_set.py:28:0: anomalous-backslash-in-string: Anomalous backslash in string: '\ '. String constant might be missing an r prefix.
test/units/modules/net_tools/test_ip_link_set.py:42:0: anomalous-backslash-in-string: Anomalous backslash in string: '\ '. String constant might be missing an r prefix.

The test ansible-test sanity --test docs-build [explain] failed with 2 errors:

docs/docsite/rst/modules/list_of_all_modules.rst:1956:0: undefined-label: undefined label: ip_link_set_module (if the link has no caption the label must precede a section header)
docs/docsite/rst/modules/list_of_net_tools_modules.rst:18:0: undefined-label: undefined label: ip_link_set_module (if the link has no caption the label must precede a section header)

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

test/units/modules/net_tools/test_ip_link_set.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

test/units/modules/net_tools/test_ip_link_set.py:0:0: missing: __metaclass__ = type

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

test/units/modules/net_tools/test_ip_link_set.py:12:1: E303: too many blank lines (4)
test/units/modules/net_tools/test_ip_link_set.py:268:9: E126: continuation line over-indented for hanging indent

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 29, 2019

The test ansible-test sanity --test docs-build [explain] failed with 2 errors:

docs/docsite/rst/modules/list_of_all_modules.rst:1956:0: undefined-label: undefined label: ip_link_set_module (if the link has no caption the label must precede a section header)
docs/docsite/rst/modules/list_of_net_tools_modules.rst:18:0: undefined-label: undefined label: ip_link_set_module (if the link has no caption the label must precede a section header)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

test/units/modules/net_tools/test_ip_link_set.py:0:0: missing: __metaclass__ = type

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 29, 2019

The test ansible-test sanity --test docs-build [explain] failed with 2 errors:

docs/docsite/rst/modules/list_of_all_modules.rst:1956:0: undefined-label: undefined label: ip_link_set_module (if the link has no caption the label must precede a section header)
docs/docsite/rst/modules/list_of_net_tools_modules.rst:18:0: undefined-label: undefined label: ip_link_set_module (if the link has no caption the label must precede a section header)

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/ip_link_set/aliases:0:0: missing alias `shippable/posix/group[1-4]` or `unsupported`

click here for bot help

@ansibot ansibot added core_review and removed needs_revision labels Dec 29, 2019
@amarao amarao changed the title New module ip_link_set WIP: New module ip_link_set Dec 31, 2019
@ansibot ansibot added the WIP label Dec 31, 2019
@amarao amarao changed the title WIP: New module ip_link_set WIP: New module ip_link_device_attribute Jan 1, 2020
@amarao amarao changed the title WIP: New module ip_link_device_attribute New module ip_link_device_attribute Jan 1, 2020
@ansibot ansibot removed the WIP label Jan 1, 2020
Copy link
Contributor

tremble left a comment

Some tweaks to the English

@ansibot ansibot added needs_revision and removed core_review labels Jan 7, 2020
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 7, 2020

@amarao This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 commits.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@ansibot ansibot added the needs_rebase label Jan 7, 2020
amarao added 2 commits Dec 22, 2019
This module should cover most of ip link set functions.
@amarao amarao force-pushed the amarao:ip_link_set branch from aee6686 to 7c5d63f Jan 7, 2020
@ansibot ansibot removed the needs_rebase label Jan 7, 2020
@amarao

This comment has been minimized.

Copy link
Author

amarao commented Jan 7, 2020

@tremble, I hope, I solved those issues. Moreover, I squashed commits to reduce history clutter.

Copy link
Contributor

tremble left a comment

A couple more minor tweaks.

As far as the tests are concerned I generally prefer to organize the tests as
check_mode, assert, apply, assert, apply for idempotency, assert

The tests would also be more readable if you added some blank lines between each test. I often have the command and the assert together, then a blank link, and add a comment between each cluster of tests

# Test that setting A makes the elephant stand on its head.
ChangeA - In check_mode
Assert - 'changed' and expected output

ChangeA - Make the change
Assert - 'changed' and expected output

ChangeA - Rerun with same options to test idempotency
Assert - 'not changed' and expected output

# Test that Setting B makes the elephant lift its trunk.
ChangeB - In check_mode
Assert - 'changed' and expected output

ChangeB - Make the change
Assert - 'changed' and expected output

ChangeB - Rerun with same options to test idempotency
Assert - 'not changed' and expected output
...

Doing it this way makes it easier to follow, makes it easier to make sure that you have both check_mode and idempotency tested, and also helps test that check_mode didn't accidentally make a change.

@amarao

This comment has been minimized.

Copy link
Author

amarao commented Jan 8, 2020

A couple more minor tweaks.

As far as the tests are concerned I generally prefer to organize the tests as
check_mode, assert, apply, assert, apply for idempotency, assert

The tests would also be more readable if you added some blank lines between each test. I often have the command and the assert together, then a blank link, and add a comment between each cluster of tests

Test that setting A makes the elephant stand on its head.

ChangeA - In check_mode
Assert - 'changed' and expected output
ChangeA - Make the change
Assert - 'changed' and expected output
ChangeA - Rerun with same options to test idempotency
Assert - 'not changed' and expected output

Test that Setting B makes the elephant lift its trunk.

ChangeB - In check_mode
Assert - 'changed' and expected output
ChangeB - Make the change
Assert - 'changed' and expected output
ChangeB - Rerun with same options to test idempotency
Assert - 'not changed' and expected output
...

Doing it this way makes it easier to follow, makes it easier to make sure that you have both check_mode and idempotency tested, and that helps test that check_mode didn't accidentally make a change.

... Yes, it's a great idea. I reworked all integration tests in this matter, and they become way more easy to read/write/debug.

@samdoran samdoran removed the needs_triage label Jan 9, 2020
@amarao

This comment has been minimized.

Copy link
Author

amarao commented Jan 10, 2020

@tremble, could you check it again? Thanks.

@resmo resmo self-requested a review Jan 13, 2020
@amarao

This comment has been minimized.

Copy link
Author

amarao commented Jan 20, 2020

@resmo, hello.

Can you look into this, please?

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