Skip to content

[lldp] Clean up service start logic owing to port init start optimization#17133

Merged
yxieca merged 1 commit intosonic-net:bookwormfrom
vivekrnv:lldp_timer
Nov 20, 2023
Merged

[lldp] Clean up service start logic owing to port init start optimization#17133
yxieca merged 1 commit intosonic-net:bookwormfrom
vivekrnv:lldp_timer

Conversation

@vivekrnv
Copy link
Contributor

@vivekrnv vivekrnv commented Nov 10, 2023

Why I did it

As part of service start optimization done, we've removed timer units and used PORT_INIT_DONE as the checkpoint for starting delayed services. Thus this logic is redundant

This fixes the following error log.

Nov  3 14:16:54.281546 r-leopard-44 ERR systemctl[10553]: Failed to get unit file state for lldp.timer: No such file or directory

How I did it

Redundant lldp timer code is removed from syncd.sh.

How to verify it

Build and test

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@vivekrnv vivekrnv requested a review from lguohan as a code owner November 10, 2023 01:47
@vivekrnv vivekrnv requested review from dgsudharsan and removed request for lguohan November 10, 2023 01:47
@@ -1 +0,0 @@
per_namespace/lldp.timer.j2 No newline at end of file
Copy link
Contributor

@saiarcot895 saiarcot895 Nov 10, 2023

Choose a reason for hiding this comment

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

Does the actual file need to be deleted as well, not just the symlink?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actual file doesn't exist

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the lldp.service file, not the lldp.timer file. It looks like the timer was removed in #13969.

@yxieca yxieca requested a review from arlakshm November 10, 2023 21:41
Copy link
Contributor

@yxieca yxieca left a comment

Choose a reason for hiding this comment

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

@arlakshm to assess the removal of lldp.timer.j2

@arlakshm
Copy link
Contributor

@arlakshm to assess the removal of lldp.timer.j2

@SuvarnaMeenakshi @abdosi can help check if this change will have impact on chassis bootup?

@vivekrnv
Copy link
Contributor Author

vivekrnv commented Nov 20, 2023

@SuvarnaMeenakshi @abdosi @yxieca Any update on the review?

@SuvarnaMeenakshi
Copy link
Contributor

SuvarnaMeenakshi commented Nov 20, 2023

I don't see this change impacting multi-asic or chassis apart from the fact that lldp will always start after the delay which was not the case earlier (not due to this PR but due to PR 13969)
This PR is more to clean up the change that was missed in the enhancement PR.
The script change here is checking if lldp.timer is enabled, if it is enabled then it would let the timer wait and start service upon warm-boot or fast-boot; it will start immediately if for cold boot.
With the enhancement changes in PR 13969, lldp is a delayed service and will start after the delay for all scenarios and all platforms.
Is that acceptable? @yxieca

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.

6 participants