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

nmcli - fixed false changed status for dummy connections #3615

Conversation

Ajpantuso
Copy link
Collaborator

SUMMARY

Dummy connections do not default 802-3-ethernet.mtu to auto if no value is explicitly provided. This is in contrast to other connection types which requires an additional check.

Fixes #3612

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/modules/net_tools/nmcli.py

ADDITIONAL INFORMATION
  • Tested against the ethernet connection type to confirm that null mtu does result in auto being set in that case and this is specific to dummy connections

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module net_tools plugins plugin (any type) labels Oct 26, 2021
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 26, 2021
@haddystuff
Copy link
Contributor

haddystuff commented Oct 26, 2021

I have checked this fix and have noticed one thing:
If i set mtu some value(for example 1500) in playbook:

- name: nmcli set interface
      nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.151/32
        state: present
        type: dummy
        mtu: 1500

Then i deleted mtu value:

- name: nmcli set interface
      nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.151/32
        state: present
        type: dummy

nmcli module doesn't detect any changes.
Mtu remains the same(1500) on host.
Correct me if i'm wrong, it seems like it should be set back to auto or be deleted.

@Ajpantuso
Copy link
Collaborator Author

Correct me if i'm wrong, it seems like it should be set back to auto or be deleted.

In this case because there is no true default value I think it's better for no change to be reported (and certainly no action to be taken as the user didn't indicate a desired state). However this is probably an existing issue with the ethernet and team-slave types as those should default to auto when the option is not provided since that's how nmcli behaves.

@haddystuff
Copy link
Contributor

We would have a problem with different interface configuration depending on system initial state. Seem like it's not an expected behavior. Maybe we can do something like this(#3618)?

@Ajpantuso
Copy link
Collaborator Author

We would have a problem with different interface configuration depending on system initial state. Seem like it's not an expected behavior. Maybe we can do something like this(#3618)?

Something like #3618 is possible, but the setting value should be 0 when modifying and auto when checking. However that would be a breaking change.

If you want to ensure consistency instead of omitting the parameter just provide 0 instead.

Using your example:

- name: nmcli set interface
      nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.151/32
        state: present
        type: dummy
        mtu: 1500

Will become auto for ethernet, dummy, and team-slave with the below:

- name: nmcli set interface
      nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.151/32
        state: present
        type: dummy
        mtu: 0

In effect you are enforcing the default behavior yourself since nmcli is inconsistent.

@felixfontein felixfontein added backport-2 check-before-release PR will be looked at again shortly before release and merged if possible. labels Oct 27, 2021
@haddystuff
Copy link
Contributor

haddystuff commented Oct 27, 2021

Sorry for reacting too slow, but i came out with this change #3625.
The idea is to change "802-3-ethernet.mtu" to "auto" only if this parameter already exist on interface

@Ajpantuso
Copy link
Collaborator Author

Closing in favor of #3625

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug module module net_tools plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nmcli always returning "changed"
4 participants