Code review comment for lp:~jimbaker/pyjuju/expose-provider-ec2

On Tue, Aug 9, 2011 at 11:41 AM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Approve
> Looks good, +1 with a few extra comments:
>
> [13]
>
> + raise ProviderInteractionError(
> + "Got EC2 error (id %r: %s) when looking up machine" % (
> + provider_id, ex))
>
> Please reword it like this:
>
> "EC2 error when looking up instance %s: %s" % (provider_id, e)
>

Done

>
>
> [14]
>
> + raise ProviderInteractionError(
> + "Machine (id: %r) was not found" % provider_id)
>
> Please reword as:
>
> "EC2 instance %s was not found"
>

Done

>
> [15]
>
> + except EC2Error:
> + log.debug("Ignoring machine group %s that just
> disappeared",
> + ensemble_machine_group)
> + pass # Security group for machine is no longer around,
> ignore
>
> The pass + comment may be dropped as it's redundant with the previous line.
>

Removed pass, but per conversation in sprint room, the logic was wrong, so
it nows raises this as ProviderInteractionError. This is because this is
almost certainly because of a security group that cannot be deleted because
it's used by another instance (generally being shut down). This will be
addressed in another branch that does security group cleanup on ensemble
shutdown.

>
> [16]
>
> +# TODO These security group functions do not handle the eventual
> +# consistency seen with EC2. This is likely not the right place to
> +# address this issue. The provisioning agent is more likely the place
> +# to do this.
>
> This would mean we're introducing eventual consistency within our API,
> which sounds really bad to handle. The eventual consistency aspect
> should be handled as closely to the EC2 API as possible, rather than
> across all of the library.
>
> Please adapt the comment to reflect this.
>

Fixed

>
> [17]
>
> + "Got EC2 error (id %r: %s) when attempting to open %s/%s" % (
> + machine.instance_id, ex, port, protocol))
>
> "EC2 error when attempting to open %s/%s on machine %s: %s"
>

Fixed

>
> [18]
>
> Similar as the above for the other messages.
>

Fixed

>
> [19]
>
> + for ip_permission in security_groups[0].allowed_ips:
> + if ip_permission.cidr_ip != "0.0.0.0/0":
>
> Is it possible for this list to be empty? In which circumstances?
> Do we have to handle it to avoid breaking the agent entirely?
>

security_groups will not be empty

>
> [20]
>
> + if from_port == to_port:
> + # Ignore ranges of ports, since they are set outside of
> + # Ensemble (at this time at least)
>
> This comment seems reversed. This branch of logic is the one that
> does _not_ ignore. It should probably be reworded in the positive
> side to be more clear ("Only take ...").
>

Changed

>
> [21]
>
> It would be awesome to take the chance we have Mark and Clint here
> and take them through a whole run of that branch before merging,
> both to share the knowledge and to help ensuring the latest changes
> work fine.
>

Mark and Jorge both ran this successfully (a reasonable sub for Clint).