Thanks a lot for the review. Hope the following patch (next mail) will address all your concerns.
On Tue, Feb 22, 2011 at 4:52 PM, Michael Hanselmann <[email protected]> wrote: > Am 22. Februar 2011 15:37 schrieb Rene Nussbaumer <[email protected]>: >> On Tue, Feb 22, 2011 at 2:40 PM, Michael Hanselmann <[email protected]> >> wrote: >>> Am 22. Februar 2011 13:55 schrieb René Nussbaumer <[email protected]>: >>>> --- a/lib/client/gnt_cluster.py >>>> +++ b/lib/client/gnt_cluster.py >>>> +def _InstanceStart(opts, inst_list, start): >>>> […] >>>> + results = jex.GetResults() >>>> + bad_cnt = len([False for (success, _) in results if not success]) >>> >>> No need for a list, just use a generator. >> >> I don't get that > >>>> type([False for _ in range(2)]) > <type 'list'> >>>> type(False for _ in range(2)) > <type 'generator'> Ah, got it now. Thanks :) > >>>> + # This shouldn't fail >>>> + AssertCommand(["gnt-cluster", "epo", "-f", "--all"]) >>> >>> Won't this shut down all machines? >> >> No, at this stage the QA no OOB script, if the the nodes do not >> support OOB they will not been shut down and just beeing noted on the >> output that they need manual care. > > Please add a check asserting this. Done. > >>>> + # All instances should have been stopped now >>>> + result_output = GetCommandOutput(master["primary"], >>>> + "gnt-instance list --no-header -o >>>> status") >>>> + AssertEqual(compat.any(status == "running" >>>> + for status in result_output.splitlines()), False) >>> >>> You should rather check that all are stopped. They could be, after >>> all, in some error status. >> >> That was intentional, because the command would have failed in one of >> the instances failed stopped, so we wouldn't reach that point. But I >> can change … to ADMIN_down I guess? > > Yes, please. Done. >>>> + def _FakePing(*args, **kwargs): >>>> + return True >>> >>> Here you should verify that live_port, etc. is set correctly. >> >> I don't see the point in that. This function should just emulate a >> host up, not checking anything. > > You should make sure the helper function calls the function with the > right parameters. See > ganeti.jqueue_unittest.py:_FakeQueueForProc.acquire for example. Okay, done. >>>> + def _FakeSleep(secs): >>> >>> Assert that secs is >= 0 and <= some value. >> >> Same applies here. > > You want to make sure the wait time calculation doesn't go wrong. Done. >>>> + def testPingFnRemoveHostsUp(self): >>>> + seen = {} >>>> + def _FakeSeenPing(ip, *args, **kwargs): >>>> + node = self.ips2node[ip] >>>> + >>>> + if node in seen: >>>> + seen[node] += 1 >>>> + else: >>>> + seen[node] = 1 >>>> + return True >>> >>> If you make “seen” a set, this becomes much simpler. >>> >>> self.assertFalse(node not in seen) >>> seen.add(node) >> >> But this is not doing the same. I want to have exactly 1 call for >> every node, not 0, not 2, just 1. If I simply add them into the set I >> don't know how many times it was called. > > You don't need to. _FakeSeenPing will already fail if it's called more > than once. Ahhh true, because of the assert. Changed. René
