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] Support static configuration of slices via netcfg #462

Merged
merged 5 commits into from
Jan 26, 2022

Conversation

ccascone
Copy link
Member

@ccascone ccascone commented Jan 25, 2022

This PR modifies theSlicingManager to:

  • Remove capabilities for dynamic queue allocation, we assume user-defined static allocation.
  • Store additional TC parameters such as maxRateBps, gminRateBps, etc.
  • Populate stores as a consequence of netcfg events, instead of REST API or CLI calls.

This PR combines changes from the following already-reviewed intermediate PRs:

…ion 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
* 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
* 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
@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #462 (313b2da) into main (4387313) will increase coverage by 1.53%.
The diff coverage is 73.61%.

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

@@             Coverage Diff              @@
##               main     #462      +/-   ##
============================================
+ Coverage     67.74%   69.28%   +1.53%     
- Complexity      645      694      +49     
============================================
  Files            63       59       -4     
  Lines          4545     4616      +71     
  Branches        490      514      +24     
============================================
+ Hits           3079     3198     +119     
+ Misses         1222     1157      -65     
- Partials        244      261      +17     
Impacted Files Coverage Δ
.../java/org/stratumproject/fabric/tna/Constants.java 100.00% <ø> (ø)
.../org/stratumproject/fabric/tna/PipeconfLoader.java 0.00% <0.00%> (ø)
...ratumproject/fabric/tna/behaviour/FabricUtils.java 77.58% <ø> (ø)
...abric/tna/behaviour/pipeliner/FabricPipeliner.java 30.83% <0.00%> (ø)
...aviour/pipeliner/FilteringObjectiveTranslator.java 82.88% <ø> (ø)
...viour/pipeliner/ForwardingObjectiveTranslator.java 74.19% <ø> (ø)
...stratumproject/fabric/tna/slicing/api/SliceId.java 83.33% <ø> (ø)
...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%> (ø)
... and 19 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 4387313...be0544d. Read the comment docs.

Copy link
Collaborator

@daniele-moro daniele-moro left a comment

Choose a reason for hiding this comment

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

PTF are failing on CI, not sure why, since you haven't touched P4 or PTFs.
We need to update at least the Tucson POD netcfg for QoS tests.

@ccascone
Copy link
Member Author

retest this please

@ccascone ccascone merged commit ac45f26 into main Jan 26, 2022
@ccascone ccascone deleted the qos-refactoring branch January 26, 2022 18:11
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