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

[SDFAB-844 - Part 1] Refactor SlicingManager to assume static allocation of queues to TCs #453

Merged
merged 25 commits into from
Jan 25, 2022

Conversation

ccascone
Copy link
Member

@ccascone ccascone commented Jan 13, 2022

This is the first PR refactoring SlicingManager to support netcfg-based static QoS config for Aether 2.0.

The PR is not functional. Merging this to the main branch will break SD-Fabric. The plan is to merge this PR and subsequent ones to the qos-refactoring feature branch, giving reviewers a chance to review changes incrementally. The feature branch will be later merged to main once all PRs have been reviewed and approved.

At high level, we modify SlicingManager to assume a static allocation of queues to TCs. Queue allocation will soon be provided via netcfg.

In more details, this PR includes the following changes:

  • Introduce the TrafficClassDescription class as a way to describe the configuration of a specific traffic class instance within a slice, including attributes such as queue ID, maximum rate, and guaranteed minimum rate.
  • Re-organize service interfaces, moving methods to manipulate slice/TCs to a new SlicingProviderService interface.
  • Remove REST APIs and CLI commands for manipulating slices and TCs. This can only be performed by providers, and netcfg will be the primary interface.
  • Remove hardcoded initialization of slices, we will use netcfg for that.
  • Remove queue allocation logic. Dynamic slice provisioning is a long-term plan and how to best support it is still unclear. For now we shouldn't worry about maintaining code that we don't need.
  • Clean ups an simplifications in SlicingManager and related tests.

Future PRs will focus on:

  • Implementation of NetworkConfigSlicingProvider
  • Update UP4 and FabricUpfProgrammable to obtain the mobile slice IDs from the netcfg (instead of hardcoding it)

@ccascone ccascone requested a review from pierventre January 13, 2022 19:03
@ccascone ccascone changed the base branch from main to qos-refactoring January 13, 2022 19:04
@ccascone ccascone changed the title [WIP] QoS refactoring part 1 [WIP] SlicingManager refactoring part 1 Jan 13, 2022
@ccascone ccascone changed the title [WIP] SlicingManager refactoring part 1 [SDFAB-844 - Part 1] Refactor SlicingManager to assume static allocation of queues to TCs Jan 14, 2022
Copy link
Collaborator

@charlesmcchan charlesmcchan left a comment

Choose a reason for hiding this comment

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

I would argue that those CLI commands are still useful when troubleshooting.
I'd suggest keeping them, but change the implementation such that they push an netcfg internally and trigger the same code execution path like a netcfg update. (Essentially a REST alternative that's easier to use. A similar example can be found in sr-blackhole command)

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #453 (a8bd2ca) into qos-refactoring (2fccc7d) will increase coverage by 1.45%.
The diff coverage is 70.49%.

❗ Current head a8bd2ca differs from pull request most recent head 3309d3d. Consider uploading reports for the commit 3309d3d to get more accurate results
Impacted file tree graph

@@                  Coverage Diff                  @@
##             qos-refactoring     #453      +/-   ##
=====================================================
+ Coverage              66.95%   68.41%   +1.45%     
+ Complexity               621      615       -6     
=====================================================
  Files                     63       57       -6     
  Lines                   4452     4287     -165     
  Branches                 472      456      -16     
=====================================================
- Hits                    2981     2933      -48     
+ Misses                  1235     1122     -113     
+ Partials                 236      232       -4     
Impacted Files Coverage Δ
...stratumproject/fabric/tna/behaviour/Constants.java 100.00% <ø> (ø)
...abric/tna/behaviour/upf/FabricUpfProgrammable.java 57.24% <ø> (+3.83%) ⬆️
...ject/fabric/tna/slicing/NetcfgSlicingProvider.java 0.00% <0.00%> (ø)
...stratumproject/fabric/tna/slicing/api/QueueId.java 71.42% <ø> (-6.35%) ⬇️
...project/fabric/tna/slicing/cli/FlowAddCommand.java 0.00% <0.00%> (ø)
...project/fabric/tna/slicing/cli/FlowGetCommand.java 0.00% <0.00%> (ø)
...ject/fabric/tna/slicing/cli/FlowRemoveCommand.java 0.00% <0.00%> (ø)
...atumproject/fabric/tna/web/SlicingWebResource.java 0.00% <0.00%> (ø)
...abric/tna/slicing/api/TrafficClassDescription.java 34.48% <34.48%> (ø)
...oject/fabric/tna/behaviour/FabricCapabilities.java 25.00% <50.00%> (-4.55%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fccc7d...3309d3d. Read the comment docs.

@ccascone
Copy link
Member Author

ccascone commented Jan 21, 2022

I would argue that those CLI commands are still useful when troubleshooting.
I'd suggest keeping them, but change the implementation such that they push an netcfg internally and trigger the same code execution path like a netcfg update. (Essentially a REST alternative that's easier to use. A similar example can be found in sr-blackhole command)

@charlesmcchan I'm a bit reluctant to put in the work to re-establish those CLI commands because I'm lazy 🙂... for the following reasons:

  • Adding/removing individual slices/tc requires a major refactoring of [SDFAB-844 - Part 3] Add NetcfgSlicingProvider #457, where we only allow to add or remove the whole config block for all slices (I'll reply to your comment about diff logic in that PR)
  • We agreed that static configuration is fine for now, we don't have a requirement for dynamically changing it. So as long as we verify that slice provisioning at system startup works well, that should be enough
  • If needed, one can use the onos-netcfg command to manipulate slices. So we still have a way to do troubleshooting if needed.

If you agree I'd leave them out for now. If we find a reason to reinstate them, we can always look at the git history. What do you think?

