Hi!
thanks for the review.
On Tue, Jan 22, 2013 at 5:14 PM, Michael Hanselmann <han...@google.com>wrote:
> 2013/1/22 Helga Velroyen <hel...@google.com>:
> > --- a/qa/qa-sample.json
> > +++ b/qa/qa-sample.json
> > + "networks": {
> > + "inexistent-networks": [
> > + "network1",
> > + "network2",
> > + "network3",
>
> The comma here makes the file invalid JSON.
>
Ok, done.
>
> > + ]
> > + }
> > +
> > "tests": {
> > "# Whether tests are enabled or disabled by default": null,
> > "default": true,
> > @@ -171,7 +179,9 @@
> > "# Make sure not to include the disk(s) required for Dom0 to be
> up": null,
> > "# in the volume group used for instances. Otherwise the whole":
> null,
> > "# system may stop working until restarted.": null,
> > - "instance-disk-failure": false
> > + "instance-disk-failure": false,
> > +
> > + "network": false
>
> Please move this further up, e.g. between “group*” and “node*”. The
> tests at the end are a bit “special”.
>
Okay.
>
> > },
> >
> > "options": {
> > diff --git a/qa/qa_network.py b/qa/qa_network.py
> > new file mode 100644
> > index 0000000..6eeb099
> > --- /dev/null
> > +++ b/qa/qa_network.py
> > +def GetNonexistentNetworks(count):
> > + """Gets network names which shouldn't exist on the cluster.
> > +
> > + @param count: Number of networks to get
> > + @rtype: list
> > +
> > + """
> > + networks = qa_config.get("networks", {})
> > +
> > + default = ["network1", "network2", "network3"]
> > + assert count <= len(default)
> > +
> > + candidates = networks.get("inexistent-networks", default)[:count]
> > +
> > + if len(candidates) < count:
> > + raise Exception("At least %s non-existent networks are needed" %
> count)
> > +
> > + return candidates
>
> Duplicated logic from GetNonexistentGroups. Please merge.
>
Okay, interdiff:
diff --git a/qa/qa_network.py b/qa/qa_network.py
index 6eeb099..5648fd6 100644
--- a/qa/qa_network.py
+++ b/qa/qa_network.py
@@ -33,31 +33,23 @@ def GetNonexistentNetworks(count):
"""Gets network names which shouldn't exist on the cluster.
@param count: Number of networks to get
- @rtype: list
+ @rtype: integer
"""
- networks = qa_config.get("networks", {})
-
- default = ["network1", "network2", "network3"]
- assert count <= len(default)
-
- candidates = networks.get("inexistent-networks", default)[:count]
-
- if len(candidates) < count:
- raise Exception("At least %s non-existent networks are needed" % count)
-
- return candidates
+ return qa_utils.GetNonexistentEntityNames(count, "networks", "network")
def TestNetworkAddRemove():
"""gnt-network add/remove"""
(network1, network2) = GetNonexistentNetworks(2)
- # Add some networks of different sizes
- AssertCommand(["gnt-network", "add", "--network", "192.168.0.1/30",
network1])
- AssertCommand(["gnt-network", "add", "--network", "192.168.1.0/24",
network2])
+ # Add some networks of different sizes.
+ # Note: Using RFC5737 addresses.
+ AssertCommand(["gnt-network", "add", "--network", "192.0.2.0/30",
network1])
+ AssertCommand(["gnt-network", "add", "--network", "198.51.100.0/24",
+ network2])
# Try to add a network with an existing name.
- AssertCommand(["gnt-network", "add", "--network", "192.168.2.0/24",
network2],
+ AssertCommand(["gnt-network", "add", "--network", "203.0.133.0/24",
network2],
fail=True)
AssertCommand(["gnt-network", "remove", network1])
@@ -80,7 +72,7 @@ def TestNetworkConnect():
link = default_link
AssertCommand(["gnt-group", "add", group1])
- AssertCommand(["gnt-network", "add", "--network", "192.168.0.1/24",
network1])
+ AssertCommand(["gnt-network", "add", "--network", "192.0.2.0/24",
network1])
AssertCommand(["gnt-network", "connect", network1, mode, link, group1])
AssertCommand(["gnt-network", "disconnect", network1, group1])
diff --git a/qa/qa_utils.py b/qa/qa_utils.py
index 45baea6..6c8fa76 100644
--- a/qa/qa_utils.py
+++ b/qa/qa_utils.py
@@ -653,17 +653,40 @@ def GetNonexistentGroups(count):
"""Gets group names which shouldn't exist on the cluster.
@param count: Number of groups to get
- @rtype: list
+ @rtype: integer
"""
- groups = qa_config.get("groups", {})
+ return GetNonexistentEntityNames(count, "groups", "group")
- default = ["group1", "group2", "group3"]
+
+def GetNonexistentEntityNames(count, name_config, name_prefix):
+ """Gets entity names which shouldn't exist on the cluster.
+
+ The actualy names can refer to arbitrary entities (for example
+ groups, networks).
+
+ @param count: Number of names to get
+ @rtype: integer
+ @param name_config: name of the leaf in the config containing
+ this entity's configuration, including a 'inexistent-'
+ element
+ @rtype: string
+ @param name_prefix: prefix of the entity's names, used to compose
+ the default values; for example for groups, the prefix is
+ 'group' and the generated names are then group1, group2, ...
+ @rtype: string
+
+ """
+ entities = qa_config.get(name_config, {})
+
+ default = [name_prefix + str(i) for i in range(count)]
assert count <= len(default)
- candidates = groups.get("inexistent-groups", default)[:count]
+ name_config_inexistent = "inexistent-" + name_config
+ candidates = entities.get(name_config_inexistent, default)[:count]
if len(candidates) < count:
- raise Exception("At least %s non-existent groups are needed" % count)
+ raise Exception("At least %s non-existent %s are needed" %
+ (count, name_config))
return candidates
>
> > +def TestNetworkAddRemove():
> > + AssertCommand(["gnt-network", "add", "--network", "192.168.0.1/30",
> network1])
>
> Use addresses from RFC 5737, elsewhere too.
>
Okay.
Cheers,
Helga
