Hi,
A patch went in a while back that broke my client code which sends the
gre_tunnel_dump API and processes the replies. Here's the patch -
https://github.com/FDio/vpp/commit/74cf96576768f6e9200848c613428990049b3563.
This was discovered a couple of months ago when we updated our build from
stable/2506 to stable/2510. At the time, we just applied a patch locally to
make things work because we did not have time to properly fix it. I'm
trying to update again from stable/2510 to stable/2602 so I thought I
should probably fix it upstream for real this time.
Here are some details on the issues:
The patch adds a global 'option status = "in_progress";'. I have seen that
option on individual API messages but I have no idea whether this has any
meaning when declared globally. Is this a valid usage or is it just being
silently ignored by vppapigen?
It also adds this service definition:
service {
rpc gre_tunnel_dump returns gre_tunnel_dump_reply events
gre_tunnel_details;
rpc gre_tunnel_dump_v2 returns gre_tunnel_dump_v2_reply events
gre_tunnel_details_v2;
};
I don't think "events" is correct there. I think if the API handlers for
gre_tunnel_dump and gre_tunnel_dump_v2 used a cursor and the macros to send
details messages in batches, it might be appropriate to use "stream" there
instead of event. As it is, those API handlers don't do that cursor magic,
they just iterate all GRE tunnels and send details messages without sending
the reply message (i.e. typical dump/details behavior), so really this
service definition and the dump_reply/dump_reply_v2 message types might be
entirely useless.
I don't think vppapigen does the right thing with the names
gre_tunnel_dump_v2 and gre_tunnel_details_v2. The script expects
_dump/_details at the end of a dump/details API name. The new APIs should
be called gre_tunnel_v2_dump/gre_tunnel_v2_details. Random non sequitur -
there is one other dump/details message that likely has this problem
- policer_dump_v2.
So... I'm trying to figure out how to make things work correctly again.
Ideally we could revert the patch and tell the author "try again". But it
was merged > 6 months ago and it adds new functionality (support for GRE
key), which I assume someone somewhere might care about and would not like
to disappear. So just reverting it is probably not the nice thing to do at
this point. So the second best option here IMO would be:
- Remove the global option status in_progress. Even if it's a valid
configuration, I don't think the GRE APIs, which were formerly considered
production, should be considered "in progress".
- Put gre_tunnel_dump back the way it was before (remove it from that
service rpc definition, remove or deprecate the unused
gre_tunnel_dump_reply)
- Either rename the new APIs or add new ones with the correct names and
mark the bad ones deprecated so they can be removed later.
Does this seem like a sound plan? Is any of this going to trip up the crc
checker script? Is there some reason why the stuff I wrote above is wrong
and this patch is good and I should leave it alone?
Thanks,
-Matt
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#26936): https://lists.fd.io/g/vpp-dev/message/26936
Mute This Topic: https://lists.fd.io/mt/118730935/21656
Group Owner: [email protected]
Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/14379924/21656/631435203/xyzzy
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
