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

IOS cliconf: remove garbage from top configuration #57523

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@hexdump0x0200
Copy link
Contributor

commented Jun 7, 2019

SUMMARY

The "get_config" method should return the device configuration without any additional non-configuration strings, because otherwise this method would be redundant and you could use the "run_commands" method, passing it the "show running-config" command. The change was made to the plugin, and not to the "ios_config" module, because this problem is common and not directly related to the module and it may be useful in other modules and functions. Making a change to the plugin will allow you to stop cleaning these lines in other parts of the code. Comments on the date of the change are not removed from the configuration, since they are valid configuration lines (they are ignored) and their need for each specific situation must be solved by the calling code. These comments may lead to a false comparison of some saved configuration and received from the device (if you compare them directly as strings), but the developer should use the "get_diff" method for comparison or the possibilities of comparing the class "ansible.module_utils.network.common.config.NetworkConfig"

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/plugins/cliconf/ios.py

@ansibot

This comment has been minimized.

@ansible-zuul

This comment has been minimized.

Copy link

commented Jun 7, 2019

Build succeeded (third-party-check pipeline).

@hexdump0x0200

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

integration tests failed on 2 tasks:

test/integration/targets/ios_ntp/tests/cli/ntp_configuration.yaml:48

idempotency check fails because there is an incorrect configuration of NTP_ACL - it does not exist and the configuration string "ntp access-group peer NTP_ACL" is not added to the configuration, but the ios_ntp module still completes successfully. But since the configuration string is not really added to the configuration, the idempotency test fails. Thus, the failure of this test is not associated with the changes made.

test/integration/targets/ios_smoke/tests/cli/misc_tests.yaml:34
the task "run ios commands to test command_timeout" according to the conditions of the test should be completed by timeout, but this does not happen (the task is executed successfully) for reasons unknown to me. Can commands run faster than 1s?

@pabelanger

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

Thanks for looking at test, this are currently know issues. So, good news is your change didn't break anything it seems :)

@ansibot ansibot removed the needs_triage label Jun 8, 2019

@pabelanger

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

We just fixed ios_ntp, so going to retest to confirm. We should only have 1 failure left.

@pabelanger

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

recheck

@ansible-zuul

This comment has been minimized.

Copy link

commented Jun 10, 2019

Build succeeded (third-party-check pipeline).

@hexdump0x0200

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Problem
test/integration/targets/ios_ntp/tests/cli/ntp_configuration.yaml:48
is resolved.

Problem
test/integration/targets/ios_smoke/tests/cli/misc_tests.yaml:34
persists

There is a new problem on python2.7
test/integration/targets/ios_bgp/tests/cli/basic.yaml:355
I can not say anything on this issue. I checked on my equipment. All tests pass.
OS debian 9.9
python 2.7.13
IOS XE 16.03.08

@pabelanger

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

okay, tests are green now. Going to recheck to confirm that is still true.

@pabelanger

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

recheck

@ansible-zuul

This comment has been minimized.

Copy link

commented Jun 11, 2019

Build succeeded (third-party-check pipeline).

@ganeshrn ganeshrn requested a review from justjais Jun 19, 2019

@ansibot ansibot added the stale_ci label Jun 19, 2019

@justjais

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@hexdump0x0200 Thanks for raising the PR, but we have come across this issue before, plz refer the issue #50029.

@hexdump0x0200

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

The proposed workaround solves the problem only for a specific case: saving the configuration by the ios_config module. But the cause of the problem is deeper, and the problem with ios_config is only its manifestation. The proposed change does not affect the ios_config module (as you previously suggested in PR #50184). Instead, the change is made to the cliconf plugin, which performs configuration retrieval and is used not only by the ios_config module, but also in other modules, eliminating the need for additional filtering (developers of other modules can benefit from this). In the current state, this method does almost nothing, but returns the output of show running-config / show startup-config unchanged and thus has less value for developers than it could. Practically, the developer of the module can use not the get_config method, but use the run_commands method, all the same, the result is the same (which some do: ios_config.py:L512). Indirectly, this also leads to the fact that the vendor-specific code for deleting these lines flows into a vendor-neutral one config.py:L38. At the moment, other modules do not break, because they either filter the result themselves, or immediately after receiving the configuration via get_config, transmit its result to the referenced vendor-neutral NetworkConfig, which performs filtering, and it filters only ios-specific lines (there are no lines from other vendors).
I’ll repeat my opinion: the get_config method should return the device configuration without any additional non-configuration strings, because otherwise this method would be redundant and you could use the run_commands method, passing it the show running-config command.

@hexdump0x0200

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

I agree with the ambiguity of this change, especially if we consider that cliconf plugins for other vendors also do not filter the returned result, but I don’t know if anyone except ios needs it. I would like to know the opinion of other developers about this issue, namely, the change in general, the need to filter the results at the lower (cliconf) or middle level (module_utils, but some developers use the cliconf methods directly, bypassing module_utils), and not only in the context correction of the backup result created by ios_config.

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.