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

If available, read port settings from Config DB first #723

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

andriymoroz-mlnx
Copy link
Contributor

Signed-off-by: Andriy Moroz [email protected]

What I did
Implemented #2 from this comment to the issue

Why I did it
Fixes issue with the custom port split reset after SONiC-to-SONiC update

How I verified it
Executed SONiC-to-SONiC update on one of setups where we use custom port split. Verified split survived.

Details if related

@prsunny
Copy link
Collaborator

prsunny commented Dec 6, 2018

retest this please

if (!port_config_file.empty())
{
handlePortConfigFile(p, port_config_file, warm);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we "throw" if port_config_file.empty() is true in this else branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to keep the old logic as it was before

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, if not in this PR, we probably need add some logic here later in case both places do not have the port config.

Copy link
Contributor

@nikos-github nikos-github left a comment

Choose a reason for hiding this comment

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

Please test the cases where the ports have an alias and then they don't and vice versa for s2s upgrade in both scenarios where we read port configuration from config_db and port_config.ini.

@andriymoroz-mlnx
Copy link
Contributor Author

@nikos-github
since the initial configuration always comes from port_config.ini, ports will always have alias (ini file parser uses port name if alias not specified)
the only scenario which will be different after this change is when you intentionally remove aliases from the config_db.json. In that case after s2s upgrade ports config will remain without aliases (previously aliases were always recreated from ini file).

@nikos-github
Copy link
Contributor

@andriymoroz-mlnx Thank you. I understand how it works. I'm just requesting explicit testing of the various scenarios including the ones where the alias is explicitly specified but then it's not and is derived from the port name and vice versa.

@andriymoroz-mlnx
Copy link
Contributor Author

andriymoroz-mlnx commented Jan 15, 2019

@nikos-github
First time (e.g. install from ONIE) alias will be used from port_config.ini (or generated if not specified). It means DB will always have it.
Next boot will use DB (no matter s2s or reboot)
To achieve the case "then it's not" after s2s you need to use port_config.ini without aliases, remove config DB and regenerate it. In this case data from port_config.ini will be used which leads to the "first time case"
If this is not what you meant, could you please write steps?

@nikos-github
Copy link
Contributor

@andriymoroz-mlnx If I have a config_db.json file with an alias, where will the code look for the alias? Similarly if I have a config_db.json without an alias, will the code go looking in port_config.ini for one? If the answer to this is yes, then that's wrong.

@andriymoroz-mlnx
Copy link
Contributor Author

@nikos-github
code always uses the DB.
port_config.ini is used only once on start and only when there is no ports info in the DB

@yxieca yxieca merged commit 7375015 into sonic-net:master Jan 23, 2019
zhenggen-xu pushed a commit to zhenggen-xu/sonic-swss that referenced this pull request Jun 28, 2019
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
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.

6 participants