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

WIP : Extended ack support #759

Closed
wants to merge 2 commits into from
Closed

Conversation

ffourcot
Copy link
Collaborator

Hello Peter,

During some debug with kernel netlink messages, it appears to us that extended ack support could be a great feature in pyroute2, allowing to read more useful error messages.

I wrote in this MR a proof of concept of how it's working, that one can reproduce with :

In [1]: from pyroute2 import IPRoute

In [2]: ipr = IPRoute(ext_ack=True)

In [3]: ipr.link("dump", if_netnsid=4)  # here 4 must be an invalid netns number
{'error': -22, 'attrs': [('NLMSGERR_ATTR_MSG', 'Invalid target network namespace id')], 'header': {'length': 60, 'type': 3, 'flags': 514, 'sequence_number': 255, 'pid': 30621}}

However, there are still a lot of work before to be able to merge it. I didn't find a good way to parse standard nlmsg as nlmsgerr when NLM_F_ACK_TLVS flah is set.

And there are a lot of missing feature, like capped messages, cookies, etc. But I have currently no idea of the best idea to go forward. Could you have a look on it?

It's a proof of concept patch:
 * add nlmsgerr class
 * add ext_act option to call setsockopt
@svinota
Copy link
Owner

svinota commented Dec 21, 2020

Great! Thanks a lot, looking

@ffourcot
Copy link
Collaborator Author

Hello @svinota, any news on this topic? I didn't had the time to investigate until now, but I still think that this feature could be great in pyroute2. Any advice to move forward?

Thanks

@svinota
Copy link
Owner

svinota commented Feb 20, 2021

Didn't have time either, but I believe the project must have this feature.

Let me play a bit over this weekend and I'll merge it in this or other way.

@@ -175,6 +178,11 @@ def parse(self, data, seq=None, callback=None):

try:
msg.decode()
# TODO: not the good method to do that
Copy link
Owner

Choose a reason for hiding this comment

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

Why don't you think it's a proper place to parse an error message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my opinion, this should be in msg.decode() itself, if possible. It looks weird for me to call msg.decode() twice

And it's not only about error parsing. I hardcoded the nlmsgerr class here, but we can receive some "cookies" with relevant data when extack is enabled

@svinota
Copy link
Owner

svinota commented Feb 23, 2021

The change looks quite simple, I'm for it

@svinota
Copy link
Owner

svinota commented Mar 2, 2021

Looks like I don't understand something, but can not get it working.

@ffourcot , could you please provide a testcase that would run on net-next 5.11+?

@ffourcot
Copy link
Collaborator Author

ffourcot commented Mar 3, 2021

Hello @svinota,

Yes! I will take some times to provide two scripts (one returning one error, and one with cookies)

@svinota
Copy link
Owner

svinota commented Mar 5, 2021

Got it working, thus I can proceed now with implementation.

Regarding extra decoding — I'll look if it may be done more properly.

@svinota
Copy link
Owner

svinota commented Mar 6, 2021

@ffourcot I plan to raise an exception on nlmsgerr() as it's done on ordinary errors.

Any objections / comments ?

(venv) {ffourcot-extended_ack} $ cat e55.py 
from pyroute2 import IPRoute
with IPRoute(ext_ack=True) as ipr:
    ipr.link('dump', if_netnsid=4)

(venv) {ffourcot-extended_ack} $ python e55.py 
Traceback (most recent call last):
  File "/home/test/Projects/pyroute2/e55.py", line 4, in <module>
    ipr.link('dump', if_netnsid=4)
  File "/home/test/Projects/pyroute2/pyroute2/iproute/linux.py", line 1378, in link
    ret = self.nlm_request(msg,
  File "/home/test/Projects/pyroute2/pyroute2/netlink/nlsocket.py", line 390, in nlm_request
    return tuple(self._genlm_request(*argv, **kwarg))
  File "/home/test/Projects/pyroute2/pyroute2/netlink/nlsocket.py", line 881, in nlm_request
    for msg in self.get(msg_seq=msg_seq,
  File "/home/test/Projects/pyroute2/pyroute2/netlink/nlsocket.py", line 393, in get
    return tuple(self._genlm_get(*argv, **kwarg))
  File "/home/test/Projects/pyroute2/pyroute2/netlink/nlsocket.py", line 718, in get
    raise msg['header']['error']
pyroute2.netlink.exceptions.NetlinkError: (-22, 'Invalid target network namespace id')

@ffourcot
Copy link
Collaborator Author

ffourcot commented Mar 6, 2021

Got it working, thus I can proceed now with implementation.

Great! Thank you

@ffourcot I plan to raise an exception on nlmsgerr() as it's done on ordinary errors.

It's perfect from my point of view. The real message from kernel code will really help

@svinota
Copy link
Owner

svinota commented Mar 7, 2021

Merged manually.

The condition to raise the exception is NLMSG_DONE + NLM_F_ACK_TLVS + msg['error'], sinceNLM_F_ACK_TLVS may happen in other dumps as well but w/o any error attached.

Please review the functionality and don't hesitate to open tickets and PR:s.

@svinota svinota closed this Mar 27, 2021
svinota added a commit that referenced this pull request Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants