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

Revert "[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it" #2396

Merged
merged 1 commit into from
Jul 25, 2022

Conversation

prsunny
Copy link
Collaborator

@prsunny prsunny commented Jul 25, 2022

…whether FEC mode is supported before setting it (#2333)"

This reverts commit dc8bc1c.
@prsunny
Copy link
Collaborator Author

prsunny commented Jul 25, 2022

@stephenxs , can you check why/how it got passed in the original PR? Is there any gap in the build process?

@prgeor
Copy link
Contributor

prgeor commented Jul 25, 2022

@prsunny, @stephenxs instead of reverting can we fix the issue as:-

https://github.com/Azure/sonic-swss/blob/master/orchagent/p4orch/tests/fake_portorch.cpp#L521

bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode)
++bool PortsOrch::setPortFec(Port &port, std::string &mode)

@prsunny prsunny merged commit 6565b50 into master Jul 25, 2022
@prsunny prsunny deleted the revert-2333-supported_feds branch July 25, 2022 23:40
stephenxs added a commit to stephenxs/sonic-swss that referenced this pull request Jul 27, 2022
…d check whether FEC mode is supported before setting it (sonic-net#2333)" (sonic-net#2396)"

This reverts commit 6565b50.
@stephenxs
Copy link
Collaborator

stephenxs commented Jul 27, 2022

@stephenxs , can you check why/how it got passed in the original PR? Is there any gap in the build process?

Yes.
This is a racing condition in the azure build process.
The compiling error occurred in orchagent/p4orch/tests/fake_portorch.cpp which was not part of the building at the time #2333 was being built in the azure pipeline last Thursday (Thu at 7:54 AM).
It has been added by PR #2375 which was merged also last Thursday but a bit later (Thu Jul 21 18:29:13 2022 -0700)

There is a way to guarantee such issues will be avoided and detected during building/merging,

  • while merging a PR, rerun the test and block merging if there is an issue

but I believe it will slow down the process of opening/merging PR significantly because re-running the test takes several hours and it’s not always stable.
So this is a trade-off and I tend to treat the current way as the best for now:

  • do not handle the racing condition
  • revert the PR if an issue is detected.

BTW, PR #2400 is opened for taking the code back and resolving the compiling issue.

@prsunny
Copy link
Collaborator Author

prsunny commented Jul 27, 2022

@stephenxs , can you check why/how it got passed in the original PR? Is there any gap in the build process?

Yes. This is a racing condition in the azure build process. The compiling error occurred in orchagent/p4orch/tests/fake_portorch.cpp which was not part of the building at the time #2333 was being built in the azure pipeline last Thursday (Thu at 7:54 AM). It has been added by PR #2375 which was merged also last Thursday but a bit later (Thu Jul 21 18:29:13 2022 -0700)

There is a way to guarantee such issues will be avoided and detected during building/merging,

  • while merging a PR, rerun the test and block merging if there is an issue

but I believe it will slow down the process of opening/merging PR significantly because re-running the test takes several hours and it’s not always stable. So this is a trade-off and I tend to treat the current way as the best for now:

  • do not handle the racing condition
  • revert the PR if an issue is detected.

BTW, PR #2400 is opened for taking the code back and resolving the compiling issue.

Thanks for your analysis @stephenxs . We are working with build team to see how this can be addressed in future.

stephenxs added a commit to stephenxs/sonic-swss that referenced this pull request Aug 4, 2022
…d check whether FEC mode is supported before setting it (sonic-net#2333)" (sonic-net#2396)"

This reverts commit 6565b50.
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
…whether FEC mode is supported before setting it (sonic-net#2333)" (sonic-net#2396)

This reverts commit dc8bc1c.
*Revert "[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it"
prgeor pushed a commit that referenced this pull request Aug 8, 2022
…FEC mode is supported before setting it (#2400)

* Revert "Revert "[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it (#2333)" (#2396)"

This reverts commit 6565b50.

* Adjust the prototype of setPortFec

Signed-off-by: Stephen Sun <stephens@nvidia.com>
yxieca pushed a commit that referenced this pull request Aug 11, 2022
…FEC mode is supported before setting it (#2400)

* Revert "Revert "[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it (#2333)" (#2396)"

This reverts commit 6565b50.

* Adjust the prototype of setPortFec

Signed-off-by: Stephen Sun <stephens@nvidia.com>
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