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

confd: Enable config for lldp tx interval #882

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

axkar
Copy link
Collaborator

@axkar axkar commented Jan 9, 2025

This change introduces the configuration of the tx-interval parameter for the LLDP service, allowing control over the frequency of LLDP hello messages. By reducing the tx-interval, the waiting time for LLDP packets during tests is significantly shortened. This improvement eliminates the need to toggle the interface down and up to force the emission of LLDP packets.

Checklist

Tick relevant boxes, this PR is-a or has-a:

  • Bugfix
    • Regression tests
    • ChangeLog updates (for next release)
  • Feature
    • YANG model change => revision updated?
    • Regression tests added?
    • ChangeLog updates (for next release)
    • Documentation added?
  • Test changes
    • Checked in changed Readme.adoc (make test-spec)
    • Added new test to group Readme.adoc and yaml file
  • Code style update (formatting, renaming)
  • Refactoring (please detail in commit messages)
  • Build related changes
  • Documentation content changes
    • ChangeLog updated (for major changes)
  • Other (please describe):

@axkar axkar requested a review from troglobit January 9, 2025 10:13
@axkar axkar force-pushed the lldp-tx-interval branch from 5996edf to 16c70ba Compare January 9, 2025 10:15
@axkar axkar requested a review from mattiaswal January 9, 2025 10:33
@axkar axkar self-assigned this Jan 9, 2025
src/confd/yang/infix-lldp.yang Outdated Show resolved Hide resolved
src/confd/yang/infix-lldp.yang Outdated Show resolved Hide resolved
@axkar axkar requested review from troglobit and removed request for mattiaswal January 10, 2025 13:02
Copy link
Contributor

@troglobit troglobit left a comment

Choose a reason for hiding this comment

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

Nice work so far!

src/confd/src/infix-services.c Outdated Show resolved Hide resolved
src/confd/src/infix-services.c Outdated Show resolved Hide resolved
src/confd/src/infix-services.c Outdated Show resolved Hide resolved
This change introduces the configuration of the tx-interval parameter
for the LLDP service, allowing control over the frequency of LLDP hello
messages. By reducing the tx-interval, the waiting time for LLDP
packets during tests is significantly shortened. This improvement
eliminates the need to toggle the interface down and up to force the
emission of LLDP packets in the test environment.
@axkar axkar requested a review from troglobit January 15, 2025 13:38
Copy link
Contributor

@troglobit troglobit left a comment

Choose a reason for hiding this comment

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

Only one final comment, nice work!

Comment on lines +339 to +341
if (systemf("initctl -nbq restart lldpd")) {
ERROR("Failed to restart lldpd");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use restart here, we want to "roll in" the next generation at the same time via finit's initctl reload mechanism. The reason for this is simply that changes we add to the /etc/lldpd.d/*.conf may include references to interfaces that have not been created yet. So, instead of restart, use initctl -nbq touch ... construct, used for other services in this file, to mark it for restarting by Finit.

Now, previously we thought lldpd reconfigured itself automatically on SIGHUP, but that does not seem to be the case. To ensure Finit does not send SIGHUP, but instead performs the actual process restart (stop + start) for us, we need to change the .conf snippet in package/skeleton-init-finit/skeleton/etc/finit.d/available/lldpd.conf.

From

service [2345] env:-/etc/default/lldpd lldpd -d $LLDPD_ARGS -- LLDP daemon (IEEE 802.1ab)

to:

service <!> env:-/etc/default/lldpd \
        [2345] lldpd -d $LLDPD_ARGS -- LLDP daemon (IEEE 802.1ab)

The <!> construct tells Finit this service does not support SIGHUP.

To test this, without rebuilding from distclean, you need to perform a make skeleton-init-finit-rebuild.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clear as a tear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants