stratum / fabric-tna

The SD-Fabric data plane
https://docs.sd-fabric.org/
30 stars 15 forks source link

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

Closed ccascone closed 2 years ago

ccascone commented 2 years ago

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:

Future PRs will focus on:

codecov[bot] commented 2 years ago

Codecov Report

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

:exclamation: 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%) :arrow_up:
...ject/fabric/tna/slicing/NetcfgSlicingProvider.java 0.00% <0.00%> (ø)
...stratumproject/fabric/tna/slicing/api/QueueId.java 71.42% <ø> (-6.35%) :arrow_down:
...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%) :arrow_down:
... 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 commented 2 years ago

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:

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 commented 2 years ago

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.