Skip to content

Conversation

@bsun-sudo
Copy link
Contributor

@bsun-sudo bsun-sudo commented Feb 3, 2025

SUMMARY

DCBX_INTERFACES module implementation

GitHub Issues

N/A

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

sonic_dcbx_interfaces

related PR

ansible-network/resource_module_models#296

OUTPUT

Regression Test HTML report:

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

unit test
regression test
regression-2025-05-01-01-39-49.pdf

playbook

@haemanthisree
Copy link
Contributor

I have pushed the changes to merge the DCBx global and interfaces modules. Will close out the global DCBx PRs.
Regression run results :-
dcbx_regression_ansible.pdf

@stalabi1 stalabi1 changed the title add ansible for interface DCBx configuration Add DCBx resource module Feb 17, 2025
Copy link
Collaborator

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

I am not finished reviewing the files in this PR, but I am posting the comments that I currently have entered. This will allow time to incorporate the comments I have made so far and, especially, to add support for "replaced" and "overridden" states while I am reviewing the remaining parts of the "config" file and the "test" files.

@kerry-meyer
Copy link
Collaborator

Please post the regression test report for the DCBx resource module under "Output' (after "Regression Test HTML report:) in the top section of this PR.

Copy link
Collaborator

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

I have finished checking the current version of the "config" file for this PR and am posting my comments for the additional sections I have reviewed since the initial posting of comments.

@bsun-sudo
Copy link
Contributor Author

I am not finished reviewing the files in this PR, but I am posting the comments that I currently have entered. This will allow time to incorporate the comments I have made so far and, especially, to add support for "replaced" and "overridden" states while I am reviewing the remaining parts of the "config" file and the "test" files.

The "replace" and "overridden" were not added for DCBx because, during previous discussions, we were under the impressions that these were not mandatory. Now that they are mandatory, we will add them.

Following is the implementation details based on recent discussion.

For DCBX:

DCBx only applies on physical interfaces,
Default means not CONFIG_DB entry for that attribute
even when global or interface-level admin is not configured, the GET will always include these in the response with respective default values,
So “have” will always have everything, and “want” can be empty or subset of “have”. DCBx uses DELETE to revert global or interface attributes to default configuration

For replace:

If “want” doesn’t include global DCBx, then no change to the global DCBx
If “want” includes global DCBx and it defers fromh “have”, then PATCH if “want” has non-default configuration and DELETE if “want” has default configuration
If an interface is not in “want”, no change to that interface
If an interface is in the “want”, and it matches the configuration in “have” for that interface, do nothing for the interface
If an interface is in the “want”, and it has different configuration than the ones in the “have” for that interface,
If all configuration for that interface in “want” has default configuration, DELETE that interface, else
If the interface in “want” has any non-default configuration and is different than the “have” configuration, PATCH that configuration
If the interface in “want” has any default configuration, and is different than the “have” configuration, DELETE that configuration

For overridden:

If global in “want”, and the same as in “have”, do nothing for global
If global in “want” is non-default and it defers from “have” , PATCH global
If global in “want” is default and it defers from “have”, DELETE global
If global not in ”want”, and “have” doesn’t have default value for global, DELETE the global
If global not in “want”, and “have” has default value for global, do nothing for the global
For any interface not in “want”, and “have” has any non-default configuration for it, DELETE that interface
For any interface not in “want”, and “have” has all default configurations for it, do nothing for that interface
For any interface in “want”, and “have” has same configuration as “want”, do nothing for this interface
For any interface in “want”, and “have” doesn’t have same configuration,
If all configuration for that interface in “want” has default value, DELETE that interface, else
If the interface in “want” has same configuration in the “have” for an attribute, do nothing for that configuration
If the interface in “want” has any non-default configuration and is different than the “have” configuration, PATCH that configuration
If the interface in “want” has any default configuration and is different than the “have” configuration, DELETE that configuration

Bing Sun added 2 commits April 14, 2025 21:50
Copy link
Collaborator

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

I am not done checking all the changes from the previous review, but I'm done with checking the "module" file. I have "resolved" most (but not all) of the previous comments for this file and am posting my current additional comments for the file.

I will follow up soon with issue resolution and verification of code review changes made to the other files for this PR.

Please also take a look at and fix the sanity and ansible-lint errors for this PR and provide fixes for these.

One other request: Please resolve the current merge conflicts. This should only require a simple one-line change to two shared files.

@bsun-sudo
Copy link
Contributor Author

I am not done checking all the changes from the previous review, but I'm done with checking the "module" file. I have "resolved" most (but not all) of the previous comments for this file and am posting my current additional comments for the file.

I will follow up soon with issue resolution and verification of code review changes made to the other files for this PR.

Please also take a look at and fix the sanity and ansible-lint errors for this PR and provide fixes for these.

One other request: Please resolve the current merge conflicts. This should only require a simple one-line change to two shared files.

  1. merge conflicts resolved
  2. updated with sanity fix. Sanity passed on my dri vm. Could you please start the sanity here?

Copy link
Collaborator

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

  • Most of the previous comments are "Resolved" now, but a few are not. I have posted replies or new comments for these.
  • A few comments are posted for new content.
  • The posted regression results are not readable. Please re-post them. Test cases have not yet been reviewed. They will be checked when passing regression results are posted.
  • There are ansible-lint and sanity failures that need to be addressed.
  • Code coverage for the dcbx config file is currently at 56%. Additional UT cases are needed to bring the coverage as close to 90+% as possible. (80% is acceptable.)

Copy link
Collaborator

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

I have checked the regression test file. The set of test cases looks good. I am requesting only one minor change to enhance the "replaced" state test coverage by deleting a few lines from the second "replaced" test case.

(All files have now been checked for this PR review, so after resolution of the current issues, the PR review will be complete.)

Copy link
Collaborator

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

All current proposed code changes and corresponding test results look good.

Thank you very much for all your work to provide the DCBx resource module.

Approved.

@kerry-meyer kerry-meyer merged commit 8f01b1d into ansible-collections:main May 1, 2025
19 checks passed
@stalabi1 stalabi1 added this to the v3.1.0 milestone Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new_resource_module This pull request adds a new resource module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants