Change in vdsm[master]: Improvement of the GuestAgent class memory usage

zhshzhou at linux.vnet.ibm.com zhshzhou at linux.vnet.ibm.com
Wed Dec 19 09:18:15 UTC 2012


Zhou Zheng Sheng has posted comments on this change.

Change subject: Improvement of the GuestAgent class memory usage
......................................................................


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

(3 inline comments)

I think you can improve the testing code.

....................................................
File tests/guestIFTests.py
Line 102:             t = testCase(*t)
Line 103:             fakeGuestAgent._handleMessage(t.msgType, t.message)
Line 104:             for (k, v) in t.assertDict.iteritems():
Line 105:                 self.assertEqual(fakeGuestAgent.guestInfo[k], v)
Line 106: 
1. Is it better to put the _handelData test code to a test_handleData() test method? The variable definition for testCase, msgTypes, inputs and outpust can be moved to a setUp() method, and assign the values to self._testCase, self._msgTypes, ... nose runs setUp() for each test method so I think it's a good place to put common definitions. Then in a separate  test_handleData() method you can reuse those definitions.


2.
"config.set('vars', 'guest_agent_max_allowed_message_size', '100')" 
and 
"fakeGuestAgent._clearReadBuffer()" 
is not related to  test_handleMessage(), so move them to test_handleData() maybe better.
Line 107:         # Guest agent must not be stopped
Line 108:         fakeGuestAgent._stopped = False
Line 109:         # Retrieving configured message size
Line 110:         maxMessageSize = config.getint('vars',


Line 125:             for chunk in messageChunks(msg_str):
Line 126:                 fakeGuestAgent._handleData(chunk)
Line 127:                 if chunk[-1] != '\n':
Line 128:                     self.assertEqual(fakeGuestAgent._messageState,
Line 129:                                      guestIF.MessageState.TOO_BIG)
If chunk[-1] equals to '\n', the fakeGuestAgent._messageState should be NORMAL.
Line 130: 
Line 131:             # At the end the message state has to be NORMAL again
Line 132:             self.assertEqual(fakeGuestAgent._messageState,
Line 133:                              guestIF.MessageState.NORMAL)


Line 132:             self.assertEqual(fakeGuestAgent._messageState,
Line 133:                              guestIF.MessageState.NORMAL)
Line 134: 
Line 135:             for (k, v) in t.assertDict.iteritems():
Line 136:                 self.assertEqual(fakeGuestAgent.guestInfo[k], v)
After the _handleMessage() testing is executed, the fakeGuestAgent.guestInfo is updated to match the values in the assertDict. Then all the above messages for _handleData() exceeds 100B and are discarded, and the fakeGuestAgent.guestInfo remains the same, so all the assertEquals will be absolutely OK. Even if the messages are not discarded, they will be parsed as before and update the fakeGuestAgent.guestInfo again with the same values. So these assertEquals proves nothing, they can be dropped. Also, if you move the _handleData() testing code to a separate test method, these assertEquals will fail.

I think we can test the following situations.

1. Message is too big and gets discarded, so the fakeGuestAgent.guestInfo is not updated.

2. Message is normal and gets accepted, so the fakeGuestAgent.guestInfo is updated.

3. A big message + normal message are send to _handleMessage() in one time, and the big one gets discarded, the normal one gets accepted, the fakeGuestAgent.guestInfo is updated only by the normal one.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf6274bb10c9e3b80962b69c5df316f03ee21214
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <peet at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list