Add SWSS support for link event damping feature by royyi8 · Pull Request #2933 · sonic-net/sonic-swss

royyi8 · 2023-10-13T22:21:00Z

What I did
Added support for link event damping in SWSS.

Required Syncd PR: sonic-net/sonic-sairedis#1297
CLI PR: sonic-net/sonic-utilities#3001
HLD: https://github.com/sonic-net/SONiC/blob/master/doc/link_event_damping/Link-event-damping-HLD.md

Why I did it

How I verified it
Use the config interface damping CLI to set the port attributes on the switch and observe that Syncd processes link event damping parameters.

Details if related

Junchao-Mellanox · 2023-11-08T04:15:30Z

orchagent/port/portcnt.h

+    struct {
+
+        struct {
+            uint32_t value;


is it possible to use std::optional or boost::optional here?

Hi @Junchao-Mellanox, thanks for your review. There was a refactor change in portsorch.cpp, and my change follows the existing port config. Maybe there can be a cleanup task in the future to update the port config to use std::optional.

Junchao-Mellanox · 2023-11-08T04:19:53Z

orchagent/portsorch.cpp

+    CHECK_ERROR_AND_LOG_AND_RETURN(
+        sai_port_api->set_port_attribute(port.m_port_id, &attr),
+        "Failed to set link event damping algorithm (" << link_event_damping_algorithm << ") for port 0x"
+                                     << std::hex << port.m_port_id);


would it better to print port alias?

I updated the log message to print port alias.

Junchao-Mellanox · 2023-11-08T04:24:06Z

orchagent/portsorch.cpp

+                    p.m_flap_penalty = aied_config.flap_penalty;
+                    m_portList[p.m_alias] = p;
+
+                    SWSS_LOG_NOTICE("Set port %s link event damping config successfully", p.m_alias.c_str());


there is already a log in setPortLinkEventDampingAiedConfig, how about changing that to NOTICE and remove all logs here?

I removed some log messages that were also in setPortLinkEventDampingAiedConfig.

prsunny · 2024-05-08T00:06:16Z

@royyi8 , could you please rebase and fix the build failure?

prsunny · 2024-05-10T15:59:34Z

@royyi8 , still a build failure. Can you check so that we can try to get this before branch cutoff this month

prsunny · 2024-05-13T21:29:12Z

@royyi8 , thank you for actively working on this. Could you also please check the coverage issue and add some coverage?

royyi8 · 2024-05-15T20:20:10Z

@prsunny, I fixed the code coverage issue. By the way, all PRs will need to be merged for the link event damping feature to be complete. The full list of PRs is in the HLD: sonic-net/SONiC#1071.

cc: @mint570

prsunny · 2024-05-15T21:24:31Z

@royyi8 , could you please plan to provide a sonic-mgmt test for this feature as thats the only one missing for this.

@Ashish1805 , @royyi8, @mint570 , could you please add the following to complete the feature?

sonic-mgmt tests
yang-model support for the config_db changes

kcudnik · 2024-05-15T21:26:50Z

orchagent/port/porthlpr.cpp

    { PORT_ROLE_DPC, Port::Role::Dpc }
 };

+static const std::unordered_map<std::string, sai_redis_link_event_damping_algorithm_t> linkEventDampingAlgorithmMap =


should global names be prefixed with g_ ?

Sure, I changed the name to g_linkEventDampingAlgorithmMap.

kcudnik · 2024-05-15T21:27:39Z

orchagent/port/porthlpr.cpp

    return this->getFieldValueStr(port, PORT_ADMIN_STATUS);
 }

+std::string PortHelper::getDampingAlgoStr(const PortConfig &port) const


not sure if Str is reqired in function naming, you already knw what type is returned

I removed Str from the function name, now it is getDampingAlgorithm.

kcudnik · 2024-05-15T21:38:43Z

orchagent/port/porthlpr.cpp

