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

SCTP stream mapping, addressing #708 #722

Closed
wants to merge 12 commits into from
Closed

Conversation

mwelzl
Copy link
Contributor

@mwelzl mwelzl commented Dec 23, 2020

Closes #708.
More details on stream mapping. This also fixes references (hopefully), and adds one.

@mwelzl
Copy link
Contributor Author

mwelzl commented Dec 23, 2020

How do I prevent the new build error from happening? Delete my repo and just checkout everything again?

It says:
"# No configuration was found in your project. Please refer to https://circleci.com/docs/2.0/ to get started with your configuration."

I did pull the master, create a new branch, work there and push that. This used to work.

… to reflect the usage of a newly reserved Adaptation Code Point.
@mwelzl mwelzl requested a review from tfpauly December 23, 2020 15:15
Copy link
Contributor

@theri theri left a comment

Choose a reason for hiding this comment

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

I just tried to wrap my head around this and I've got a few questions, see below.

draft-ietf-taps-impl.md Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
mwelzl and others added 3 commits January 27, 2021 15:11
Co-authored-by: Theresa Enghardt <ietf@tenghardt.net>
Co-authored-by: Theresa Enghardt <ietf@tenghardt.net>
Co-authored-by: Theresa Enghardt <ietf@tenghardt.net>
Copy link
Contributor

@martinduke martinduke left a comment

Choose a reason for hiding this comment

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

So this is not what I expected at all. I am rusty on the details of SCTP streams, but why all the special flags for stream mapping? Why not just use the concepts of cloning, connection groups, and entanglement to open new streams?

@mwelzl
Copy link
Contributor Author

mwelzl commented Jan 27, 2021

This is a good conversation to have, because the same questions (below) will need to be asked for QUIC. Maybe the answers are trivial for all of them, I don't know - shouldn't be hard to ckeck.

Cloning, groups, entanglement is just how we present it to the application. To implement this, the following questions need to be addressed:

  1. What happens when an application opens a stream-mapped Connection?
  2. How does scheduling / prioritization work?
  3. What happens when an application closes a stream-mapped Connection?
  4. (optional??) What will happen if the peer isn't aware of stream mapping, but it's just a native QUIC / SCTP / ... server?

LINE ADDED - THIS FORCES NUMBERING TO BEGIN FROM 1 AGAIN

  1. is trivial for both SCTP and QUIC: we just start using a new stream, the semantics allow for that.
  2. should be easy, but in SCTP we need user message interleaving ("I-DATA"), or else we can get heavy HOL blocking delay when sending large messages. That's just an SCTP mechanism that we need so we can turn this on and be efficient; SCTP itself finds out if the peer supports it.
  3. here, SCTP's stream reset extension is semantically very close to "closing", and it's in line with our strict semantics towards the application. We need something... one could also only rely on timeouts perhaps, but then there's the obvious question: when should a Closed event fire? "Never" could be an answer, but that comes with resource wastage, or complex timeout management... when the transport has a signal, surely that's good to use.
  4. I wrote "optional" above because I think we CAN decide that we don't care about this and assume that the application knows exactly what it's doing. Then we need nothing more. However, if we DO want to be able to allow, e.g., a client to connect to server X without specifying a protocol, and that server may be running native SCTP, or QUIC... or it may be running TAPS... then what should happen? In SCTP, the "adaptation layer indication" can let a client find out that the SCTP server really does expect stream mapping - and it's safe to use, because, if it's not supported, the client can just fall back. BUT this complicates matters ... for one, we'd need the application to tell us: "I know I'm talking to a TAPS system" vs. "I know nothing, just try" so that we could ignore this or disable mapping in case we can't negotiate this. We could also always require the negotiation, but that can become a deployment hurdle, I suppose. Perhaps we can discuss item 4 at the interim? I'm fine with removing this, but it's good to at least talk about this once, I think.

@martinduke
Copy link
Contributor

Hi @mwelzl,

What exactly do you mean by "stream mapping"? Are you referring to an application requiring stream IDs to map to specific purposes (e.g. certain messages in Stream 0 in RFC4666)?

My initial position is that applications defined in this way are tightly coupled to their transports and are therefore poor fits for TAPS. In contrast, HTTP/3 has no such mapping and I strongly suspect you could run it over SCTP or even entangled TCP connections.

I am having some trouble understanding the numbering of your questions: some of them seem like answers to other questions?

@mwelzl
Copy link
Contributor Author

mwelzl commented Jan 27, 2021

@martinduke - weird, it shows me 1-4, then 1-4 when editing, even without the line that I just added, but when I finished my comment, it turned into 1-8. Anyway, now the numbering is fixed, sorry!

Regarding "stream mapping", this PR says: 'Mapping Connection objects to SCTP streams is called "stream mapping" and has additional requirements as follows.' - so it's the process of turning an application-level Connection into a stream of an underlying transport. Hopefully this clarifies some misunderstandings?

