On Fri, Jul 29, 2011 at 3:09 PM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
> Here are a few additional points Jim.  Nothing huge, I think:
>
>
> [1]
>
>         self._watched_services = {}
> +        self._watched_machines = set()
>
>
> Bogus line.  It's duplicated.
>

Fixed


>
>
> [2]
>
>             machine = yield self.provider.get_machine(instance_id)
>             machine.machine_id = machine_id
>             (...)
>             current_ports = yield self.provider.get_opened_ports(machine)
>
> We really need to fix this.  You ask the provider for a machine, it
> gives you the machine with a field it has no idea about how to
> retrieve, you stuff an id into that object, and then give it _back_
> to the provider because it will need it.
>
> Result:
>
> - The provider is the one responsible for its machines, but can't
>  create one with the needed data.
> - The provider expects a machine_id in arbitrary moments, and doesn't
>  tell when.
> - Arbitrary locations set the id, because they magically know the dance.
>
> No one but you understand that.  We need a more consistent API to
> handle this.
>
> If get_open_ports and friends need unique identifier, let's provide it
> explicitly.  This should be the external id, not the internal id used
> within zookeeper.
>
> Unless the providers can set the machine_id attribute, please take it
> off from the machine.
>

Changed accordingly to a model where machine_id is passed explicitly.
Confirmed with William that this is OK.


>
>
> [3]
>
> +    def __init__(self, instance_id, dns_name=None,
> +                 private_dns_name=None, launch_time=None,
> machine_id=None):
>
> Per above, please drop this.
>

Reverted


>
> [4]
>
> -        launch_deferred = self.get_machine_variables(machine_data)
> -        launch_deferred.addCallback(self.start_machine)
> +        machine_variables = yield self.get_machine_variables(machine_data)
> +        provider_machines = yield self.start_machine(
> +            machine_variables, machine_data)
>
> The idea of having a data bag like machine_data was a bad one.  Now we
> have logic all around which depends on arbitrary fields inside it, and
> do not say so.  Let's not increase the problem.
>
> If start_machine needs the machine_id, let's just provide the machine_id
> rather than a bag.
>

Changed so that start_machine takes machine_id. Note there are two
start_machine functions, which is probably not ideal. I only refactored this
one.


>
> [5]
>
> +        return open_provider_port(self, machine, port, protocol)
>
> Much better, thanks!
>

Cool!


>
> [6]
>
> +        except EC2Error:
> +            # AWS may return an error when the instance is not found
> +            # (but presumably eventually will be), eg,
> +            # txaws.ec2.exception.EC2Error: Error Message:
> +            # The instance ID 'i-afe113ce' does not exist
> +            raise ProviderInteractionError(
> +                "Machine (id: %r) was not found" % provider_id)
>
> This looks too generic.  What other errors can EC2Error represent?
>

Well, I have certainly seen them, but I don't know of a complete list. I
will look. I have made this and the similar usage in
ensemble.providers.ec2.accessor, more generic.


>
> Also, should we perhaps try again a couple of times before giving
> up if the provider_id was indeed not found, to compensate for the
> boredom of eventual consistency?
>
> For the latter, free to just add a comment in case you don't want
> to do now.  The former looks like an issue we'll want to address
> before merging.
>

Hmm, I think the best way to do this is take advantage of the periodic
machine check (and make that more robust per bug 639888). I added some
corresponding comments.

A scenario where a simple retry might have issues is where a service is
mistakenly exposed then quickly unexposed. It's possible that a retry
associated with exposed might stamp on that of unexposed.


> [7]
>
> -            # TODO: For now authorize external traffic to the instance,
> -            # except for the ZooKeeper port.  The expose functionality
>
> Good to see that stuff going away.
>

Definitely


>
> [8]
>
> +        if ensemble_machine_group in group_ids:
> +            yield self._provider.ec2.delete_security_group(
> +                ensemble_machine_group)
>
> Has this logic been tested against EC2?  Won't this command explode if
> the group does not exist?
>

Tested numerous times, but I added tests around scenarios of security groups
existing, including getting deleted in a race (which would trigger the
above).


>
> [9]
>
> +    except EC2Error:
> +        raise ProviderInteractionError(
> +            "Machine (id: %r) not found when attempting to open %s/%s" % (
>
> Again, is that the only error that can occur?  EC2Error feels more generic
> than that.
>

Generalized


>
> [10]
>
> +    except EC2Error:
> +        raise ProviderInteractionError(
> +            "Machine (id: %r) not found when attempting to close %s/%s" %
> (
>
> Ditto.
>

Likewise


>
> [11]
>
> +    except EC2Error:
> +        raise ProviderInteractionError(
> +            "Machine (id: %r) not found when attempting "
> +            "to get opened ports" % machine.instance_id)
>
> Ditto.
>

Same


>
> [12]
>
> +        for port in xrange(int(ip_permission.from_port),
> +                           int(ip_permission.to_port) + 1):
> +            opened_ports.add((port, ip_permission.ip_protocol))
>
> Feels like we may have issues around this.  I can easily see someone
> adding a huge range of ports, and Ensemble exploding while trying to
> handle them individually.
>
> We only open individual ports, right?  If so, I suggest skipping
> ranges, since they must necessarily have been opened by hand.
>

Now skips ranges, and of course tests that case.