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

Handling of configurations blocks with end-* at the end of the block #39673

Merged
merged 5 commits into from May 8, 2018

Conversation

gdpak
Copy link
Contributor

@gdpak gdpak commented May 3, 2018

SUMMARY

Fixes #39351

Few commands like route-policy and prefix-set have end-policy/end-set at the end of the block
e.g.
prefix-set ebpg_filter
192.168.0.0/16 ge 17 le 30
end-set

here current parser logic sets parent for line '192.168.0.0/16 ge 17 le 30' as 'prefix-set ebpg_filter'. It
also marks 'end-set' as new parent with no children.

Now say when we have a diff between candidate and running-config. we try to just play above config block without 'end-set' . This will cause all configs to fail below this line as command prompts will not accept any command until we come-out of this prefix-set context.

Solutions approach:

  1. while parsing config set a new field 'trailing_parent' in config for such config blocks when we find new parent with "end-*'
  2. Included end-* in diff so that it does not get excluded while comparing with running_config

1 approach is clean but much more intrusive in common code of parsing. Right now I have implemented and tested approach 2) we can discuss if there is alternative better approach

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/network/common/config.py

ANSIBLE VERSION

2.6

@gdpak gdpak requested a review from privateip May 3, 2018 12:33
@ansibot
Copy link
Contributor

ansibot commented May 3, 2018

@gdpak this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added bug This issue/PR relates to a bug. merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels May 3, 2018
@ansibot
Copy link
Contributor

ansibot commented May 3, 2018

@ansibot ansibot added networking Network category and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels May 3, 2018
@gundalow gundalow removed the needs_triage Needs a first human triage before being processed. label May 3, 2018
@gundalow
Copy link
Contributor

gundalow commented May 3, 2018

Worth adding unit tests for this?

@@ -290,6 +290,9 @@ def _diff_line(self, other):
for item in self.items:
if item not in other:
updates.append(item)
# special trailing parents need to be included in diff
elif item.line == 'end-policy' or item.line == 'end-set':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this configuration is specific for a particular platform. Is it possible to sanitize the config indentation in the module code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now there is no platform specific instance of NetworkConfig. It can be done to inherit this class at iosxr code and overwrite function 'difference' and 'diff_exact' to handle these special commands. will that work ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pros of above approach: no extra check penalty for other OS, no regression on other
con: code duplication and fixes in common code would need to be ported

@gdpak
Copy link
Contributor Author

gdpak commented May 7, 2018

@gundalow added test cases @ganeshrn moved functionality to os specific fix. Please have a look

@gdpak
Copy link
Contributor Author

gdpak commented May 7, 2018

Integration tests:

ansible-test network-integration --inventory /tmp/inventory_deepak iosxr_config --testcase misplaced_sublevel -vvvv

TASK [iosxr_config : debug] **********************************************************************************************************************
task path: /macos/ansible/test/integration/targets/iosxr_config/tests/cli/misplaced_sublevel.yaml:27
ok: [iosxr02] => {
"msg": "END cli/misplaced_sublevel.yaml on connection=local"
}
META: ran handlers
META: ran handlers

PLAY RECAP ***************************************************************************************************************************************
iosxr02 : ok=21 changed=6 unreachable=0 failed=0

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. support:network This issue/PR relates to code supported by the Ansible Network Team. test This PR relates to tests. and removed support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels May 7, 2018
@ansibot
Copy link
Contributor

ansibot commented May 7, 2018

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

lib/ansible/modules/network/iosxr/iosxr_config.py:192:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/network/iosxr/iosxr_config.py:232:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/network/iosxr/iosxr_config.py:233:17: E225 missing whitespace around operator
lib/ansible/modules/network/iosxr/iosxr_config.py:242:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/network/iosxr/iosxr_config.py:243:17: E225 missing whitespace around operator
lib/ansible/modules/network/iosxr/iosxr_config.py:250:41: E226 missing whitespace around arithmetic operator
lib/ansible/modules/network/iosxr/iosxr_config.py:253:1: E302 expected 2 blank lines, found 1

click here for bot help

@ansibot ansibot added 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. and removed core_review In order to be merged, this PR must follow the core review workflow. labels May 7, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label May 8, 2018
Copy link
Member

@ganeshrn ganeshrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 8, 2018
@gdpak gdpak merged commit ef577b7 into ansible:devel May 8, 2018
@gdpak gdpak deleted the iosxr_route_policy branch May 8, 2018 04:32
gdpak added a commit to gdpak/ansible that referenced this pull request May 8, 2018
…nsible#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues

(cherry picked from commit ef577b7)
gdpak added a commit that referenced this pull request May 8, 2018
…39843)

* Handling of configurations blocks with end-* at the end of the block (#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues

(cherry picked from commit ef577b7)

* changelog entry
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 12, 2018
…nsible#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 14, 2018
…nsible#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 14, 2018
…nsible#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues
tonal pushed a commit to tonal/ansible that referenced this pull request May 15, 2018
…nsible#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 15, 2018
…nsible#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 15, 2018
…nsible#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues
ilicmilan pushed a commit to ilicmilan/ansible that referenced this pull request Nov 7, 2018
…nsible#39673)

* handle end-policy issue

* revert changes in iosxr cliconf

* fix trailing parents not included in difference

* Moving fix to platform specific fix

* pep 8 issues
@ansible ansible locked and limited conversation to collaborators May 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. networking Network category support:network This issue/PR relates to code supported by the Ansible Network Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iosxr_config crash on NCS5508 6.2.3
4 participants