Subject: [SAM] Review of draft-samrg-sam-baseline-protocol-00

From: Marc Petit-Huguenin <[email-masked]>
Date: Sun, 06 Nov 2011 10:11:58 -0800

Sorry for the delay in reviewing this.  I waited to base the review on an
implementation, but unfortunately I never got the time to do so.  So this is
only a partial review.

1. Title

I do not know if the rules are different for the IRTF, but shouldn't this draft
been called draft-irtf-samrg-sam-baseline-protocol instead?

2. Section 7.2.1 "...peer id closest to and less than the GroupId"

This idea of a Node-ID been close to another ID is specific to certain types of
overlays (like the CHORD-RELOAD default), but will probably do not make sense
for other types.  The text either need to be modified to work with any type of
overlay, or to explicitly say that this draft is only for RELOAD overlays in
which this make sense.

3. Section 7.2.1 structure definition

The Dictionary type is not defined in this spec.

Also I would suggest to follow the standard convention of using lowercase for
the character of a field.

4. Section 7.2.[2-5]

RELOAD message are client/server based.  A request is sent and retransmitted
until a matching response is received.  I am sure how the
Join/JoinAccept/JoinConfirm/JoinDecline mechanism is working inside this framework.

5. Section 10.1. Data Model section

The Data Model is probably SINGLE here.

6. Section 10.1. Access Control section

The Access Control policy NODE-MATCH assumes that the Resource Name is the
Node-ID of signer of the ALMtree entry.  If you want to use the SessionKey as
Resource Name, you probably need to define a new Access Control policy.

Nits
- ----

- - Section 5

s/Register Kind-ID points/Register Kind-ID codepoints/

- - Section 7.3.3

s/diffent/different/

- - Section 7.2.6

s/idenfied/identified/

- - Section 8.2

s/rendevous/rendezvous/

- - Section 8.5

s/conirm/confirm/

- -- 
Marc Petit-Huguenin
Personal email: [email-masked]
Professional email: [email-masked]
Blog: http://blog.marc.petit-huguenin.org

---

From: Dr Mario Kolberg <[email-masked]>
Date: Thu, 17 Nov 2011 16:29:57 +0000

Dear Marc,

many thanks for your review. See below for our responses:

On 06/11/2011 18:11, Marc Petit-Huguenin wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
You are correct. Title will be changed.
> overlay, or to explicitly say that this draft is only for RELOAD overlays in
RELOAD spec states that they support unstructured overlays as well. It 
would be nice to retain that flexibility. We propose to change the text 
to: "The usual interpretation in a DHT based overlay of group_id is that 
the peer with peer id closest to and less than the group_id is the root 
of the tree. However,
other overlay types are supported."
> the character of a field.
Reload defines DictionaryEntry (section 6.2.3). Dictionary is a 'data 
model' (section 6.2).

Will change to lowercase.
> Join/JoinAccept/JoinConfirm/JoinDecline mechanism is working inside this framework.
For Join,  JoinAccept and JoinReject  are matching responses.

If JoinReject is received, then no further action occurs on this request.
If JoinAccept, then JoinConfirm and JoinDecline  are the possible 
matching responses.

With these changes, then Join and JoinAccept will be treated as requests as
in RELOAD which are retransmitted until either retry limit is reached or 
a matching
response received,

> Resource Name, you probably need to define a new Access Control policy.
This sections are currently removed from the draft.
> Nits
all fixed. Thanks!

Regards,
Mario

-- 
The Sunday Times Scottish University of the Year 2009/2010
The University of Stirling is a charity registered in Scotland, 
 number SC 011159.

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Thu, 24 Nov 2011 08:52:30 -0800

Hi,

More comments below.

>> implementation, but unfortunately I never got the time to do so.  So this is

[...]

> Reload defines DictionaryEntry (section 6.2.3). Dictionary is a 'data model'
> (section 6.2).

But Dictionary is never defined as a data structure in RELOAD, and even if it
was defined as an array of DictionaryEntry, that would not match the text
("name-value list of properties...") - the closest structure definition that
matches this text in RELOAD would be IceExtension.

I would suggest to explicitly define Dictionary as follow:

struct {
  opaque name<0..2^16-1>;
  opaque value<0..2^16-1>;
  } DictionaryElement;

struct {
  DictionaryElement elements<0..2^16-1>;
  } Dictionary;

> For Join,  JoinAccept and JoinReject  are matching responses.

But this is not how the RELOAD routing works.  RELOAD says that a response uses
the code value + 1 of the request, excepted for the ErrorResponse.  The problem
here is that a standard implementation of RELOAD will not be able to process
correctly theses messages.  If the goal is to change the routing algorithm, I
would suggest to add a ForwardingOption to each message, with the
FORWARD_CRITICAL flag set, so standard RELOAD implementations do not even try to
route these.

[...]

- -- 
Marc Petit-Huguenin

---

From: "John Buford" <[email-masked]>
Date: Fri, 25 Nov 2011 01:31:26 -0500

