Code review comment for lp:~soren/nova/iptables-concurrency

2011/2/22 Vish Ishaya <email address hidden>:
> This is awesome stuff soren. I'm testing and there are a few issues:
>
> 1. The purpose of SNATTING is so that all the snatting rules can run AFTER all of the postrouting rules.  It looks like you've done it the other way around.

Ah, I see. Yeah, that intent wasn't entirely clear to me. Fixed, thanks!

> Also this seems to break with multiple workers on the same host.

Yes, the "local" chain has the same problem. I realised this as I was
falling asleep last night. I'll try to come up with a way to address
this.

> 2. The snatting rule for public ips is broken:
> 404     +            ("SNATTING", "-d %s -j SNAT --to %s" % (fixed_ip, floating_ip))]
> should be: "-s %s..."

Wow. Good catch!

> 3. I think you added a rule to both SNATTING and POSTROUTING by accident:
> 333     +    iptables_manager.ipv4['nat'].add_rule("SNATTING",
> 334     +                                          "-s %s -j SNAT --to-source %s" % \
> 335     +                                           (FLAGS.fixed_range,
> 336     +                                            FLAGS.routing_source_ip))
> 337     +
> 338     +    iptables_manager.ipv4['nat'].add_rule("POSTROUTING",
> 339     +                                          "-s %s -j SNAT --to-source %s" % \
> 340     +                                           (FLAGS.fixed_range,
> 341     +                                            FLAGS.routing_source_ip))
>
> This rule should only be in SNATTING

Fixed, thanks!

> 4. The old code added the above rule to the SNATTING chain, but added all other floating ips to the beginning of the chain.  The purpose of this is so that it is a fallback.  If an instance has a public ip it snats to its public ip, otherwise it uses the routing source ip.  The current ordering means that all instances will always use the routing_source_ip.  Consider the following:

That makes perfect sense. Thanks! I didn't realise this was how it
worked before. I'll fix this.

--
Soren Hansen        | http://linux2go.dk/
Ubuntu Developer    | http://www.ubuntu.com/
OpenStack Developer | http://www.openstack.org/