@martinduke
Copy link
Contributor

@mwelzl -- thanks, that helps a lot.

  1. Agreed, although I find your phrasing a little odd and am not sure that the "stream mapping" term is a useful construct.

  2. We already have connection (stream) and message prioritization. If an SCTP implementation has limitations (no IDATA) that means it has to finish a message before starting another, I'm not sure that raises to the level of the API. If an SCTP implementation can do I-DATA chunks, it SHOULD use them in case high-priority messages/streams come in.

  3. Yes, SCTP should send a stream reset if it supports that extension. Any new connection object might technically reuse the stream, but that can be hidden from the application.

taps-interface says "The Closed Event informs the application that the Remote Endpoint has closed the Connection. There is no guarantee that a remote Close will indeed be signaled." so I think this only fires if the peer says a stream reset.

  1. I don't understand what it means for the peer to be unaware of stream mapping. The TAPS API had better have a way of determining exactly what payload bits should go over the wire, and delivering the same to the application. As I said in my previous message, if the application is tightly coupled to the transport semantics, it should not be run over TAPS.

@mwelzl
Copy link
Contributor Author

mwelzl commented Jan 27, 2021

Hi @martinduke - I think we're converging! Just to try to clarify some more, I'm answering your points with numbers (and I sure hope numbers stay intact now :-) ):

  1. sorry for odd phrasing, I did my best to be as clear as possible; regarding the term, it's what we used to call it in the NEAT project, I think, but we can call this whatever fits better, I don't mind!
  2. Exactly; and I agree, it doesn't raise to the level of the API - was there something in this PR that made you think that this would affect the API? I'm happy to fix that!
  3. Yes, I agree
  4. Yes - I guess the concern is better phrased as being downward compatible to previous versions of the application which are not written against TAPS. This may well be an obscure wish - technically, it's not hard to support in SCTP, and that's why I put it there, but it's getting clear to me that the confusion + complexity disadvantage outweighs the minor technical gain. I don't mind removing this!

@abrunstrom
Copy link
Contributor

@mwelzl I think the answer to question 4 is also related to the default setting of the multistreaming transport property (sorry lost that mail thread). If the default is prefer we can perhaps not assume that the application is aware and then we would have to ensure that the other end supports it. Or do we assume that you only use Connection Groups if you know that the other end support multistreaming?

@abrunstrom
Copy link
Contributor

And to add one more comment, I think the problem is that you are trying to support both cases now, the application knows that the peer supports multistreaming or the application does not know. But i do not see how taps can distinguish the two cases. We have to select one of them as our assumption and possibly adapt the default setting of the multistreaming transport property accordingly i think.

draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
To implement this functionality, SCTP stream reconfiguration {{?RFC6525}} MUST be
supported by both the client and the server side.

To avoid head-of-line blocking, stream mapping SHOULD only be implemented
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I agree with this. A non-8260 SCTP still provides stream multiplexing with some HOLB avoidance properties, even if it's not as good. This is your question (2), which I think we agreed on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confused. Question 2 was "close = stream reset", 3 was "I-DATA to avoid HOLB".
What is it you don't agree with? The "SHOULD only"?

I'm sceptical because I believe that small messages could get enqueued behind HUGE other messages, and there's also no per-stream flow control, at least not in the absence of I-DATA. A native SCTP application may know what it's doing, but a TAPS application would just use Connections and get a blockage surprise. So that doesn't seem good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I-DATA actually was question 2 :) But i agree it seems a bit risky to use multi-streaming without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh yes right, I looked at a response by @martinduke and he had changed the numbering. Tsk :) First github turns the numbering into 1-8 for me, then this... sorry folks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So - what is it we have and don't have agreement about? The text now says MUST for the "stream reset" SCTP extension, as it allows to implement the equivalent of CLOSE (we could be less restrictive, but I fear that it makes things more complex... we have to invent timeouts for this; also, note that this hasn't been tested with an implementation - the only implementation that we have of doing this uses "stream reset"). For I-DATA, it says SHOULD.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid we've QUIC-optimized our model; we have connection close and abort, and connection group abort. For SCTP, we'd want connection group close and abort, and an optional connection abort, IIUC

We could decide to add connection group close; or, we could just use the lack of a connection (stream) close in SCTP to simply signify it in the API by closing all the connections/streams.

Personally, I think we should have graceful close and abort for connection groups and write something in the QUIC mapping about how the former is handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You say "the lack of a connection (stream) close in SCTP": but we do have stream reset, and it's practically the same. This is why we require it with a MUST, and this capability is negotiated with the peer by SCTP itself - so if we require it on one side, we have it. A stream reset is quite like a FIN.

So, I don't see why you'd need a group close for SCTP? It may be a convenient thing to have, in general, sure, but I don't see what this has to do with SCTP in particular - being an API addition, I see this as separate from this PR, in fact.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you're right; it's a clean close. It's the stream abort that we're missing.

