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

add api yaml v1 #32

Merged
merged 6 commits into from
Sep 23, 2024
Merged

add api yaml v1 #32

merged 6 commits into from
Sep 23, 2024

Conversation

chinaunicomyangfan
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • documentation

What this PR does / why we need it:

add api yaml v1

Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

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

Please see my suggestion for compliance with the new API versioning and release process.

code/API_definitions/NetworkSliceBooking.yaml Outdated Show resolved Hide resolved
code/API_definitions/NetworkSliceBooking.yaml Outdated Show resolved Hide resolved
@ShutingQing
Copy link
Collaborator

Thanks Herbert, we'll start the release process.

@hdamker
Copy link
Contributor

hdamker commented Jul 1, 2024

Thanks Herbert, we'll start the release process.

@ShutingQing No rush needed, take first the time to have a work in progress version which is discussed within the team and agreed to be (pre-)released. The important point is that this PR here will create a "wip" version but not a version with a version number.

Note: I have not commented on the content of the PR.

@ShutingQing ShutingQing requested a review from hdamker July 1, 2024 07:23
ServiceArea:
$ref: "#/components/schemas/Area"
SLAProfile:
$ref: "#/components/schemas/SLAProfile"
Copy link
Collaborator

Choose a reason for hiding this comment

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

SLAProfile

I suggest to remove the prefix "SLA".
1: The area and the service time should be part of the SLA as well.
2: An SLA contains may other aspects, which are not handled by the API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

qosProfile

properties:
MaxNumofTerminals:
$ref: "#/components/schemas/NumofTerminals"
DLThroughputPerTerminal:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the relation to the QOS Profiles, as developed by QOD Group?

type: object
properties:
min:
$ref: "#/components/schemas/Float"
Copy link
Collaborator

@tlohmar tlohmar Jul 1, 2024

Choose a reason for hiding this comment

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

I suggest to simplify the yaml and use the type number directly. --> data type is number for format float

maximum: 20


Throughput:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to allow unit prefixes like kilo or mega.

type: object
properties:
StartTime:
$ref: "#/components/schemas/TimeStamp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need a Data type for TimeStamp? It should be a simple string with a format date-time.

EndTime:
$ref: "#/components/schemas/TimeStamp"

Float:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to remove the data-type and use the OpenAPI "number" data-type. See Section on Numbers

maximum: 180

TimeStamp:
type: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to add "format: date-time" (i.e. not just in the description).

code/API_definitions/NetworkSliceBooking.yaml Show resolved Hide resolved
type: array
items:
$ref: "#/components/schemas/Point"
minItems: 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a requirement, that the third point is not on the same line as the first two points.

Copy link
Contributor

Choose a reason for hiding this comment

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

As this area definition is used also in other APIs I suggest to bring the discussion to DeviceLocation and/or Commonalities. As a minimum requirement the polygon must not be self-intersecting.

Copy link
Collaborator

@tlohmar tlohmar Jul 1, 2024

Choose a reason for hiding this comment

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

Aha, yes, would be good to re-use data types (when possible). Likely the best to bring this discussion to Commonalities.

I opened an issue in commonalities (Issue 242)

code/API_definitions/NetworkSliceBooking.yaml Show resolved Hide resolved
hdamker
hdamker previously approved these changes Jul 1, 2024
@hdamker hdamker self-requested a review July 1, 2024 15:36
@hdamker hdamker dismissed their stale review July 1, 2024 15:37

Requested formal changes done, but I haven't reviewed the content, hence can't approve it.

Copy link
Collaborator

@tlohmar tlohmar left a comment

Choose a reason for hiding this comment

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

Some updates, specifically around the Area data type.

description: Number of terminals
format: int32
minimum: 1
maximum: 20
Copy link
Collaborator

@Masa8106 Masa8106 Jul 2, 2024

Choose a reason for hiding this comment

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

Do you need to limit range with the maximum number of terminals which is 20? I would like to suggest to remove "maximum: 20".

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Maximum Num of Terminals" is suggested to keep it. This is an attribute that used to evaluation whether Network can assure the slice to be booked and meet the customers' need.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To keep the parameter "Maximum Num of Terminal" itself is fine with me. I wonder the reason to limit the number 20 (and not 30 or 50)?

@@ -0,0 +1,293 @@
openapi: 3.0.3
info:
title: API for Network slicing Booking
Copy link
Collaborator

Choose a reason for hiding this comment

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