+        else if (field == PORT_DAMPING_ALGO)
+        {
+            if (!this->parsePortLinkEventDampingAlgorithm(port, field, value))
+            {
+                return false;
+            }
+        }
+        else if (field == PORT_MAX_SUPPRESS_TIME)
+        {
+            if (!this->parsePortLinkEventDampingConfig(port.link_event_damping_config.max_suppress_time, field, value))
+            {
+                return false;
+            }
+        }
+        else if (field == PORT_DECAY_HALF_LIFE)
+        {
+            if (!this->parsePortLinkEventDampingConfig(port.link_event_damping_config.decay_half_life, field, value))
+            {
+                return false;
+            }
+        }
+        else if (field == PORT_SUPPRESS_THRESHOLD)
+        {
+            if (!this->parsePortLinkEventDampingConfig(port.link_event_damping_config.suppress_threshold, field, value))
+            {
+                return false;
+            }
+        }
+        else if (field == PORT_REUSE_THRESHOLD)
+        {
+            if (!this->parsePortLinkEventDampingConfig(port.link_event_damping_config.reuse_threshold, field, value))
+            {
+                return false;
+            }
+        }
+        else if (field == PORT_FLAP_PENALTY)
+        {
+            if (!this->parsePortLinkEventDampingConfig(port.link_event_damping_config.flap_penalty, field, value))
+            {
+                return false;


all this and entire function could be refactored to have a stdL::map<string,function> to execute if string is compared and seems all the functions take teh same 3 parameters

Hi @kcudnik, thanks for reviewing the PR. I prefer to hold off refactoring the parsePortConfig function for now, since the first parameter is different between some of the functions. If you have more suggestions, please feel free to let me know.

you have opportunity right now to do refactoring, because you understand surrounding code that you are adding, and leaving this now as is, will result to add more and more code like you are adding that instead of refactoring, take this from my experience.

kcudnik · 2024-05-15T21:39:33Z

orchagent/portsorch.cpp

 }

+ReturnCode PortsOrch::setPortLinkEventDampingAlgorithm(Port &port,
+                                                       sai_redis_link_event_damping_algorithm_t &link_event_damping_algorithm) {


keep { on new line

It's fixed now.

royyi8 · 2024-05-16T21:42:03Z

@royyi8 , could you please plan to provide a sonic-mgmt test for this feature as thats the only one missing for this.

@Ashish1805 , @royyi8, @mint570 , could you please add the following to complete the feature?

sonic-mgmt tests

yang-model support for the config_db changes

@prsunny, could we please continue the discussion in the HLD PR? @Ashish1805 is keeping track of the PRs needed for the link event damping feature.

prsunny · 2024-05-17T17:02:31Z

orchagent/port/porthlpr.cpp

    return true;
 }

+bool PortHelper::parsePortLinkEventDampingAlgorithm(PortConfig &port, const std::string &field, const std::string &value) const


Can you please confirm that this is applied when user dynamically changes the link damping configs and not just during initialization?

Yes, that is correct. Link event damping algorithm and config can be applied at runtime by changing the entries in CONFIG DB.

prsunny · 2024-05-23T17:23:58Z

@royyi8 , can you please resolve conflict and rebase?

prsunny · 2024-05-28T16:47:29Z

@royyi8 , please rebase

What I did Added support for link event damping in SWSS. Required Syncd PR: sonic-net/sonic-sairedis#1297 CLI PR: sonic-net/sonic-utilities#3001 HLD: https://github.com/sonic-net/SONiC/blob/master/doc/link_event_damping/Link-event-damping-HLD.md Why I did it How I verified it Use the config interface damping CLI to set the port attributes on the switch and observe that Syncd processes link event damping parameters.

royyi8 requested a review from prsunny as a code owner October 13, 2023 22:21

Ashish1805 mentioned this pull request Oct 13, 2023

Link event damping HLD sonic-net/SONiC#1071

Merged

Junchao-Mellanox reviewed Nov 8, 2023

View reviewed changes

Junchao-Mellanox approved these changes Nov 9, 2023

View reviewed changes

prsunny requested review from dgsudharsan and prgeor May 7, 2024 23:48

royyi8 force-pushed the link_event_damping_feature branch 2 times, most recently from 22e3b06 to facf1b0 Compare May 9, 2024 17:58

royyi8 force-pushed the link_event_damping_feature branch 7 times, most recently from 274c7ac to e3e16dd Compare May 13, 2024 18:30

royyi8 force-pushed the link_event_damping_feature branch 3 times, most recently from 3f47f16 to 7dd5a28 Compare May 15, 2024 17:23

kcudnik reviewed May 15, 2024

View reviewed changes

kcudnik approved these changes May 15, 2024

View reviewed changes

royyi8 force-pushed the link_event_damping_feature branch 2 times, most recently from 208cf4e to a701460 Compare May 15, 2024 23:45

prsunny reviewed May 17, 2024

View reviewed changes

prgeor previously approved these changes May 20, 2024

View reviewed changes

Add SWSS support for link event damping feature

b881eac

royyi8 dismissed prgeor’s stale review via b881eac June 4, 2024 01:29

royyi8 force-pushed the link_event_damping_feature branch from b60f4a4 to b881eac Compare June 4, 2024 01:29

prsunny merged commit 80f5207 into sonic-net:master Jun 4, 2024

Conversation

royyi8 commented Oct 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented May 8, 2024

Uh oh!

prsunny commented May 10, 2024

Uh oh!

prsunny commented May 13, 2024

Uh oh!

royyi8 commented May 15, 2024

Uh oh!

prsunny commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royyi8 May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royyi8 May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royyi8 commented May 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented May 23, 2024

Uh oh!

prsunny commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

prsunny commented May 15, 2024 •

edited

Loading

royyi8 May 15, 2024 •

edited

Loading

royyi8 May 15, 2024 •

edited

Loading