Added ansible support for LLDP TLVs(port-vlan-id, vlan-name, link-aggregation, maximum-frame-size) #406

thenmozhi-gopal · 2024-06-25T15:55:36Z

SUMMARY

Added ansible support for LLDP TLVs(port-vlan-id, vlan-name, link-aggregation, maximum-frame-size)
Related PR:
ansible-network/resource_module_models#264

GitHub Issues

List the GitHub issues impacted by this PR. If no Github issues are affected, please indicate this with "N/A".

GitHub Issue #
N/A

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

Sonic_lldp_interfaces

OUTPUT

ADDITIONAL INFORMATION

Checklist:

I have performed a self-review of my own code to ensure there are no formatting, linting, or security issues
I have verified that new and existing unit tests pass locally with my changes
I have not allowed coverage numbers to degenerate
I have maintained at least 90% code coverage
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
I have maintained backward compatibility or have provided any relevant "breaking_changes" descriptions in a "fragment" file in the "changelogs/fragments" directory of this repository.
I have provided a summary for this PR in valid "fragment" file format in the "changelogs/fragments" directory of this repository branch. Reference : Ansible Change Log Document

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Regression test report:
Initial regression-report.pdf
Regression-report -latest - 12 Dec 2024
regression-2024-12-12-17-26-02.html.pdf

…regation, maximum-frame-size)

kerry-meyer · 2024-07-29T23:34:33Z

In order to proceed with the code review and merging of these changes, please fix the sanity and UT errors flagged for the current change set posted in this PR.

changelogs/fragments/406-lldp-tlv-support.yaml

awhaley-dell

The content of this looks good from my first pass. I have only a minor formatting suggestion for the fragment file and a question about a unit test validation function.

tests/unit/modules/network/sonic/test_sonic_lldp_interfaces.py

kerry-meyer

This change set looks good for the most part, but I am posting several questions and requests for changes.

I would also like to check the UT coverage results after the additional changes are pushed.

plugins/modules/sonic_lldp_interfaces.py

plugins/module_utils/network/sonic/facts/lldp_interfaces/lldp_interfaces.py

plugins/module_utils/network/sonic/config/lldp_interfaces/lldp_interfaces.py

plugins/modules/sonic_lldp_interfaces.py

plugins/module_utils/network/sonic/config/lldp_interfaces/lldp_interfaces.py

kerry-meyer

I'm adding a couple more comments/questions on the unit test files.

kerry-meyer · 2024-12-06T07:02:21Z

tests/unit/modules/network/sonic/fixtures/sonic_lldp_interfaces.yaml

+    - path: data/openconfig-lldp:lldp/interfaces/interface=Ethernet0/config/openconfig-lldp-ext:allowed-vlans
+      method: patch
+      data:
+        openconfig-lldp-ext:allowed-vlans: '10,15-20'


This format specification is incorrect. It looks like the functional code is correct and works, but the format shown here isn't the format in which the code in the "config" file would send this request. If it did, the request would be rejected. The value associated with the allowed_vlans key should be a list containing unquoted int members and quoted ranges delimited by ".." (not "-"). This line should be:

Suggested change

openconfig-lldp-ext:allowed-vlans: '10,15-20'

openconfig-lldp-ext:allowed-vlans:

- 10

- '15..20'

This means that the unit test should be failing here. If it isn't failing on this, the reason for not detecting the failure needs to be fixed.

This looks okay now.

But before marking this "Issue" resolved: Did you determine why it didn't fail before? Was there some problem in the validation? If not, why did it pass?

tests/unit/modules/network/sonic/test_sonic_lldp_interfaces.py

changelogs/fragments/406-lldp-tlv-support.yaml

stalabi1 · 2024-12-06T23:16:38Z

Diff and check mode aren't implemented in for this module. Perhaps you can implement it in this PR. You can refer to the implementation of diff and check mode in the lldp_global module or any of the other several modules where it is implemented.

dellemc.enterprise_sonic/plugins/module_utils/network/sonic/config/lldp_global/lldp_global.py

Line 149 in 1ded095

if self._module.check_mode:

thenmozhi-gopal · 2024-12-12T12:25:11Z

Diff and check mode aren't implemented in for this module. Perhaps you can implement it in this PR. You can refer to the implementation of diff and check mode in the lldp_global module or any of the other several modules where it is implemented.

dellemc.enterprise_sonic/plugins/module_utils/network/sonic/config/lldp_global/lldp_global.py

Line 149 in 1ded095

if self._module.check_mode:

