Change in vdsm[master]: Basic tests for the tc module

danken at redhat.com danken at redhat.com
Fri Jul 13 19:42:38 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: Basic tests for the tc module
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(9 inline comments)

Thank you very much, Roman, for this contribution. I have several minor comments, and one big request: if you can verify that a ping into bridge_1 is actually mirrored into bridge_2.

....................................................
File AUTHORS
Line 33:    Roman Fenkhuber <roman at fenkhuber.at>
please keep sorted.

....................................................
File tests/Makefile.am
Line 28: 	tcTests.py \
please keep sorted

Line 47: 	vdsClientTests.py
hmm, it's not quite sorted... but still.

....................................................
File tests/tcTests.py
Line 48: class TestProcessRequest(TestCaseBase):
_process_request is a badly-named function that should be kept as a private implementation detail. I do not see the grand benefit of testing it here.

Line 52:                 _process_request(['/usr/bin/echo', '-n', 'hello']))
If you decide to keep this test, please use EXT_ECHO = "/bin/echo"

so it works on my Fedora ;-)

Line 99:         self.assertTrue('PROMISC' in _process_request([EXT_IFCONFIG,
could you use

 ethtool.get_flags(self.bridge) & ethtool.IFF_PROMISC

for that? We are trying to get rid of EXT_IFCONFIG.

Line 115:     @ValidateRunningAsRoot
I think it's enough to perform this validation on setUp. if it fails there, it won't be run here.

Line 122:         tc.qdisc_add_ingress(self.bridge_1)
I'd prefer to test the higher-level function, tc.setPortMirroring instead of copying two of its lines to here.

Line 126:         self.assertTrue("mirred" in filter)
Could you at least add a

 TODO: verify that traffic on bridge_1 is mirrored to bridge_2

? if you can actually implement the test (ping + tcpdump?) it would be even better.

--
To view, visit http://gerrit.ovirt.org/6225
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iee9437d1b5a96b3896df157f13888485ae7292d2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Roman Fenkhuber <roman at fenkhuber.at>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>


More information about the vdsm-patches mailing list