Handling of NLM_F_DUMP_INTR is apparently missing

[crosser]

We got a hard to reproduce condition in our tests, when a dump of network interfaces and addresses returned incomplete information. Because we have already encountered a related problem with nftables that turned out to be related to improper reaction NLM_F_DUMP_INTR condition, I suspect that the same thing is happening in pythoute2.

The point is, when the kernel is producing a dump of a kernel structure over multiple netlink messages, and the structure changes in the mid-way, NLM_F_DUMP_INTR is added to the header flags, that must be treated by userspace as an indication that the requested dump contains inconsistent data and must be re-requested. See function nl_dump_check_consistent() in the kernel source include/net/netlink.h.

I cannot find related functionality in pyroute2, which may explain the error that we encountered. I suspect that the necessary check for the flag NLM_F_DUMP_INTR in the received netlink messages is missing.

[svinota]

Yep, looks like that. Thanks, investigating!

[svinota]

Yep. What kind of the feedback would you expect here — a simple flag in the response, or an exception raised?

[crosser]

Either of those will do, though in my opinion it will be fine to simply retry the request until it succeeds completes with anything else than NLM_F_DUMP_INTR, without exposing it to the user. It is not, logically, and "error situation", just a "normal situation" that is exposed to userspace only because kernel cannot recover from it transparently, it needs to retroactively invalidate data that it has already passed over.

[svinota]

Things get more complicated. I've reproduced the 0x10 flag NLM_F_DUMP_INTR responses having ~50k IP addresses on the system, but look.

By default the library reads the response and returns it to the user as a list, but it's only the default historical behaviour. Having nlm_generator = True the library returns generators instead of lists, thus making transparent subsequent retries impossible: the response may be partly returned at the time we get a message with the flag 0x10 in the header.

So I leave it as is for now — only adding the flag, thanks — and as a workaround I can propose checking the response's msg['header']['flags'] for NLM_F_DUMP_INTR:

from pyroute2.netlink import NLM_F_DUMP_INTR

for msg in ipr.addr('dump'):
    if msg['header']['flags'] & NLM_F_DUMP_INTR:
        the_retry_logic()

If you can propose any improvements and shortcuts that will not break the compatibility — pls ping me. Thanks!

[crosser]

I realize that if the procedure that returns the data is a generator, than it is impossible to make retry logic invisible to the caller.

However just hoping that all users of the library will know that they need to check an obscure flag, in my opinion, will not work. The next person who, like me, suddenly encounters truncated data will have no idea what's going on.

In addition to that, such approach will make it impossible to use list comprehensions like this (taken from real code):

def _get_all_ifs(ipr):
    return [
        (
            el["index"],
            el.get_attr("IFLA_IFNAME"),
            el.get_nested("IFLA_LINKINFO", "IFLA_INFO_KIND"),
        )
        for el in ipr.get_links()
    ]

that I, for one, rather like! 😁

I think that it may be more "user-friendly" to raise an exception with a suggestive name. Then when this (rare) situation happens, at least the user will get a clue what happened, rather than struggle with mysteriously truncated result. With exception, I would write the above code as

def _get_all_ifs(ipr):
    while True:
        try:
            return [
                (
                    el["index"],
                    el.get_attr("IFLA_IFNAME"),
                    el.get_nested("IFLA_LINKINFO", "IFLA_INFO_KIND"),
                )
                for el in ipr.get_links()
            ]
        except NetlinkDumpInterrupted:
            continue

I my eyes, this looks less ugly than checking the flag in every returned element.

What do you think?

P.S. I am not 100% sure, but it may be necessary to consume potentially remaining messages waiting in the socket, after the one with raised NLM_F_DUMP_INTR flag, unless you close and reopen the socket. See this patch as an example. "Exception" approach allows to do it in the library, before raising the exception, and deny the user the opportunity to make another error.

[crosser]

As it seems to work now, are there plans to tag a release?.. We'd build and internal package, it's nicer to have it anchored on a tagged version than on a commit id... Thanks!

[svinota]

Yep, sure. Let's tag it on Thursday — I would like to have one more feature in the release :) And hope to add it tonight + some time for testing.

[svinota]

Ok, closing #878 and tagging the release.

NB: pls take look at #881

[svinota]

tagged

[svinota]

Closing the ticket, pls don't hesitate to re-open in the case of any issues.