Agree with the dictionary definition suggestion.

For the JoinRequest => JoinAccept,JoinReject 3-some,
we could do JoinRequest => JoinResponse with sub-types for the response
Accept,Reject

I wasn't aware that all message routing required an encoded "+1" response.

Thanks for suggestions.

John

-----Original Message-----
From: [email-masked] [mailto:[email-masked]] On Behalf Of Marc
Petit-Huguenin
Sent: Thursday, November 24, 2011 11:52 AM
To: Dr Mario Kolberg
Cc: sam
Subject: Re: [SAM] Review of draft-samrg-sam-baseline-protocol-00

Hi,

More comments below.

[...]

> Reload defines DictionaryEntry (section 6.2.3). Dictionary is a 'data
model'
> (section 6.2).

But Dictionary is never defined as a data structure in RELOAD, and even if
it
was defined as an array of DictionaryEntry, that would not match the text
("name-value list of properties...") - the closest structure definition that
matches this text in RELOAD would be IceExtension.

struct {
  opaque name<0..2^16-1>;
  opaque value<0..2^16-1>;
  } DictionaryElement;

struct {

> For Join,  JoinAccept and JoinReject  are matching responses.

But this is not how the RELOAD routing works.  RELOAD says that a response
uses
the code value + 1 of the request, excepted for the ErrorResponse.  The
problem
here is that a standard implementation of RELOAD will not be able to process
correctly theses messages.  If the goal is to change the routing algorithm,
I
would suggest to add a ForwardingOption to each message, with the
FORWARD_CRITICAL flag set, so standard RELOAD implementations do not even
try to
route these.

[...]

- -- 
Marc Petit-Huguenin
Personal email: [email-masked]
Professional email: [email-masked]
Blog: http://blog.marc.petit-huguenin.org
_______________________________________________
SAM mailing list
[email-masked]
http://www.irtf.org/mailman/listinfo/sam

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Fri, 25 Nov 2011 07:53:51 -0800

> I wasn't aware that all message routing required an encoded "+1" response.

OK, let me backtrack a little bit here.  After checking the spec, I do not think
that you have to use a ForwardingOptions for this as the nodes will be happy to
route any of your messages.  The real problem would be in the interpretation by
an implementation of this text in 5.2.1:

"Because messages may be lost in transit through the overlay, RELOAD
 incorporates an end-to-end reliability mechanism.  When an
 originating node transmits a request it MUST set a timer to the
 current overlay-reliability-timer.  If a response has not been
 received when the timer fires, the request is retransmitted with the
 same transaction identifier.  The request MAY be retransmitted up to
 4 times (for a total of 5 messages).  After the timer for the fifth
 transmission fires, the message SHALL be considered to have failed.
 Note that this retransmission procedure is not followed by
 intermediate nodes.  They follow the hop-by-hop reliability procedure
 described in Section 5.6.3."

The question here is how does this mechanism matches a response with a request.
 My initial interpretation was that a response has 1) the same transaction_id
and 2) either message_code equals to the request message_code + 1 or to the
error value (0xffff).  So I guess that it depends on the interpretation of a
particular implementation.  In all cases, I would suggest to ask the authors of
draft-ietf-p2psip-base for a clarifications on the rules to use to match a
response with a request.  If they say that my interpretation is correct, then
you will have to use the modification that you proposed above.  If they say that
only the transaction_id is needed to match them, then your original proposal
would work.

- -- 
Marc Petit-Huguenin

---

From: "John Buford" <[email-masked]>
Date: Fri, 25 Nov 2011 12:50:12 -0500

Yes this is a good question to bring to the P2PSIP mailing list.  I will
post it there.

Thanks,
John

-----Original Message-----
From: [email-masked] [mailto:[email-masked]] On Behalf Of Marc
Petit-Huguenin
Sent: Friday, November 25, 2011 10:54 AM
To: John Buford
Cc: 'sam'
Subject: Re: [SAM] Review of draft-samrg-sam-baseline-protocol-00

> I wasn't aware that all message routing required an encoded "+1" response.

OK, let me backtrack a little bit here.  After checking the spec, I do not
think
that you have to use a ForwardingOptions for this as the nodes will be happy
to
route any of your messages.  The real problem would be in the interpretation
by
an implementation of this text in 5.2.1:

The question here is how does this mechanism matches a response with a
request.
 My initial interpretation was that a response has 1) the same
transaction_id
and 2) either message_code equals to the request message_code + 1 or to the
error value (0xffff).  So I guess that it depends on the interpretation of a
particular implementation.  In all cases, I would suggest to ask the authors
of
draft-ietf-p2psip-base for a clarifications on the rules to use to match a
response with a request.  If they say that my interpretation is correct,
then
you will have to use the modification that you proposed above.  If they say
that
only the transaction_id is needed to match them, then your original proposal
would work.

>>> implementation, but unfortunately I never got the time to do so.  So
this
> is
>>> Also I would suggest to follow the standard convention of using
lowercase
> for

- -- 
Marc Petit-Huguenin