[Bug report] dataplane poll with default port_number doesn't work when there are multiple ptf nn agents

[w1nda]

Source code: #https://github.com/p4lang/ptf/blob/c554f83685186be4cfa9387eb5d6d700d2bbd7c0/src/ptf/dataplane.py#L900

Simplified code:

    def poll(
        self, device_number=0, port_number=None, timeout=None, exp_pkt=None, filters=[]
    ):
        def grab():
            self.logger.debug("Grabbing packet")
            for rcv_port_number, pkt, time in self.packets(device_number, port_number):
                rcv_device_number = device_number
                if not exp_pkt or match_exp_pkt(exp_pkt, pkt):
                    return DataPlane.PollSuccess(
                        rcv_device_number, rcv_port_number, pkt, exp_pkt, time
                    )
            return None

        with self.cvar:
            ret = ptfutils.timed_wait(self.cvar, grab, timeout=timeout)

        return ret

The problem is: when there are multiple ptf nn agents connecting the dataplane, there will be equvilent count of "device numbers", however, when we call poll with port_number=None, it will only poll packet from the device with number 0.

That doesn't make sense, when exp_pkt is not none and port number is none, we should poll packets from all devices.

[jafingerhut]

Would it be even more flexible and potentially useful if the following options were provided to callers of poll?

1. The current behavior, where the caller can specify a single device number to perform the poll operation on.
2. A new behavior, perhaps indicated by providing a new special named value as the device_number parameter to mean "all devices", that behaves as you suggest?

It seems worth preserving a way to get behavior 1, in case someone really wants it.

[w1nda]

Would it be even more flexible and potentially useful if the following options were provided to callers of poll?

1. The current behavior, where the caller can specify a single device number to perform the poll operation on.
2. A new behavior, perhaps indicated by providing a new special named value as the device_number parameter to mean "all devices", that behaves as you suggest?

It seems worth preserving a way to get behavior 1, in case someone really wants it.

Thanks for help

Yes, behavior 1 is worth preserving.

For behavior 2, that's what I need, I agree with your proposal, when can reserve a special value for device_number to mean "all devices", as the "all port" is indicated by None, so I think None is a good option.

[jafingerhut]

Are you willing to write a PR with a proposed change, for others to review?

Ideally it would be best if such a change was backwards compatible, i.e. except for the new case of device_number=None with new behavior, all existing cases should behave the same when device_number != None.

[w1nda]

Are you willing to write a PR with a proposed change, for others to review?

Ideally it would be best if such a change was backwards compatible, i.e. except for the new case of device_number=None with new behavior, all existing cases should behave the same when device_number != None.

Yes, I'm happy to code that.

However, seems that there is no test in ptf repo, so I think it's better to patch ptf package in sonic repo, when the patch works fine and was tested with good quality, I will backport the patch to ptf repo.

[jafingerhut]

Are you willing to write a PR with a proposed change, for others to review? Ideally it would be best if such a change was backwards compatible, i.e. except for the new case of device_number=None with new behavior, all existing cases should behave the same when device_number != None.

Yes, I'm happy to code that.

However, seems that there is no test in ptf repo, so I think it's better to patch ptf package in sonic repo, when the patch works fine and was tested with good quality, I will backport the patch to ptf repo.

That sounds like a reasonable plan.