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

[synchronous-mode] Document for configuration of synchronous mode #672

Merged
merged 4 commits into from
Nov 4, 2020

Conversation

shi-su
Copy link
Contributor

@shi-su shi-su commented Sep 4, 2020

Document for the configuration design of synchronous mode for review.

@shi-su shi-su marked this pull request as ready for review September 4, 2020 22:06
1. Allow users to enable or disable synchronous mode via CLI.
2. Follow the image-specified mode when upgrading an image with cold/warm/fast-reboot if the synchronous mode configuration is not explicitly specified by users in configDB.
3. Use the user-specified mode when an explicit configuration of synchronous mode exists in configDB.
4. Support enabling/disabling the synchronous mode when deploying a minigraph.
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 4, 2020

Choose a reason for hiding this comment

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

deploying a minigraph [](start = 56, length = 21)

parsing a minigraph and generating config_db.json #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

|---------------------------|-----------------------------------|
| enable | Enable synchronous mode. |
| disable | Disable synchronous mode. |
| Field does not exist | Orchagent and syncd use the mode specified by `orchagent.sh` and `syncd-init-common.sh`, respectively. |
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 4, 2020

Choose a reason for hiding this comment

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

Field does not exist [](start = 2, length = 20)

Let's be verbose. If the field does not exist, and user never use the CLI, will it appear in ConfigDB or not at all? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a description for this scenario.

|---------------------------|-----------------------------------|
| enable | Enable synchronous mode. |
| disable | Disable synchronous mode. |
| Field does not exist | Orchagent and syncd use the mode specified by `orchagent.sh` and `syncd-init-common.sh`, respectively. |
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 4, 2020

Choose a reason for hiding this comment

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

specified by orchagent.sh and syncd-init-common.sh [](start = 63, length = 54)

The design doc have some details not fixed:

  1. if Field does not exist, what is the behavior? Will new image change the behavior?
  2. orchagent.sh and syncd-init-common.sh will use the same mode, so better to define the behavior in one place. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added discussion in section 3.4 to clarify that new images can change the behavior if the field does not exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for 2, both orchagent.sh and syncd-init-common.sh follow the configuration in config_DB. The reason to make the field in config_DB empty as default and let the two files to choose their own mode with the field empty is to make sure the image can change the behavior accordingly with cold/warm/fast reboot if a user-specified mode does not exist. It is kind of like trading off this for better controllability in image upgrade. Since the mode defined by the two files are expected to change only once from async mode to sync mode, I think doing such a tradeoff should be ok.

### 3.1 Configuration Requirements
1. Allow users to enable or disable synchronous mode via CLI.
2. Follow the image-specified mode when upgrading an image with cold/warm/fast-reboot if the synchronous mode configuration is not explicitly specified by users in configDB.
3. Use the user-specified mode when an explicit configuration of synchronous mode exists in configDB.
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 4, 2020

Choose a reason for hiding this comment

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

You need to provide design detail on how to achieve this. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added design details about where the configuration in configDB is read and applied.

1. Allow users to enable or disable synchronous mode via CLI.
2. Follow the image-specified mode when upgrading an image with cold/warm/fast-reboot if the synchronous mode configuration is not explicitly specified by users in configDB.
3. Use the user-specified mode when an explicit configuration of synchronous mode exists in configDB.
4. Support enabling/disabling the synchronous mode when deploying a minigraph.
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 4, 2020

Choose a reason for hiding this comment

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

. [](start = 77, length = 1)

Do you need design for https://github.com/Azure/SONiC/wiki/L2-Switch-mode? #Closed

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 think this L2 switch mode does not need specific designs for it. I added Section 3.6 to discuss about the L2 switch mode.

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comment

@qiluo-msft qiluo-msft merged commit fc8a240 into sonic-net:master Nov 4, 2020
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