p4lang / p4runtime

Specification documents for the P4Runtime control-plane API
Apache License 2.0
142 stars 88 forks source link

Using strings for role #344

Closed konne88 closed 3 years ago

konne88 commented 3 years ago

Could we add string based roles to P4Runtime. The idea is that a string is much more readable (e.g. "tunnel manager") than to use an id (e.g. 1).

message WriteRequest {
  uint64 device_id = 1;
  uint64 role_id = 2;          // deprecated
  string role_name = 6;     // new
  Uint128 election_id = 3;

// ...

message SetForwardingPipelineConfigRequest {
  // ...
  uint64 device_id = 1;
  uint64 role_id = 2;          // deprecated
  string role_name = 6;     // new
  Uint128 election_id = 3;
  Action action = 4;
  ForwardingPipelineConfig config = 5;
}
konne88 commented 3 years ago

@antoninbas @stefanheule What do you think about this idea? If this seems reasonable, I'm happy to send out the PR to change the spec.

stefanheule commented 3 years ago

Seems very reasonable, I'm a big fan of making things easier to debug/understand. If someone still prefers an integer as the role, they can still do that by encoding it as a byte-string (or even a regular string), so this is not limiting any prior usecases either.

jafingerhut commented 3 years ago

It appears that performance concerns are less of an issue for the role, since it is only once per WriteRequest as probably the most frequent message it appears in.

This might tread into "number of angels that can dance on the head of a pin" kinds of discussion, but does this mean that a P4Runtime server must continue to support role_id until and unless it upgrades to a version of P4Runtime API that explicitly breaks backwards compatibility and drops role_id completely?

konne88 commented 3 years ago

Let's discuss this in tomorrow's meeting. In general, the way we've handled this is by returning UNIMPLEMENTED for any feature that we don't support (of which there are many).