search

LNST-project / lnst

Linux Network Stack Test
GNU General Public License v2.0
73 stars 33 forks source link

Devices/Device.py: Fix for broken cleanup() with missing tc tool #362

Closed SirPhuttel closed 8 months ago

SirPhuttel commented 8 months ago

If no tc utility is present on the agent system, exec_cmd() raises an exception. If uncaught, the previous address flush request will also not complete. The address remaining in place will cause an EEXIST error upon next test run, requiring manual intervention to resolve the situation.

Fixes: 49e523d5aef46 ("Slave: Add basic support for tc.")

SirPhuttel commented 8 months ago

@jtluka my bet was right, this fixes the problem. Could you please review?

jtluka commented 8 months ago

The fix looks ok in general, however I'd like to enhance it a bit so that we don't silently discard potential issues when iproute-tc is installed but fails when executed.

In particular I'd like to have error message check for "command not found" similar to what we have in https://github.com/LNST-project/lnst/blob/bd86ed010dc69dee0277717e384059e18e173db5/lnst/Devices/Device.py#L926

Maybe using function decorator similar to sriov_capable, would be best. Decorator definition is here

Anyway, I'm ok with the patch as is, @olichtne what's your opinion?

olichtne commented 8 months ago

Anyway, I'm ok with the patch as is, @olichtne what's your opinion?

i'm it should be ok to simply do

except ExecCmdFail:
    logging.exception("tc command error during cleanup")

this will log both the traceback and some basic message instead of silently passing the exception.

jtluka commented 8 months ago

Anyway, I'm ok with the patch as is, @olichtne what's your opinion?

i'm it should be ok to simply do

except ExecCmdFail:
    logging.exception("tc command error during cleanup")

this will log both the traceback and some basic message instead of silently passing the exception.

The exception is already logged in the exec_cmd code, so this is not necessary.

olichtne commented 8 months ago

ah, fair point, in that case this should be ok as is.

wrt. sriov_capable i'm not sure what that would add in this case?

jtluka commented 8 months ago

wrt. sriov_capable i'm not sure what that would add in this case?

Avoiding duplication of the code. If we were to add error message parsing, we'd have to include that in both _clear_tc_qdisc and _clear_tc_filters. Using wrapper that would do the check of iproute-tc availability would keep the code at single place.

But it's not worth it. Let's merge this.