Support fallback ports in replicas for multicast

[matthewtlam]

We wish to support fallback ports for multicast by adding BackupReplicas to Replicas when the primary port in the Replica becomes unavailable.

[matthewtlam]

cc: @smolkaj for visibility

[jafingerhut]

Looks reasonable to me.

[matthewtlam]

@chrispsommers Could you please take a look?

[chrispsommers]

LGTM, but can you provide a brief example of "side-effect?" Also I want to address #507 (comment).

[smolkaj]

Presumably we want to make support for backup replicas optional? @chrispsommers @jafingerhut?

If so, Matthew, can we call that out explicitly and require targets to either support them or return UNIMPLEMENTED?

[matthewtlam]

@jafingerhut @smolkaj @chrispsommers I was wondering if it was worth specifying if we wish to make it a property that no replica port can be used again in any of it's backup replica ports?

[jafingerhut]

@jafingerhut @smolkaj @chrispsommers I was wondering if it was worth specifying if we wish to make it a property that no replica port can be used again in any of it's backup replica ports?

If you consider that an error for a client to try to specify that, and for a server to be required to return an error in that situation, it seems worth specifying the conditions under which the server must return an error, and which error code should be returned.

Another condition that you might want an error for:

(a) among all backup replicas for a single port, the set of (port,instance) values should contain no duplicates
(b) among all replicas and their backups for an entire multicast group, the set of (port,instance) values should contain no duplicates.

(b) is stronger than (a), of course. If you intend (b), I'd write down (b) in the spec.

[smolkaj]

Presumably we want to make support for backup replicas optional? @chrispsommers @jafingerhut?

If so, Matthew, can we call that out explicitly and require targets to either support them or return UNIMPLEMENTED?

@chrispsommers @jafingerhut gentle ping in case you want to chime in on this. My recommendation is to make backup replica support optional, but require servers to communicate if they don't support them by returning an UNIMPLEMENTED error if a client tries to program a backup replica. Thoughts?

[jafingerhut]

Presumably we want to make support for backup replicas optional? @chrispsommers @jafingerhut?
If so, Matthew, can we call that out explicitly and require targets to either support them or return UNIMPLEMENTED?

@chrispsommers @jafingerhut gentle ping in case you want to chime in on this. My recommendation is to make backup replica support optional, but require servers to communicate if they don't support them by returning an UNIMPLEMENTED error if a client tries to program a backup replica. Thoughts?

Making support for this optional on the server side, with an UNIMPLEMENTED return error status if the server does not support it, seems reasonable to me, especially if you expect to encounter devices that do not support the feature. Early on, at least, that seems likely.

[chrispsommers]

Presumably we want to make support for backup replicas optional? @chrispsommers @jafingerhut?
If so, Matthew, can we call that out explicitly and require targets to either support them or return UNIMPLEMENTED?

@chrispsommers @jafingerhut gentle ping in case you want to chime in on this. My recommendation is to make backup replica support optional, but require servers to communicate if they don't support them by returning an UNIMPLEMENTED error if a client tries to program a backup replica. Thoughts?

Agreed, good call!

[jafingerhut]

Presumably we want to make support for backup replicas optional? @chrispsommers @jafingerhut?
If so, Matthew, can we call that out explicitly and require targets to either support them or return UNIMPLEMENTED?

@chrispsommers @jafingerhut gentle ping in case you want to chime in on this. My recommendation is to make backup replica support optional, but require servers to communicate if they don't support them by returning an UNIMPLEMENTED error if a client tries to program a backup replica. Thoughts?

Making support for this optional on the server side, with an UNIMPLEMENTED return error status if the server does not support it, seems reasonable to me, especially if you expect to encounter devices that do not support the feature. Early on, at least, that seems likely.

A question for you: suppose a client tries to use the feature, and a server not only does not implement it, but is using the current version of the P4Runtime protobuf definitions to parse messages. Such a server will not even "see" the backup replicas in the data structures it parses from incoming requests, true? And thus I suspect they would quietly implement everything except the backup replicas functionality, with no error, yes?

As long as a client realizes that, and/or you design some other mechanism for a client to know whether the server supports this feature, seems reasonable to me.

[smolkaj]

Yes, agreed, @jafingerhut.
But I think this is okay and no different from when we add other features in a non-breaking way, no?
In general, a client cannot rely on behavior/a feature added in version x.y.z unless it ensures that the server has version x.y'.z' with y' >= y (assuming semantic versioning.

[smolkaj]

@jafingerhut @smolkaj @chrispsommers I was wondering if it was worth specifying if we wish to make it a property that no replica port can be used again in any of it's backup replica ports?

If you consider that an error for a client to try to specify that, and for a server to be required to return an error in that situation, it seems worth specifying the conditions under which the server must return an error, and which error code should be returned.

Another condition that you might want an error for:

(a) among all backup replicas for a single port, the set of (port,instance) values should contain no duplicates (b) among all replicas and their backups for an entire multicast group, the set of (port,instance) values should contain no duplicates.

(b) is stronger than (a), of course. If you intend (b), I'd write down (b) in the spec.

Since we're already disallowing duplicate primary replicas within a group today, it seems natural to generalize this to backup replicas and require uniqueness across all replicas (whether primary or backup) within a group. IIUC, that's property (b) proposed above.

[jafingerhut]

Since we're already disallowing duplicate primary replicas within a group today, it seems natural to generalize this to backup replicas and require uniqueness across all replicas (whether primary or backup) within a group. IIUC, that's property (b) proposed above.

Yes, your restatement of what I wrote as option (b) looks equivalent to me.

[matthewtlam]

@smolkaj @jafingerhut @chrispsommers I have updated the P4runtime Spec with the ideas we've discussed on support for backup replicas being optional. As well as the properties of replicas and backup replicas and the associated error messages for breaking the properties

[smolkaj]

Final nits.
Approving early.

[matthewtlam]

I've added the changes to the final nits that Steffen requested

[matthewtlam]

@chrispsommers was wondering if you could please take another look and see if everything looks good?