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

[orchagent] Do not restore port admin if port admin is configured #3447

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

Conversation

PJHsieh
Copy link

@PJHsieh PJHsieh commented Jan 3, 2025

What I did
Only restore port admin if port admin is not configured.

Why I did it
The pCfg.admin_status might be overridden by prior configurations.

How I verified it

Details if related

Issue:
The pCfg.admin_status might be overridden by prior configurations.

Solution:
Only restore port admin if port admin is not configured.
@PJHsieh PJHsieh requested a review from prsunny as a code owner January 3, 2025 03:13
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny prsunny requested a review from prgeor January 6, 2025 20:05
@prsunny
Copy link
Collaborator

prsunny commented Jan 6, 2025

@PJHsieh , do you've some logs and examples of the issue you are fixing in this PR?

@PJHsieh
Copy link
Author

PJHsieh commented Jan 7, 2025

@PJHsieh , do you've some logs and examples of the issue you are fixing in this PR?

Hi, @prsunny , please check the following flow.

Example:
Initialization
The variable admin_status is initialized with the value of p.m_admin_state_up. Assume p.m_admin_state_up is true.

User set port FEC:
When configuring FEC, the variable p.m_admin_state_up is set to false.

User set port admin:
Additionally, the user configures the port admin to false in the same iteration. In this case, the value of pCfg.admin_status.value will be restored to true.

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.

3 participants