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

Error Handling Framework: Orchestration changes to report SAI API failures #1100

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

Conversation

sivamukka
Copy link

@sivamukka sivamukka commented Oct 18, 2019

Error handling framework is responsible for notifying ASIC/SAI programming failures to the applications.

Changes in OrchAgent includes:

  • Receives notifications from syncd
  • Process the notifications from syncd and logs the errors into error database
  • Notify the status to registered applications
  • Clear the error database entries based on the CLI command

Dependent PRs:
sonic-net/sonic-swss-common#309

Related PRs:
sonic-net/sonic-utilities#666
sonic-net/sonic-sairedis#523

Signed-off-by: Siva Mukka (siva.mukka@broadcom.com)

@sivamukka sivamukka changed the title Error Handling Framework: Orchestration changes for Error Handling Framework Error Handling Framework: Orchestration changes to report SAI API failures Oct 18, 2019
Copy link
Contributor

@daall daall left a comment

Choose a reason for hiding this comment

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

Review is in the comments. Let me know if you have any questions!

Comment on lines 1 to 16
/*
* Copyright 2019 Broadcom Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to include this?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. It's a new file added as part of error handling framework.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion! I meant the license header, not the file itself. 😝

We don't include a license header in any of the source files in SWSS - that information is available at the top of the repo. You can delete this comment.

Copy link
Author

Choose a reason for hiding this comment

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

This file header is present for all contributions from Broadcom. Please let me know if this is a concern.

orchagent/errororch.cpp Outdated Show resolved Hide resolved
orchagent/errororch.cpp Outdated Show resolved Hide resolved
orchagent/errororch.cpp Outdated Show resolved Hide resolved
orchagent/errororch.cpp Outdated Show resolved Hide resolved
orchagent/errororch.cpp Outdated Show resolved Hide resolved
orchagent/errororch.cpp Outdated Show resolved Hide resolved
std::shared_ptr<Table> table;

/*
* The following steps are performed:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, the comments are helpful but putting the different steps into explicit methods would help with readability.

Copy link
Author

@sivamukka sivamukka Dec 4, 2019

Choose a reason for hiding this comment

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

Done. Added comments at individual sections.

orchagent/errororch.cpp Outdated Show resolved Hide resolved
@@ -581,3 +597,129 @@ bool NeighOrch::removeNeighbor(const NeighborEntry &neighborEntry)

return true;
}

bool NeighOrch::mapToErrorDbFormat(sai_object_type_t& object_type, std::vector<FieldValueTuple> &asicValues,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion but it might be helpful to break this up based on the type of interface for readability.

Copy link
Author

Choose a reason for hiding this comment

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

I would like to leave it as is, as we have only two types of interfaces

- Receives notifications from syncd
- Process the notifications from syncd and logs the errors into error database
- Notify the status to registered applications
@daall daall requested a review from qiluo-msft December 18, 2019 07:21
@prsunny prsunny self-requested a review as a code owner September 2, 2022 23:17
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
…t-boot (sonic-net#1100)

Fast-reboot is utilizing warm-reboot infrastructure to improve its performance, but it should ignore warm-boot logic when syncd starts in fast-boot.
As well it shouldn't use temporary view between init and apply.
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.

2 participants