Re: [libvirt] [PATCH v3 2/6] virnetdaemon: Add post exec restart support for multiple servers
On 10/01/16 16:27, Martin Kletzander wrote:
On Mon, Nov 30, 2015 at 04:03:01PM +0100, Erik Skultety wrote:
> Since the daemon can manage and add (at fresh start) multiple servers,
> we also should be able to add them from a JSON state file in case of a
> daemon restart. This patch introduces virNetDaemonAddServersPostExec
> method which harvests the data about servers from a JSON file supporting
> both old format with a single server and a new one storing an array of
> servers. The method makes use of the original
> virNetDaemonAddServerPostExec,
> declaring the latter as static.
> Patch also updates virnetdaemontest accordingly.
There's only one thing wrong with this patch: In virnetdaemontest you
remove the ability for testing something that should've failed. You
also effectively remove one such test. Was it because you forgot about
it or did you remove it intentionally? I would prefer it to stay there.
The removal is the only thing keeping me from acking this patch.
Hmm, I can't think of any reasons why I did that, but you're definitely
right about including such a test in there. However, I did a small
investigation and found out, that the current solution doesn't work as
expected, see the following scenarios:
SCENARIO 1} Include a test, that should pass, but fails unexpectedly
OUT:
TEST: virnetdaemontest
1) ExecRestart initial ... OK
2) ExecRestart initial-nomdns ... OK
3) ExecRestart anon-clients ... OK
4) ExecRestart anon-clients ... libvirt: XML-RPC error : internal
error: Cannot add more servers post-exec than there were pre-exec
libvirt: XML-RPC error : internal error: Cannot add more servers
post-exec than there were pre-exec
FAILED
5) ExecRestart admin-nomdns ... OK
I separated test case 4 so it is clear that there are 2 identical error
messages caused by multiple calls to virDispatchError, first being in
virnetdaemontest and the other one in virtTestRun routine.
SCENARIO 2} Include a test which should fail, but would actually pass
TEST: virnetdaemontest
1) ExecRestart initial ... OK
2) ExecRestart initial-nomdns ... OK
3) ExecRestart anon-clients ... OK
4) ExecRestart anon-clients ... libvirt: XML-RPC error : internal
error: Cannot add more servers post-exec than there were pre-exec
OK
5) ExecRestart admin-nomdns ... libvirt: XML-RPC error : internal
error: Test should've failed
OK
Testcase 5 failed with error stating that the test should have failed
the execution but it didn't. Yet, the testcase returns success.
I'll include all necessary changes in this patch once I adjust all the
remaining ones in this v3 series and post them all.
Erik