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

Remove update of mgmt oper status in swss #3439

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

Conversation

SuvarnaMeenakshi
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi commented Dec 20, 2024

What I did
Issue to be fix: Currently operational status of mgmt interface is not present or correct for multi-asic devices.

Why I did it
Initial PR that added mgmt oper status feature in swss: #630
sonic-net/sonic-buildimage#21245 adds a script to update oper status of management interface periodically. In doing so, we no longer need to update oper status of mgmt interface in swss.

How I verified it
Verified on single-asic platform

Details if related

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@gechiang gechiang left a comment

Choose a reason for hiding this comment

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

LGTM.

@prsunny
Copy link
Collaborator

prsunny commented Jan 2, 2025

@SuvarnaMeenakshi , could you link the original PR that added this feature? Is there any other dependency that we've to address?

@SuvarnaMeenakshi
Copy link
Contributor Author

@SuvarnaMeenakshi , could you link the original PR that added this feature? Is there any other dependency that we've to address?

hi @prsunny this is the PR that added this feature: #630

@@ -50,43 +49,6 @@ LinkSync::LinkSync(DBConnector *appl_db, DBConnector *state_db) :
{
string key = idx_p->if_name;

/* Explicitly store management ports oper status into the state database.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to keep the for loop as this was also added as part of #630

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we don't need this, removed to match initial PR changes

@@ -197,17 +159,6 @@ void LinkSync::onMsg(int nlmsg_type, struct nl_object *obj)
nlmsg_type, key.c_str(), admin, oper, addrStr, ifindex, master);
}

if (!key.compare(0, MGMT_PREFIX.length(), MGMT_PREFIX))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should remove the check at line 134 as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this : key.compare(0, MGMT_PREFIX.length(), MGMT_PREFIX)

@arlakshm
Copy link
Contributor

arlakshm commented Jan 8, 2025

@SuvarnaMeenakshi, can you please address the comments?

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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

Successfully merging this pull request may close these issues.

5 participants