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

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. Also this seems to break with multiple workers on the same host.

    1 60 nova-network-SNATTING all -- * * 0.0.0.0/0 0.0.0.0/0
    1 60 nova-network-POSTROUTING all -- * * 0.0.0.0/0 0.0.0.0/0
    1 60 nova-compute-SNATTING all -- * * 0.0.0.0/0 0.0.0.0/0
    1 60 nova-compute-POSTROUTING all -- * * 0.0.0.0/0 0.0.0.0/0

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..."

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

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:

Chain nova-network-SNATTING (1 references)
 pkts bytes target prot opt in out source destination
    0 0 SNAT all -- * * 10.0.0.0/8 0.0.0.0/0 to:10.0.0.1
    0 0 SNAT all -- * * 10.0.0.2 0.0.0.0/0 to:10.6.6.0

(the 10.0.0.2 was in destination, i moved it manually, because it is addressed by item 2 above)
10.0.0.2 is the fixed ip of the instance
10.6.6.0 is the public_ip of the instance
10.0.0.1 is the routing_source_ip
note that the snat rule at the bottom will never get hit because the first rule will catch all of them. In order to make this work properly you either need to add another chain that always runs after SNATTING, or you need a way to ensure that the floating rules get added to the beginning of the SNATTING chain.

Vish