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

Handle connection deletions in PCE #225

Merged
merged 36 commits into from
Apr 19, 2024
Merged

Handle connection deletions in PCE #225

merged 36 commits into from
Apr 19, 2024

Conversation

sajith
Copy link
Member

@sajith sajith commented Feb 7, 2024

Resolves #224, but only somewhat (deletions are propagated to only PCE, not down to local controllers). I hope we can consider this an initial sketch.

These are the changes:

  • POST /connection handler now generates a UUID for connection requests, and returns a response of this form:
{
  "connection_id": "${connection_id}",
  "reason": "Connection published",
  "status": "OK"
}

Note that this response is not consistent with the API definition (and it wasn't consistent before too). Issue filed: #251.

  • Consequently, GET /connection/:connection_id and DELETE /connection/:connection_id accepts strings in UUID format. This doesn't seem to matter for API proper (it still can accept integers), but Swagger UI rejects connection IDs that are not UUIDs. There's an issue (SDX delete connection should accept both integer and connection GUID #234) that @congwang09 filed related to this; if we also want integer connection IDs let us resolve it separately there.

  • Tests has been expanded to exercise connection requests when topologies are fully set up and nothing is set up at all. Ideally we should return 404 when a connection ID is not found, but we do not do that yet. This is something to be addressed in a follow-up, probably when addressing Test GET /connection more #252.

  • Per discussion on Use request connection id when saving to db #258, since clients are expected to supply connection ID, and since requests without an ID will be rejected by connexion, dropped UUID generation.

This is stacked on top of #209 (because we need a handle to TEManager instance), which is in draft state right now. I will update this PR when #209 gets merged.

@sajith sajith self-assigned this Feb 7, 2024
@coveralls
Copy link

coveralls commented Feb 7, 2024

Pull Request Test Coverage Report for Build 8756575986

Details

  • 9 of 12 (75.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 50.946%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdx_controller/controllers/connection_controller.py 9 12 75.0%
Totals Coverage Status
Change from base Build 8743534762: 0.03%
Covered Lines: 708
Relevant Lines: 1420

💛 - Coveralls

Message queue is needed when publishing breakdowns -- we do not mock
this stuff.
@sajith sajith mentioned this pull request Mar 7, 2024
Since we pass on connection_id to DELETE and GET, not request_id
@sajith sajith marked this pull request as ready for review April 11, 2024 20:06
Copy link
Contributor

@congwang09 congwang09 left a comment

Choose a reason for hiding this comment

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

Thanks Sajith!

sdx_controller/swagger/swagger.yaml Show resolved Hide resolved
They should be strings or perhaps UUIDs
Datamodel spec does not specify connection ID format; datamodel schema
simply says it has to be a string; and the test requests so far have
not used UUIDs
When placing a connection, the client MUST provide a connection ID in
the request body.  Requests without an ID will be rejected even before
we get to the controller method, so there's no point in it generating
an ID.
@sajith sajith requested a review from congwang09 April 18, 2024 21:13
@sajith sajith changed the title Handle connection deletions Handle connection deletions in PCE Apr 19, 2024
Seems that connexion simply ignores `format: uuid`, but hopefully this
will act as a documentation hint.
@sajith sajith merged commit b931068 into main Apr 19, 2024
10 checks passed
@sajith sajith deleted the 224.delete-connection branch April 19, 2024 16:19
@sajith sajith linked an issue Apr 24, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Review connection id data type and allowed values on swagger Implement DELETE /connection handler
3 participants