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

[DO-NOT-MERGE] Promote slice configuration to its own netcfg subject #459

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

ccascone
Copy link
Member

@ccascone ccascone commented Jan 23, 2022

This PR is not functional and should not be merged.

We propose an alternative schema to the one introduced in #454 and #457. Instead of being nested inside an app config, slices get configured using their own netcfg event subject. The main benefit is that apps like NetcfgSlicingProvider can listen to events for individual slices, simplifying the diff update logic.

Example netcfg:

{
  "slices": {
    "0": {
      "basic": {
        "name": "Default",
        "tcs": {
          "REAL_TIME": {
            "queueId": 1,
            "isSystemTc": true
          }
        }
      }
    },
    "1": {
      "basic": {
        "name": "P4-UPF",
        "tcs": {
          "CONTROL": {
            "queueId": 2,
            "maxRateBps": "2000000"
          },
          "REAL_TIME": {
            "queueId": 3,
            "maxRateBps": "50000000"
          },
          "ELASTIC": {
            "queueId": 4,
            "gminRateBps": "10000000"
          }
        }
      }
    },
    "2": {
      "basic": {
        "name": "BESS-UPF",
        "tcs": {
          "ELASTIC": {
            "queueId": 5
          }
        }
      }
    }
  }
}

Unfortunately, this approach doesn't work in its current form. For SliceId to be used as a subject class, it needs to be registered in the Kryo serializer of ONOS's DistributedNetworkConfigStore. We cannot make ONOS depend on fabric-tna. The right solution would be to move SliceId and other classes to the ONOS core.

TODO:

  • Consider yet another alternative schema where we can get events for individual TCs (to further simplify updates)
  • Move SlicingService API classes to ONOS core

@ccascone ccascone added the 🍩 merge Do not merge label Jan 23, 2022
@codecov
Copy link

codecov bot commented Jan 23, 2022

Codecov Report

Merging #459 (d36838a) into main (2fccc7d) will increase coverage by 0.17%.
The diff coverage is 50.34%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #459      +/-   ##
============================================
+ Coverage     66.95%   67.13%   +0.17%     
- Complexity      621      632      +11     
============================================
  Files            63       58       -5     
  Lines          4452     4442      -10     
  Branches        472      480       +8     
============================================
+ Hits           2981     2982       +1     
+ Misses         1235     1212      -23     
- Partials        236      248      +12     
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% <ø> (ø)
...ject/fabric/tna/slicing/NetcfgSlicingProvider.java 0.00% <0.00%> (ø)
...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%> (ø)
... and 18 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...d36838a. Read the comment docs.

@pierventre
Copy link
Collaborator

Regarding Slicing API in the ONOS core - don't we have already a review from @Wailoks ?

@charlesmcchan
Copy link
Collaborator

We agreed to put this on hold during the stand up on 1/24. Will revisit this once we need to support dynamic update / move slicing service to ONOS core.

And to @pierventre 's comment - I believe you referred to this one https://gerrit.onosproject.org/c/onos/+/24932, but it has to be updated too since Carmelo introduced some API changes in this series.

@pierventre
Copy link
Collaborator

We agreed to put this on hold during the stand up on 1/24. Will revisit this once we need to support dynamic update / move slicing service to ONOS core.

And to @pierventre 's comment - I believe you referred to this one https://gerrit.onosproject.org/c/onos/+/24932, but it has to be updated too since Carmelo introduced some API changes in this series.

Yup - mine was just a reminder :)

Base automatically changed from qos-refactoring-pr3 to qos-refactoring January 25, 2022 19:36
Base automatically changed from qos-refactoring to main January 26, 2022 18:11
@pudelkoM pudelkoM force-pushed the main branch 2 times, most recently from a6ebb69 to fe32c4a Compare February 2, 2022 21:33
@charlesmcchan charlesmcchan removed their request for review August 5, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍩 merge Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants