ClusterHQ / flocker

Container data volume manager for your Dockerized application
https://clusterhq.com
Apache License 2.0
3.39k stars 290 forks source link

flocker-deploy can return erroneous wrong number of arguments message #595

Closed dwgebler closed 10 years ago

dwgebler commented 10 years ago

Due to a bug in the implementation of environment YAML config, flocker-deploy can end up returning an "Error: Wrong number of arguments" message (as a result of twisted.python.usage raising a TypeError with that message), when in fact the correct number of command-line arguments are given.

The real underlying problem is that flocker.node._config assigns the environment attribute of the Characteristic Application model as a dict, which Characteristic then complains is not hashable:

applications[application_name] = Application(
                name=application_name,
                image=image,
                volume=volume,
                ports=frozenset(ports),
                environment=environment)

In this code, environment is either dict or None, so we need to do something like ensuring environment is always a dict (even if empty) and then doing:

applications[application_name] = Application(
                name=application_name,
                image=image,
                volume=volume,
                ports=frozenset(ports),
                environment=frozenset(environment.items()))

This frozenset can later be converted back to a dict and processed as normal.

wallrj commented 10 years ago

Yep, that works. With a slight modification to the GearEnvironment class (below)

The tests and docstrings will probably need updating too.

diff --git a/flocker/node/_config.py b/flocker/node/_config.py
index cab15d7..e8de7d5 100644
--- a/flocker/node/_config.py
+++ b/flocker/node/_config.py
@@ -212,7 +212,7 @@ class Configuration(object):
                 image=image,
                 volume=volume,
                 ports=frozenset(ports),
-                environment=environment)
+                environment=frozenset(environment.items()))

             if config:
                 raise ConfigurationError(
diff --git a/flocker/node/gear.py b/flocker/node/gear.py
index 4a68f86..82da392 100644
--- a/flocker/node/gear.py
+++ b/flocker/node/gear.py
@@ -38,7 +38,7 @@ class GearEnvironment(object):
         the Gear REST API.
         """
         variables = []
-        for k, v in self.variables.items():
+        for k, v in self.variables:
             variables.append(dict(name=k, value=v))
         return dict(id=self.id, variables=variables)
wallrj commented 10 years ago

I think the problem actually occurs when we create a frozenset of the Applications. Which attempts to hash each Application and triggers the hash and the attrs_to_tuple function in characteristic

Perhaps we could suggest an improvement to Characteristic for better handling of dicts and other unhashable attributes. Maybe automatically call dict.items for DictType?

itamarst commented 10 years ago

I don't think that's a good idea lacking ability to make stuff read-only. There's a reason dictionaries aren't hashable; if you mutate it and it's a key in a dictionary unexpected things would happen.