@charlesmcchan
Copy link
Collaborator

charlesmcchan commented Jan 21, 2022

I would argue that those CLI commands are still useful when troubleshooting.
I'd suggest keeping them, but change the implementation such that they push an netcfg internally and trigger the same code execution path like a netcfg update. (Essentially a REST alternative that's easier to use. A similar example can be found in sr-blackhole command)

@charlesmcchan I'm a bit reluctant to put in the work to re-establish those CLI commands because I'm lazy 🙂... for the following reasons:

  • Adding/removing individual slices/tc requires a major refactoring of [WIP] [SDFAB-844 - Part 3] Add NetcfgSlicingProvider #457, where we only allow to add or remove the whole config block for all slices (I'll reply to your comment about diff logic in that PR)
  • We agreed that static configuration is fine for now, we don't have a requirement for dynamically changing it. So as long as we verify that slice provisioning at system startup works well, that should be enough
  • If needed, one can use the onos-netcfg command to manipulate slices. So we still have a way to do troubleshooting if needed.

If you agree I'd leave them out for now. If we find a reason to reinstate them, we can always look at the git history. What do you think?

I agree that it won't be too useful (compared to updating the entire block with onos-netcfg) if we can't update individual slice. So yes, I am fine deferring this.

@ccascone ccascone merged commit 3f46e67 into qos-refactoring Jan 25, 2022
@ccascone ccascone deleted the qos-refactoring-pr1 branch January 25, 2022 06:08
ccascone added a commit that referenced this pull request Jan 26, 2022
* [SDFAB-844 - Part 1] Refactor SlicingManager to assume static allocation of queues to TCs (#453)

* Add SlicingProviderService interface

* Remove slice/tc initialization from FabricUpfProgrammable

We wil use netcfg for initialization

* Remove slice/tc initialization from FabricUpfProgrammable

We wil use netcfg for initialization

* Remove REST API for adding/removing slice/tcs

* Remove unused CLI commands

* Revisit TrafficClass and remove hardcoded initialization

Sytem is not a traffic class

* Introduce TrafficClassConfig class

* Remove queue allocation logic

* Comments and renaming on SlicingManager

* Fix meter color int value

* Less ambiguous handling of BE

* Fix tests

* Remove leftover QueueStoreValue

* Javadoc

* Wordsmithing

* Improvements to TrafficClassConfig

* Rename config class to description to disambiguate from netcfg

* Use default for mobile slice

* clean up rest API docs

* address review comments

* Fix test and checkstyles

* Move distributed store destroy to preDeactivate hook

* Update default tc javadoc

* Fix tests

* [SDFAB-844 - Part 2] Add SlicingConfig class (#454)

* Add SlicingProviderService interface

* Remove slice/tc initialization from FabricUpfProgrammable

We wil use netcfg for initialization

* Remove slice/tc initialization from FabricUpfProgrammable

We wil use netcfg for initialization

* Remove REST API for adding/removing slice/tcs

* Remove unused CLI commands

* Revisit TrafficClass and remove hardcoded initialization

Sytem is not a traffic class

* Introduce TrafficClassConfig class

* Remove queue allocation logic

* Comments and renaming on SlicingManager

* Fix meter color int value

* Less ambiguous handling of BE

* Fix tests

* Remove leftover QueueStoreValue

* Javadoc

* Wordsmithing

* Improvements to TrafficClassConfig

* Rename config class to description to disambiguate from netcfg

* First stab at config class

* First stab at tests for SlicingTests

* Forgot to check in the test JSON

* Consistently call it tcDescription instead of tcConfig

* Remove ability to configure best effort queue ID

For now, we can safely assume it will always be 0

* Use default for mobile slice

* clean up rest API docs

* address review comments

* Fix test and checkstyles

* Move distributed store destroy to preDeactivate hook

* Update default tc javadoc

* javadoc

* review comments

* tests

* Fix tests

* Restore double quotes in json example

* [SDFAB-844 - Part 3] Add NetcfgSlicingProvider (#457)

* Add SlicingProviderService interface

* Remove slice/tc initialization from FabricUpfProgrammable

We wil use netcfg for initialization

* Remove slice/tc initialization from FabricUpfProgrammable

We wil use netcfg for initialization

* Remove REST API for adding/removing slice/tcs

* Remove unused CLI commands

* Revisit TrafficClass and remove hardcoded initialization

Sytem is not a traffic class

* Introduce TrafficClassConfig class

* Remove queue allocation logic

* Comments and renaming on SlicingManager

* Fix meter color int value

* Less ambiguous handling of BE

* Fix tests

* Remove leftover QueueStoreValue

* Javadoc

* Wordsmithing

* Improvements to TrafficClassConfig

* Rename config class to description to disambiguate from netcfg

* First stab at config class

* First stab at tests for SlicingTests

* Forgot to check in the test JSON

* Consistently call it tcDescription instead of tcConfig

* Move constants to root package

* Centralize app name constants in one place

It was hard to kkep track of the different app names used

* Progress on netcfg slicing provider

* Working config add/remove

* Fix copyright

* Read initial config

* wordsmithing

* Dyanmic config of system tc

* Remove ability to configure best effort queue ID

For now, we can safely assume it will always be 0

* Clean up

* Use default for mobile slice

* clean up rest API docs

* address review comments

* Fix test and checkstyles

* Doubt

* Move distributed store destroy to preDeactivate hook

* Update default tc javadoc

* javadoc

* review comments

* tests

* Fix tests

* Address review comments

* Add initial tests

* Add additional tests

* Address review comments

* Add tests for system tc handling

* Re-add double quotes to javadoc config example

* Address Charles's comment

* Hardcode mobile slice to 0 for now
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.

4 participants