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

[Banner] Added BannerCfg class to hostcfgd to handle Banner messages #79

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SviatoslavBoichuk
Copy link

@SviatoslavBoichuk SviatoslavBoichuk commented Oct 20, 2023

Why I did it

Added Banner feature related hostcfgd service according to HLD: sonic-net/SONiC#1361

This PR depends on the following PR: sonic-net/sonic-swss-common#826

How I did it

Added BannerCfg service to hostcfgd. It handles Config DB changes and restart "banner-config" systemd service which will update banners.

How to verify it

Manual test

@liat-grozovik liat-grozovik changed the title Added BannerCfg class to hostcfgd to handle Banner messages [Banner] Added BannerCfg class to hostcfgd to handle Banner messages Dec 3, 2023
@liat-grozovik
Copy link
Collaborator

@SviatoslavBoichuk please handle conflict. Please also add description of the code changes and motivation

@SviatoslavBoichuk
Copy link
Author

@SviatoslavBoichuk please handle conflict. Please also add description of the code changes and motivation

Fixed conflicts, updated PR description

key: DB table's key that was triggered change.
data: Read table data.
"""
# Handling state, login/logout and MOTD messages. Data should be a dict
Copy link
Contributor

Choose a reason for hiding this comment

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

state

Where is the code to handle state? If the feature is disabled, we expected that no extra daemon started, minimal CPU wasted in this script.

Copy link
Author

Choose a reason for hiding this comment

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

The "state" is used by "banner-config.service". Hostcfgd responsible only to track DB changes for BANNER table and restart banner service once user modified banner messages or feature state. The banner service (please see PR: sonic-net/sonic-buildimage#16957) will apply new Banners to Linux only if feature state == enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

How to disable the feature? In normal deployment, if people do not want use this feature, would like to disable it completely, minimize teh CPU wasted relating to this feature.

Copy link
Author

Choose a reason for hiding this comment

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

To disable the feature - you can use CLI command: config banner state disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the feature is disabled, we expected that no extra daemon started, minimal CPU wasted in this script. In this case, is the service "banner-config" still running?

Copy link
Author

Choose a reason for hiding this comment

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

If the feature is disabled, we expected that no extra daemon started, minimal CPU wasted in this script. In this case, is the service "banner-config" still running?

The banner-config systemd service - is running in one-shot mode - it will be run at least once on init stage - doesn't matter if feature is disabled. Regarding hostcfgd daemon - banner functionality will run as part of daemon.

Copy link
Contributor

Choose a reason for hiding this comment

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

@qiluo-msft is that thread resolved?

@liat-grozovik
Copy link
Collaborator

/azp run sonic-net.sonic-host-services

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fastiuk
Copy link
Contributor

fastiuk commented Apr 22, 2024

The test is failing because sonic-net/sonic-swss-common#826 should be merged first

@fastiuk fastiuk closed this May 8, 2024
@fastiuk fastiuk reopened this May 8, 2024
Copy link

linux-foundation-easycla bot commented May 8, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@fastiuk
Copy link
Contributor

fastiuk commented May 9, 2024

Still failing because of sonic-net/sonic-swss-common#826

@fastiuk fastiuk closed this May 13, 2024
@fastiuk fastiuk reopened this May 13, 2024
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
@fastiuk
Copy link
Contributor

fastiuk commented May 14, 2024

@qiluo-msft , added a new unit test. Didn't touch a code. Please re-review/approve

@fastiuk
Copy link
Contributor

fastiuk commented May 30, 2024

@qiluo-msft , gentle reminder :)

1 similar comment
@fastiuk
Copy link
Contributor

fastiuk commented Jun 3, 2024

@qiluo-msft , gentle reminder :)

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.

None yet

4 participants