-> "API for Network Slice Booking"

schema:
$ref: "#/components/schemas/SessionId"
responses:
"204":
Copy link
Collaborator

Choose a reason for hiding this comment

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

i believe we need some error code here, so as get session

type: integer
example: 10
description: |
An attribute specifies the required DL packet transmission latency (millisecond)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use a common rate data type here.

Copy link
Collaborator

@ShutingQing ShutingQing left a comment

Choose a reason for hiding this comment

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

I left some comments, please check files changed. Thank you. : )

properties:
MaxNumofTerminals:
$ref: "#/components/schemas/NumofTerminals"
DLThroughputPerTerminal:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Each terminal should have different DL UP throughput.

NumofTerminals:
type: integer
example: 5
description: Number of terminals
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some descriptions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Maximum Num of Terminals" is suggested to keep it. This is an attribute that used to evaluation whether Network can assure the slice to be booked and meet the customers' need.

description: Number of terminals
format: int32
minimum: 1
maximum: 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more thing. Just let me ask you.
Device access management is mentioned in api design #20.
In this PR, the API can specify the number of terminals but cannot specify which terminals will be allowed to access the slice.
Do you have a plan to add more parameter related to device access management or create new device access management API later?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes Masa, based on Thorsten's suggestion, we'd better need to make device access management function independently. Basically there is one slice resource reservation function, and another device access management function.

Copy link

linux-foundation-easycla bot commented Sep 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

$ref: "#/components/responses/Generic503"
get:
tags:
- NSB Sessions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- NSB Sessions
- Network Slice Booking Sessions

Copy link
Contributor

Choose a reason for hiding this comment

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

@ShutingQing you need to commit this suggestion as well, otherwise it doesn't fit to line 17

Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

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

Some change requests and suggestions based on the Commonalities guidelines and templates to help being more compliant from the beginning.

@ShutingQing
Copy link
Collaborator

Thanks so much Herbert for the detailed suggestions attached. We'll look into it.

@ShutingQing
Copy link
Collaborator

Hi all. Based on the last meeting discussion, there were no further comments. Later for Herbert's suggestions, we've modified one version. Currently the version looks good to me. Please other reviewers take a look. If there is no further comments, we give this an approve.

Copy link
Collaborator

@tlohmar tlohmar left a comment

Choose a reason for hiding this comment

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

Would be good to update the PullRequest and implement the comments. It is now a bit difficult to judge, how the various comments will modify the yaml.

NumberOfTerminals:
type: integer
example: 5
description: Number of terminals
Copy link
Collaborator

Choose a reason for hiding this comment

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

The term 'device' is used in other CAMARA APIs and I assume, that a 'terminal" is the same as a 'device'. When there is indeed no difference between 'terminal' and 'device', I suggest to use change to 'MaxNumberOfDevices' (or MaximumNumberOfDevices)..

Copy link
Collaborator

Choose a reason for hiding this comment

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

The term 'device' is used in other CAMARA APIs and I assume, that a 'terminal" is the same as a 'device'. When there is indeed no difference between 'terminal' and 'device', I suggest to use change to 'MaxNumberOfDevices' (or MaximumNumberOfDevices)..

That's okay for me. Fan and me will modify this. For further comments, I'll suggest to initiate another issue and pr to keep updating the yaml.

Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

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

I suppose the YAML within this PR is now a good "work in progress" starting point for the further work within the repository and I recommend to merge it as soon as possible to allow to create issues for the further discussion needed.

@ShutingQing

... For further comments, I'll suggest to initiate another issue and pr to keep updating the yaml.

That can be done only after there is a yaml merged into main. Currently the base of the discussion is only this PR here.

And thanks for accepting my suggestions - I resolved the conversations. There was one overseen which needs to be committed as there is now no "NSB Sessions" tags defined anymore.

One point which you might want to consider already here or open as an issue (as it might need some discussion):

  • The schema which is now named as QoSProfile is something different than the resource definition QoSProfile within the qos-profile API in QualityOnDemand. I would also say that it isn't a QoS Profile (for a single device), but the parameters for the requested slice (see e.g. the parameter MaxNumofTerminals). So something like "SliceProfile" maybe?

@Kai-hw Kai-hw closed this Sep 23, 2024
@Kai-hw Kai-hw reopened this Sep 23, 2024
@Kai-hw Kai-hw merged commit db60af9 into camaraproject:main Sep 23, 2024
1 check passed
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.

7 participants