So in theory a streaming protocol has four possible ending operations: connection close, stream close, connection abort, stream abort. QUIC has 3 of these; SCTP has 2, 3 with RFC 6525; HTTP/2 has all four. In some protocols these are half-closes or aborts, or full. TAPS currently has stream close, stream abort, and connection abort.

So I'm going to file a separate issue on this, because it's bigger than the SCTP mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About stream abort: not quite, I think. Using a "stream abort" would mean that the TCP-like half-closed behavior wouldn't work - you couldn't send a stream reset and then expect to receive something on a stream. However, that isn't possible anyway, because SCTP streams are only unidirectional. And, there's only a guarantee of a notification by a returning "stream reset" because we define it in this PR.

I agree that this is bigger and should be separate from this PR... but what's your plan? Do we really have a big problem here? In the end, it's a matter of having the right semantics at the API level. This began with just picking the strictest possible semantics: no half-closed connections, calling "close" means that what the app already gave to the transport system for transmission before the call will still be sent, but the app cannot expect to send or receive anything after this call. This way, SCTP (which doesn't allow half-closed associations) is covered, and the behavior is "correct" for TCP as well. These semantics also work for SCTP streams with stream reset as above.

So... altogether, I thought we'll be safe if we just go strict.

Now, quite recently, the wording was changed a bit, because of the need to support TCP half-closed connections (which is implemented via the "Final" Message Property). I felt a bit uneasy about this change, but in the end I couldn't see how things would go wrong with the wording that we now have in the API draft.

My high-level view is that we still have this rather strict thing, and then there is the option to use "Final", with no guarantee of it working (because we don't know if we get a protocol that supports half-closed connections). But there may be a devil in the details on how we worded it... definitely worth another careful look.

of a large message on one stream might block transmissions on other streams
for a long time.

To avoid conflicts between stream ids, the following procedure is recommended:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please help someone who hasn't read the SCTP specs in 15 years: is there really no deconfliction when there is simultaneous open of a new stream ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't really know myself. This is how Felix Weinrank and Michael Tuexen implemented it in NEAT - so I figured that this must be necessary. I can check with them if there's really no other way... but if there were, they would probably have used it. This said, this is also a few years ago now.

@mwelzl
Copy link
Contributor Author

mwelzl commented Jan 28, 2021

@abrunstrom just to answer your question: "Or do we assume that you only use Connection Groups if you know that the other end support multistreaming?" => that's how I thought it should work, yes - and that's also quite limiting, unless we make the distinction more explicit in the API.

Anyway: I do believe we have reached consensus that this mechanism should be taken out. I have no problem doing this! Glad to keep it simple!

@abrunstrom
Copy link
Contributor

@mwelzl ok, good, i agree, based on that assumption the mechanism does not make sense and should be removed.
maybe we should add a sentence to 6.4 on connection groups in the API draft to make it explicit that if an application does not want multistreaming this can be controlled with the multistreaming transport property?

Copy link
Contributor

@theri theri left a comment

Choose a reason for hiding this comment

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

Thanks for the recent changes, that already makes it clearer to me! I still haven't fully understood how stream mapping can still work without the peer application directly accessing SCTP streams, but it's possible I'm missing a larger picture here.

draft-ietf-taps-impl.md Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
@mwelzl
Copy link
Contributor Author

mwelzl commented Feb 17, 2021

I removed the adaptation layer indication code point, as we discussed - and with this, the examples, as the idea is to only use this when both sides know what's going on.

## Unsupported Elements of the Minimal Set

* Notification of Excessive Retransmissions (early warning below abortion threshold):
This is not supported because it is TCP-specific and hardly
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incomplete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh! More importantly, it doesn't belong here - this is an accident :( will fix

@mwelzl
Copy link
Contributor Author

mwelzl commented Mar 1, 2021

Hi everyone - I think I addressed all the comments... BUT NOTE: this PR affects the interface draft, and this was an accident!
It's supposed to be only about impl. I tried to remove the interface draft from the PR, following these steps:
https://gist.github.com/half2me/211d1d6b30a074479d181440af56c31f
and now I believe that it's ok: it shows changes from the old version of the interface draft, at the time of first writing this PR, to the new version in the current master branch... but I'd feel better about it if the file could just be removed from this PR altogether. I'm not sure how to do this...

My sincere apologies :(

-
ins: A. Fischer
name: Andreas Fischer
date: 2021
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this reference is no longer used, and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK, will do this in the new & clean PR that replaces this one.

mwelzl added a commit that referenced this pull request Apr 12, 2021
@mwelzl mwelzl closed this Apr 12, 2021
britram added a commit that referenced this pull request May 4, 2021
SCTP stream mapping, closes #708. (manual re-application of PR #722)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SCTP Connection Objects description is unclear
5 participants