diff and check mode implementation would be done as separate story (https://jira.cec.lab.emc.com/browse/SNC01F-55)

kerry-meyer · 2024-12-13T06:55:59Z

plugins/modules/sonic_lldp_interfaces.py

+              - Multiple Vlans or Vlan ranges can be configured.
+              - Ranges are specified by a start and end Vlan value separated by double dots.
+              - Vlans configured should be in the range 1-4094.
+            type: list


Although this is the only case in our collection where a range would be represented in a playbook by ".." instead of "-" and this range delimiter string is never used in the SONiC CLI, I can see the advantage of using it here and I think it is okay.

I am a little bit concerned that users will be confused by this alternate notation, but it is acceptable if the examples clearly illustrate this. (The examples have not yet been updated to this form.)

Please update the examples to show the new format. (Including the use of a list of vlans and ranges for "merged" and "deleted" states).

kerry-meyer · 2024-12-13T07:01:52Z

plugins/module_utils/network/sonic/config/lldp_interfaces/lldp_interfaces.py

+            try:
+                result.append(int(vlan))
+            except ValueError:
+                result.append(vlan)


Exceptions cause expenditure of many CPU cycles and should not be used instead of a conditional branch structure. Please use something like the following instead:

Suggested change

try:

result.append(int(vlan))

except ValueError:

result.append(vlan)

if ".." in vlan:

result.append(vlan)

else:

result.append(int(vlan))

plugins/modules/sonic_lldp_interfaces.py

kerry-meyer · 2024-12-13T07:19:31Z

plugins/module_utils/network/sonic/config/lldp_interfaces/lldp_interfaces.py

@@ -80,6 +82,20 @@ def get_lldp_interfaces_facts(self):
            return []
        return lldp_interfaces_facts

+    def convert_vlan_list(self, vlan_lst):
+        """
+        Convert a list of VLANs to a new list.


Please clarify what is being converted.

Suggested change

Convert a list of VLANs to a new list.

Convert a list of strings specifying single VLANs and VLAN

ranges to a new list containing integer values for single

vlans and strings for the ranges.

kerry-meyer · 2024-12-13T08:05:18Z

plugins/module_utils/network/sonic/config/lldp_interfaces/lldp_interfaces.py

-                    url = self.lldp_intf_config_path['suppress_tlv_delete'].format(intf_name=name, med_tlv_select="MDI_POWER")
+            if 'vlan_name_tlv' in command and command['vlan_name_tlv'] is not None:
+                if 'allowed_vlans' in command['vlan_name_tlv'] and command['vlan_name_tlv']['allowed_vlans'] is not None:
+                    allowed_vlan = command['vlan_name_tlv']['allowed_vlans']


Although SONiC accepts an allowed_vlan argument specified this way, it silently fails on any ranges included in the string. (It accepts them with no error flagged, but doesn't delete them.)

A bug should be submitted against the SONiC code to fix this, but in the meantime, the Ansible implementation would need to break the deletion ranges into comma-separated lists of integer vlan values to make this work correctly.

Please change this logic accordingly. (It would also be worthwhile to submit a Jira bug against SONiC for this flaw.)

plugins/module_utils/network/sonic/config/lldp_interfaces/lldp_interfaces.py

kerry-meyer

Although most of the previous change requests have been addressed and are now resolved, I am posting some additional change requests (and a question, which I think is answered by comments on the UT "test..." file).

kerry-meyer · 2024-12-13T09:24:10Z

tests/unit/modules/network/sonic/test_sonic_lldp_interfaces.py

@@ -47,6 +47,25 @@ def tearDown(self):
        self.mock_config_edit_config.stop()
        self.mock_get_interface_naming_mode.stop()

+    def validate_config_requests(self):
+        lldp_path = 'data/openconfig-lldp:lldp/interfaces/interface=Ethernet{}/config/openconfig-lldp-ext:suppress-tlv-advertisement'


It looks like no input request will match this string, so this function will never invoke the parent "validate" function. (The '{}" will not match any value that could be in the request.) Also, there are requests that use paths other than "...suppress_tlv_advertisement" that aren't close to matching this. (e.g. "path: data/openconfig-lldp:lldp/interfaces/interface=Ethernet0/config/openconfig-lldp-ext:allowed-vlans and "path: data/openconfig-lldp:lldp/interfaces/interface=Ethernet0/config/openconfig-lldp-ext:vlan-name-tlv-count".) For those, the parent "validate_config_request method is not being invoked anyway because it's only inside the "if interface_number" block. But even the "...suppress-tlv-advertisment..." paths have additional path information that would cause a miscompare.I realize that a previous "Issue" for this instruction was resolved by replacing "1" with "{}", but how will that resolve the problem? A modified check for the sorting criteria is needed here, using a search for just the "suppress_tlv-advertisement" substring, and the parent "validate" function needs to be invoked for all cases.

More work is needed to make this overload function work as intended. for now, it is apparently just bypassing the validation.

I agree with Kerry, using an exact match check with this string will never match a valid path value. The curly braces are not wild cards.
You can try doing the interface check after a substring match if you want that information but I think this validation feature will need another rework.

awhaley-dell · 2024-12-14T00:26:30Z

tests/unit/modules/network/sonic/test_sonic_lldp_interfaces.py

+        lldp_path = 'data/openconfig-lldp:lldp/interfaces/interface=Ethernet{}/config/openconfig-lldp-ext:suppress-tlv-advertisement'
+        interface_number = None
+        for request in self.config_requests_valid:
+            if request['path'] == lldp_path:


Suggested change

if request['path'] == lldp_path:

if "openconfig-lldp-ext:suppress-tlv-advertisement" in request['path']:

I feel you should do a substring check here instead of a full exact match check. That should address Kerry's concern about the varieties of tlv suppression.

Added ansible support for LLDP TLVs(port-vlan-id, vlan-name, link-agg…

fb8261f

…regation, maximum-frame-size)

thenmozhi-gopal requested review from kerry-meyer and stalabi1 as code owners June 25, 2024 15:55

added changelog file

8d6d98d

thenmozhi-gopal mentioned this pull request Jun 25, 2024

Added ansible support for LLDP TLVs(port-vlan-id, vlan-name, link-aggregation, maximum-frame-size) ansible-network/resource_module_models#264

Open

thenmozhi-gopal and others added 7 commits August 1, 2024 17:44

Addressed sanity issues

5967a0f

Update lldp_interfaces.py

7dcab6f

Update lldp_interfaces.py

3139d3f

Update lldp_interfaces.py

9a6583e

Addressed sanity and unit test failures

8922ebe

Merge branch 'main' into lldp_tlv

a0e8c3d

Update lldp_global.py

dcd8618

stalabi1 added the enhancement New feature or request label Sep 30, 2024

stalabi1 added this to the v3.1.0 milestone Oct 3, 2024

awhaley-dell self-requested a review December 5, 2024 22:31

awhaley-dell reviewed Dec 5, 2024

View reviewed changes

changelogs/fragments/406-lldp-tlv-support.yaml Outdated Show resolved Hide resolved

awhaley-dell reviewed Dec 5, 2024

View reviewed changes

tests/unit/modules/network/sonic/test_sonic_lldp_interfaces.py Outdated Show resolved Hide resolved

kerry-meyer requested changes Dec 6, 2024

View reviewed changes

stalabi1 reviewed Dec 6, 2024

View reviewed changes

changelogs/fragments/406-lldp-tlv-support.yaml Outdated Show resolved Hide resolved

Addressed review comments

81d7637

kerry-meyer reviewed Dec 13, 2024

View reviewed changes

kerry-meyer requested changes Dec 13, 2024

View reviewed changes

awhaley-dell reviewed Dec 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ansible support for LLDP TLVs(port-vlan-id, vlan-name, link-aggregation, maximum-frame-size) #406

Added ansible support for LLDP TLVs(port-vlan-id, vlan-name, link-aggregation, maximum-frame-size) #406

thenmozhi-gopal commented Jun 25, 2024 •

edited

Loading

kerry-meyer commented Jul 29, 2024

awhaley-dell left a comment

kerry-meyer left a comment

kerry-meyer left a comment

kerry-meyer Dec 6, 2024

thenmozhi-gopal Dec 12, 2024

kerry-meyer Dec 13, 2024

stalabi1 commented Dec 6, 2024

thenmozhi-gopal commented Dec 12, 2024 •

edited

Loading

kerry-meyer Dec 13, 2024 •

edited

Loading

kerry-meyer Dec 13, 2024

kerry-meyer Dec 13, 2024

kerry-meyer Dec 13, 2024

kerry-meyer left a comment

kerry-meyer Dec 13, 2024

awhaley-dell Dec 14, 2024

awhaley-dell Dec 14, 2024

-        Convert a list of VLANs to a new list.
+        Convert a list of strings specifying single VLANs and VLAN
+        ranges to a new list containing integer values for single
+        vlans and strings for the ranges.

	if request['path'] == lldp_path:
	if "openconfig-lldp-ext:suppress-tlv-advertisement" in request['path']:

Added ansible support for LLDP TLVs(port-vlan-id, vlan-name, link-aggregation, maximum-frame-size) #406

Are you sure you want to change the base?

Added ansible support for LLDP TLVs(port-vlan-id, vlan-name, link-aggregation, maximum-frame-size) #406

Conversation

thenmozhi-gopal commented Jun 25, 2024 • edited Loading

SUMMARY

GitHub Issues

ISSUE TYPE

COMPONENT NAME

OUTPUT

ADDITIONAL INFORMATION

Checklist:

How Has This Been Tested?

kerry-meyer commented Jul 29, 2024

awhaley-dell left a comment

Choose a reason for hiding this comment

kerry-meyer left a comment

Choose a reason for hiding this comment

kerry-meyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stalabi1 commented Dec 6, 2024

thenmozhi-gopal commented Dec 12, 2024 • edited Loading

kerry-meyer Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerry-meyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thenmozhi-gopal commented Jun 25, 2024 •

edited

Loading

thenmozhi-gopal commented Dec 12, 2024 •

edited

Loading

kerry-meyer Dec 13, 2024 •

edited

Loading