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

SDX-Controller should display vlan field for a connection/l2vpn instead of vlan_range #314

Closed
Tracked by #317
italovalcy opened this issue Aug 12, 2024 · 5 comments · Fixed by #327
Closed
Tracked by #317
Assignees

Comments

@italovalcy
Copy link
Contributor

According to the provisioning API spec, the Endpoint.VLAN should be called "vlan" instead of "vlan_range". For data input, it is already working fine. For displaying it is wrong (vlan_range).

Example:

$ curl -s http://0.0.0.0:8080/SDX-Controller/1.0.0/connection/c8659334-655f-4d6c-9d18-5c0759c62315
{
  "endpoints": [
    {
      "id": "urn:sdx:port:amlight.net:Switch01:16",
      "name": "urn:sdx:port:amlight.net:Switch01:16",
      "vlan_range": 700
    },
    {
      "id": "urn:sdx:port:tenet.ac.za:Switch06:22",
      "name": "urn:sdx:port:tenet.ac.za:Switch06:22",
      "vlan_range": 600
    }
  ],
  "id": "c8659334-655f-4d6c-9d18-5c0759c62315",
  "name": "VLAN between AMPATH/700 and TENET/600"
}

Expected behavior:

$ curl -s http://0.0.0.0:8080/SDX-Controller/1.0.0/connection/c8659334-655f-4d6c-9d18-5c0759c62315
{
  "endpoints": [
    {
      "id": "urn:sdx:port:amlight.net:Switch01:16",
      "name": "urn:sdx:port:amlight.net:Switch01:16",
      "vlan": "700"
    },
    {
      "id": "urn:sdx:port:tenet.ac.za:Switch06:22",
      "name": "urn:sdx:port:tenet.ac.za:Switch06:22",
      "vlan": "600"
    }
  ],
  "id": "c8659334-655f-4d6c-9d18-5c0759c62315",
  "name": "VLAN between AMPATH/700 and TENET/600"
}

In summary, two changes are needed:

  • Rename the field vlan_range to vlan when displaying the L2VPN
  • Convert the vlan field to be "string" according to the new specification
@italovalcy italovalcy assigned italovalcy and unassigned italovalcy Aug 12, 2024
@jab1982
Copy link

jab1982 commented Aug 12, 2024

Fix to support the AtlanticWave-SDX Provisioning specification 1.0.

@sajith
Copy link
Member

sajith commented Sep 11, 2024

I have been looking into this. Basically, in getconnection_by_id(), we are simply returning what's in the database, from connections collection:

value = db_instance.read_from_db("connections", f"{service_id}")
if not value:
return "Connection not found", 404
return json.loads(value[service_id])

What gets inserted in connections though? That seems to be simply the original connection request that place_connection() received.

if code == 200:
db_instance.add_key_value_pair_to_db(
"connections", service_id, json.dumps(body)
)

@italovalcy, could you please share some more details? What was the connection request, and what are the topologies? And what should be really the behavior of GET /connection/:service_id? I am not sure returning the original request itself is very useful. Perhaps the client wants to know breakdown that SDX produced, and status of the connection request? We do not seem to be keeping a record of the failed connection requests -- shouldn't we keep those also in the database?

@sajith
Copy link
Member

sajith commented Sep 11, 2024

Also inviting @congwang09 and @YufengXin to the discussion.

@sajith
Copy link
Member

sajith commented Sep 16, 2024

@italovalcy @YufengXin @congwang09 Please correct me if I am wrong: according to my reading of the service provisioning spec, GET /SDX-Controller/1.0.0/connection/:service_id is now GET /l2vpn/1.0/:service_id.

https://sdx-docs.readthedocs.io/en/latest/specs/provisioning-api-1.0.html#listing-retrieving-one-sdx-l2vpn

So the response should be in accordance with what's in the spec, correct? I am copying the example response from the spec:

{
  "c73da8e1-5d03-4620-a1db-7cdf23e8978c": {
    "service_id": "c73da8e1-5d03-4620-a1db-7cdf23e8978c",
    "name": "VLAN between AMPATH/300 and TENET/150",
    "endpoints": [
      {"port_id": "urn:sdx:port:tenet.ac.za:Tenet03:50", "vlan": "150"},
      {"port_id": "urn:sdx:port:ampath.net:Ampath3:50", "vlan": "300"}
    ],
    "description": "This is an example to demonstrate a L2VPN with optional attributes",
      "qos_metrics": {
        "min_bw": {
          "value": 5,
          "strict": false
      },
      "max_delay": {
        "value": 150,
        "strict": true
      }
    },
    "notifications": [
      {"email": "[email protected]"},
      {"email": "[email protected]"}
    ],
    "ownership": "user1",
    "creation_date": "20240522T00:00:00Z",
    "archived_date": "0",
    "status": "up",
    "state": "enabled",
    "counters_location": "https://my.aw-sdx.net/l2vpn/7cdf23e8978c",
    "last_modified": "0",
    "current_path": ["urn:sdx:link:tenet.ac.za:LinkToAmpath"],
    "oxp_service_ids": {
      "ampath.net": ["c73da8e1"],
      "tenet.ac.za": ["5d034620"]
    }
  }
}

Implementing the whole thing would take some effort, I think. For example:

  • oxp_service_ids would require Propagate OXP response to SDX controller sdx-lc#162 and corresponding changes in sdx-controller.
  • counters_location similarly would require... some other pieces to be implemented?
  • Implementing status and state would probably require some more consideration.
  • I am unsure what current_path is.
  • I am also not sure what ownership would mean, because the concept of a "user" is not well defined in sdx-controller.
  • last_modified would require further changes from Need an API to update existing connection (and check the request spec 2.0) #259.
  • We could probably simply copy qos_metrics from the request, but in order to be useful, those values probably should reflect what's actually allocated.

All those things calls for a meta issue. For the purpose of solving this specific issue, we could make sdx-controller send the minimum viable response:

{
  "c73da8e1-5d03-4620-a1db-7cdf23e8978c": {
    "service_id": "c73da8e1-5d03-4620-a1db-7cdf23e8978c",
    "name": "VLAN between AMPATH/300 and TENET/150",
    "endpoints": [
      {"port_id": "urn:sdx:port:tenet.ac.za:Tenet03:50", "vlan": "150"},
      {"port_id": "urn:sdx:port:ampath.net:Ampath3:50", "vlan": "300"}
    ],
    "description": "This is an example to demonstrate a L2VPN with optional attributes",
}

What do you think?

@YufengXin
Copy link
Collaborator

@sajith For this specific issue, the minimum response you suggested makes sense to me. Please go ahead to close this issue.

Then we can open new issues to address the other issues, of which could be resolved are (1) oxp_service_id populated from xop-LC (2) I think 'current_path' means the list of breakdowns. @congwang09 these two are on your plate.

Other fields, such as counter_locations, owners, soft or hard qos_metrics, need more clarification and are not ready to be implemented yet. @jab1982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

4 participants