Merge lp:~soren/nova/iptables-concurrency into lp:~hudson-openstack/nova/trunk

[Soren Hansen]

Add a new IptablesManager that takes care of all uses of iptables.

Port all uses of iptables (in linux_net and libvirt_conn) over to use this new manager.

It wraps all uses of iptables so that each component can maintain its own set of rules without interfering with other components and/or existing system rules.

iptables-restore is an atomic interface to netfilter in the kernel. This means we can make a bunch of changes at a time, minimising the number of calls to iptables.

[Jay Pipes (jaypipes)]

Seems like a lot of voodoo to me. Without any docstrings or documentation in these modules, it's all just Greek to me, not knowing the ins and outs of iptables...

[Rick Harris (rconradharris)]

lgtm, just a few minor-nits:

> 626 + print rule

Might be better to log here (or remove if now unnecessary)

> 227 + new_filter = self._modify_rules(current_lines,
> 228 + tables[table])

228 needs to be indented 1 space to the right.

> 238 + new_filter = filter(lambda l: binary_name not in l, current_lines)

Super-minor, but I'd lean against using a single letter variable name here. Aside from the fact that 'l' (ell) and '1' (one) look similar, I think it reads better if we use var name like 'line'. (Though, in most cases, I think 'i' and 'j' are fine for counting variables).

Of course this is personal preference, so, if you disagree here, feel free to disregard :)

> 245 + elif seen_chains == 1:

`seen_chains` is a boolean, so unless I'm misunderstanding, this should just be `elif seen_chains`. Related, perhaps that block should be rewritten as

  if seen_chains:
    <blah>
  else:
    <blah>

> 241 + for rules_index in range(len(new_filter)):

This might be clearer if rewritten as

  for rule_index, rule in enumerate(new_filter):

> + new_filter[rules_index:rules_index] = [str(rule) for rule in rules]

`rules_index` may end up uninitialized if the len(new_filter) == 0. We should probably handle that case even if it's unlikely (or even impossible) in practice.

review: Needs Fixing

[Todd Willey (xtoddx)]

So nova-local is now used for FORWARD and OUTPUT, whereas before it was only used for FORWARD, correct? Why the change?

Is use_nova_chains used anymore? It seems to have been replaced with the 'wrap' parameter. It may be prettier to have a helper function wrapped_chain('INPUT') or nova_chain('FORWARD') that we could use as the first parameter to add_rule, since we'd have the name in one spot, instead of having to look at two different parameters to determine what the actual chain name is. That also might let us get rid of the IptablesRule class and just store rules as tuples of (chain, rule).

I'm not quite done with this review, but I wanted to give you my early comments.

[Vish Ishaya (vishvananda)]

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

[Soren Hansen (soren)]

2011/2/22 Rick Harris <email address hidden>:
>> 245   +            elif seen_chains == 1:
> `seen_chains` is a boolean, so unless I'm misunderstanding, this should just be `elif seen_chains`.

Actually, it could just be rewritten as "else:" :) It's a leftover
from a point where seen_chains had 4 states (and was named
differently). Good catch, thanks.

Related, perhaps that block should be rewritten as

>
>  if seen_chains:
>    <blah>
>  else:
>    <blah>

I prefer it the way it is. It matches the order of the output from
iptables-save (which is first
a comment, then a specification of the table to which the following
output pertains, then the chains,
then the rules).

>> 241   +        for rules_index in range(len(new_filter)):
>
> This might be clearer if rewritten as
>
>  for rule_index, rule in enumerate(new_filter):

Done.

>> +        new_filter[rules_index:rules_index] = [str(rule) for rule in rules]
> `rules_index` may end up uninitialized if the len(new_filter) == 0. We should probably handle that case even if it's unlikely (or even impossible) in practice.

I now initialise rules_index to 0.

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

[Soren Hansen (soren)]

2011/2/22 Todd Willey <email address hidden>:
> So nova-local is now used for FORWARD and OUTPUT, whereas before it was only used for FORWARD, correct?  Why the change?

This is the original code that is being replaced:

858 - our_chains += [':nova-local - [0:0]']
859 - our_rules += ['-A FORWARD -j nova-local']
860 - our_rules += ['-A OUTPUT -j nova-local']

So no change.

> Is use_nova_chains used anymore?  It seems to have been replaced with the 'wrap' parameter.

True. I've removed the flag now.

>  It may be prettier to have a helper function wrapped_chain('INPUT') or nova_chain('FORWARD') that we could use as the first parameter to add_rule, since we'd have the name in one spot, instead of having to look at two different parameters to determine what the actual chain name is.

Why/when do you have to look in two places to work this out?

I really like that consumers of this IptablesManager need not worry
about the the naming at all, but can just define whatever name they
want, and IptablesManager ensures that it doesn't collide with
anything else.

>  That also might let us get rid of the IptablesRule class and just store rules as tuples of (chain, rule).

I started out that way, actually, but ended up rewriting it like this.
I found it more readable this way.

> I'm not quite done with this review, but I wanted to give you my early comments.

Np. Thanks!

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

[Soren Hansen (soren)]

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/

[Vish Ishaya (vishvananda)]

The rules seem to be working correctly now. One final suggestion:

The SNATTING chain is user created. I capitalized it originally because all of the default chains are capitalized. Now that we are using it along with a bunch of other custom chains (like floating-snat), i'm thinking for consistency it should be "snatting" or even "snat".

I'll go ahead and approve and leave it up to you if you want to change it.

review: Approve

[Soren Hansen (soren)]

I did consider that at one point, but I think I decided not to because I was
tired of adjusting the unit tests. ;) It's a good idea, though. I'll do
that once I'm back near my laptop.

[lp:~soren/nova/iptables-concurrency]

721. By Soren Hansen on 2011-02-22

Rename "SNATTING" chain to "snat".

[Christian Berendt (berendt)]

I tried spawning > 20 instances (hypervisor KVM) at the same time on one nova-compute node and I got 2 failed instances. I think the problem is, that there are only 15 (or 16?) available NBD devices at the same time.

(nova.compute.manager): TRACE: Error: No free nbd devices

So there has to be a hard limit somewhere while using KVM as hypervisor. Is there a flag to limit the number of spawning instances on one node at the same time? If not we should introduce one. Or is it possible to use more than 15 NBD devices at the same time?

I tested the code against rev 713 and I don't get the error described in #722477. So it seems working for me :) (but i didn't read the code)

[Christian Berendt (berendt)]

It's possible to load the nbd module using more devices

example: modprobe nbd nbds_max=32

[Soren Hansen (soren)]

Iirc, you can actually just mknod more devices, and the driver creates the
kernel structs on the fly.
Den 23/02/2011 13.29 skrev "Christian Berendt" <email address hidden>:
> It's possible to load the nbd module using more devices
>
> example: modprobe nbd nbds_max=32
> --
> https://code.launchpad.net/~soren/nova/iptables-concurrency/+merge/50597
> You are the owner of lp:~soren/nova/iptables-concurrency.

[Christian Berendt (berendt)]

> Iirc, you can actually just mknod more devices, and the driver creates the
> kernel structs on the fly.

Problem is, that the number of the devices is hardcoded to 16 in nova/virt/disk.py. But this is offtopic, I'll create a bug for this issue later...

[Christian Berendt (berendt)]

lgtm

review: Approve

[Soren Hansen (soren)]

I've spotted another concurrency problem that I might as well fix now..

[lp:~soren/nova/iptables-concurrency]

722. By Soren Hansen on 2011-02-28

Merge sync branch.

723. By Soren Hansen on 2011-02-28

Wrap IptablesManager.apply() calls in utils.synchronized to avoid having different workers step on each other's toes.

724. By Soren Hansen on 2011-02-28

Merge lock_path change.

725. By Soren Hansen on 2011-03-01

Merge sync branch.

726. By Soren Hansen on 2011-03-03

Merge sync branch and trunk

727. By Soren Hansen on 2011-03-03

Merge trunk

728. By Soren Hansen on 2011-03-07

Make iptables rules class __ne__ just be inverted __eq__.

729. By Soren Hansen on 2011-03-07

Log failed command execution if there are more retry attempts left.

730. By Soren Hansen on 2011-03-07

Use IptablesManager.semapahore from securitygroups driver to ensure we don't apply half a rule set.

731. By Soren Hansen on 2011-03-09

Merge trunk

732. By Soren Hansen on 2011-03-10

Merge trunk

733. By Soren Hansen on 2011-03-10

PEP8

Unmerged revisions

=== modified file 'nova/flags.py'
--- nova/flags.py	2011-02-25 01:04:25 +0000
+++ nova/flags.py	2011-02-28 22:36:03 +0000
@@ -321,6 +321,8 @@
 
 DEFINE_string('state_path', os.path.join(os.path.dirname(__file__), '../'),
               "Top-level directory for maintaining nova's state")
+DEFINE_string('lock_path', os.path.join(os.path.dirname(__file__), '../'),
+              "Directory for lock files")
 DEFINE_string('logdir', None, 'output to a per-service log file in named '
                               'directory')
 

=== modified file 'nova/network/linux_net.py'
--- nova/network/linux_net.py	2011-02-17 12:46:24 +0000
+++ nova/network/linux_net.py	2011-02-28 22:36:03 +0000
@@ -17,15 +17,17 @@
 Implements vlans, bridges, and iptables rules using linux utilities.
 """
 
+import inspect
 import os
 
+from eventlet import semaphore
+
 from nova import db
 from nova import exception
 from nova import flags
 from nova import log as logging
 from nova import utils
 
-
 LOG = logging.getLogger("nova.linux_net")
 
 
@@ -52,8 +54,6 @@
                         'location of nova-dhcpbridge')
 flags.DEFINE_string('routing_source_ip', '$my_ip',
                     'Public IP of network host')
-flags.DEFINE_bool('use_nova_chains', False,
-                  'use the nova_ routing chains instead of default')
 flags.DEFINE_string('input_chain', 'INPUT',
                     'chain to add nova_input to')
 
@@ -63,73 +63,334 @@
                     'dmz range that should be accepted')
 
 
+binary_name = os.path.basename(inspect.stack()[-1][1])
+
+
+class IptablesRule(object):
+    """An iptables rule
+
+    You shouldn't need to use this class directly, it's only used by
+    IptablesManager
+    """
+    def __init__(self, chain, rule, wrap=True, top=False):
+        self.chain = chain
+        self.rule = rule
+        self.wrap = wrap
+        self.top = top
+
+    def __eq__(self, other):
+        return ((self.chain == other.chain) and
+                (self.rule == other.rule) and
+                (self.top == other.top) and
+                (self.wrap == other.wrap))
+
+    def __ne__(self, other):
+        return ((self.chain != other.chain) or
+                (self.rule != other.rule) or
+                (self.top != other.top) or
+                (self.wrap != other.wrap))
+
+    def __str__(self):
+        if self.wrap:
+            chain = '%s-%s' % (binary_name, self.chain)
+        else:
+            chain = self.chain
+        return '-A %s %s' % (chain, self.rule)
+
+
+class IptablesTable(object):
+    """An iptables table"""
+
+    def __init__(self):
+        self.rules = []
+        self.chains = set()
+        self.unwrapped_chains = set()
+
+    def add_chain(self, name, wrap=True):
+        """Adds a named chain to the table
+
+        The chain name is wrapped to be unique for the component creating
+        it, so different components of Nova can safely create identically
+        named chains without interfering with one another.
+
+        At the moment, its wrapped name is <binary name>-<chain name>,
+        so if nova-compute creates a chain named "OUTPUT", it'll actually
+        end up named "nova-compute-OUTPUT".
+        """
+        if wrap:
+            self.chains.add(name)
+        else:
+            self.unwrapped_chains.add(name)
+
+    def remove_chain(self, name, wrap=True):
+        """Remove named chain
+
+        This removal "cascades". All rule in the chain are removed, as are
+        all rules in other chains that jump to it.
+
+        If the chain is not found, this is merely logged.
+        """
+        if wrap:
+            chain_set = self.chains
+        else:
+            chain_set = self.unwrapped_chains
+
+        if name not in chain_set:
+            LOG.debug(_("Attempted to remove chain %s which doesn't exist"),
+                      name)
+            return
+
+        chain_set.remove(name)
+        self.rules = filter(lambda r: r.chain != name, self.rules)
+
+        if wrap:
+            jump_snippet = '-j %s-%s' % (binary_name, name)
+        else:
+            jump_snippet = '-j %s' % (name,)
+
+        self.rules = filter(lambda r: jump_snippet not in r.rule, self.rules)
+
+    def add_rule(self, chain, rule, wrap=True, top=False):
+        """Add a rule to the table
+
+        This is just like what you'd feed to iptables, just without
+        the "-A <chain name>" bit at the start.
+
+        However, if you need to jump to one of your wrapped chains,
+        prepend its name with a '$' which will ensure the wrapping
+        is applied correctly.
+        """
+        if wrap and chain not in self.chains:
+            raise ValueError(_("Unknown chain: %r") % chain)
+
+        if '$' in rule:
+            rule = ' '.join(map(self._wrap_target_chain, rule.split(' ')))
+
+        self.rules.append(IptablesRule(chain, rule, wrap, top))
+
+    def _wrap_target_chain(self, s):
+        if s.startswith('$'):
+            return '%s-%s' % (binary_name, s[1:])
+        return s
+
+    def remove_rule(self, chain, rule, wrap=True, top=False):
+        """Remove a rule from a chain
+
+        Note: The rule must be exactly identical to the one that was added.
+        You cannot switch arguments around like you can with the iptables
+        CLI tool.
+        """
+        try:
+            self.rules.remove(IptablesRule(chain, rule, wrap, top))
+        except ValueError:
+            LOG.debug(_("Tried to remove rule that wasn't there:"
+                        " %(chain)r %(rule)r %(wrap)r %(top)r"),
+                      {'chain': chain, 'rule': rule,
+                       'top': top, 'wrap': wrap})
+
+
+class IptablesManager(object):
+    """Wrapper for iptables
+
+    See IptablesTable for some usage docs
+
+    A number of chains are set up to begin with.
+
+    First, nova-filter-top. It's added at the top of FORWARD and OUTPUT. Its
+    name is not wrapped, so it's shared between the various nova workers. It's
+    intended for rules that need to live at the top of the FORWARD and OUTPUT
+    chains. It's in both the ipv4 and ipv6 set of tables.
+
+    For ipv4 and ipv6, the builtin INPUT, OUTPUT, and FORWARD filter chains are
+    wrapped, meaning that the "real" INPUT chain has a rule that jumps to the
+    wrapped INPUT chain, etc. Additionally, there's a wrapped chain named
+    "local" which is jumped to from nova-filter-top.
+
+    For ipv4, the builtin PREROUTING, OUTPUT, and POSTROUTING nat chains are
+    wrapped in the same was as the builtin filter chains. Additionally, there's
+    a snat chain that is applied after the POSTROUTING chain.
+    """
+    def __init__(self, execute=None):
+        if not execute:
+            if FLAGS.fake_network:
+                self.execute = lambda *args, **kwargs: ('', '')
+            else:
+                self.execute = utils.execute
+        else:
+            self.execute = execute
+
+        self.ipv4 = {'filter': IptablesTable(),
+                     'nat': IptablesTable()}
+        self.ipv6 = {'filter': IptablesTable()}
+
+        # Add a nova-filter-top chain. It's intended to be shared
+        # among the various nova components. It sits at the very top
+        # of FORWARD and OUTPUT.
+        for tables in [self.ipv4, self.ipv6]:
+            tables['filter'].add_chain('nova-filter-top', wrap=False)
+            tables['filter'].add_rule('FORWARD', '-j nova-filter-top',
+                                      wrap=False, top=True)
+            tables['filter'].add_rule('OUTPUT', '-j nova-filter-top',
+                                      wrap=False, top=True)
+
+            tables['filter'].add_chain('local')
+            tables['filter'].add_rule('nova-filter-top', '-j $local',
+                                      wrap=False)
+
+        # Wrap the builtin chains
+        builtin_chains = {4: {'filter': ['INPUT', 'OUTPUT', 'FORWARD'],
+                              'nat': ['PREROUTING', 'OUTPUT', 'POSTROUTING']},
+                          6: {'filter': ['INPUT', 'OUTPUT', 'FORWARD']}}
+
+        for ip_version in builtin_chains:
+            if ip_version == 4:
+                tables = self.ipv4
+            elif ip_version == 6:
+                tables = self.ipv6
+
+            for table, chains in builtin_chains[ip_version].iteritems():
+                for chain in chains:
+                    tables[table].add_chain(chain)
+                    tables[table].add_rule(chain, '-j $%s' % (chain,),
+                                           wrap=False)
+
+        # Add a nova-postrouting-bottom chain. It's intended to be shared
+        # among the various nova components. We set it as the last chain
+        # of POSTROUTING chain.
+        self.ipv4['nat'].add_chain('nova-postrouting-bottom', wrap=False)
+        self.ipv4['nat'].add_rule('POSTROUTING', '-j nova-postrouting-bottom',
+                                  wrap=False)
+
+        # We add a snat chain to the shared nova-postrouting-bottom chain
+        # so that it's applied last.
+        self.ipv4['nat'].add_chain('snat')
+        self.ipv4['nat'].add_rule('nova-postrouting-bottom', '-j $snat',
+                                  wrap=False)
+
+        # And then we add a floating-snat chain and jump to first thing in
+        # the snat chain.
+        self.ipv4['nat'].add_chain('floating-snat')
+        self.ipv4['nat'].add_rule('snat', '-j $floating-snat')
+
+        self.semaphore = semaphore.Semaphore()
+
+    @utils.synchronized('iptables')
+    def apply(self):
+        """Apply the current in-memory set of iptables rules
+
+        This will blow away any rules left over from previous runs of the
+        same component of Nova, and replace them with our current set of
+        rules. This happens atomically, thanks to iptables-restore.
+
+        We wrap the call in a semaphore lock, so that we don't race with
+        ourselves. In the event of a race with another component running
+        an iptables-* command at the same time, we retry up to 5 times.
+        """
+        with self.semaphore:
+            s = [('iptables', self.ipv4)]
+            if FLAGS.use_ipv6:
+                s += [('ip6tables', self.ipv6)]
+
+            for cmd, tables in s:
+                for table in tables:
+                    current_table, _ = self.execute('sudo %s-save -t %s' %
+                                                    (cmd, table), attempts=5)
+                    current_lines = current_table.split('\n')
+                    new_filter = self._modify_rules(current_lines,
+                                                    tables[table])
+                    self.execute('sudo %s-restore' % (cmd,),
+                                 process_input='\n'.join(new_filter),
+                                 attempts=5)
+
+    def _modify_rules(self, current_lines, table, binary=None):
+        unwrapped_chains = table.unwrapped_chains
+        chains = table.chains
+        rules = table.rules
+
+        # Remove any trace of our rules
+        new_filter = filter(lambda line: binary_name not in line,
+                            current_lines)
+
+        seen_chains = False
+        rules_index = 0
+        for rules_index, rule in enumerate(new_filter):
+            if not seen_chains:
+                if rule.startswith(':'):
+                    seen_chains = True
+            else:
+                if not rule.startswith(':'):
+                    break
+
+        our_rules = []
+        for rule in rules:
+            rule_str = str(rule)
+            if rule.top:
+                # rule.top == True means we want this rule to be at the top.
+                # Further down, we weed out duplicates from the bottom of the
+                # list, so here we remove the dupes ahead of time.
+                new_filter = filter(lambda s: s.strip() != rule_str.strip(),
+                                    new_filter)
+            our_rules += [rule_str]
+
+        new_filter[rules_index:rules_index] = our_rules
+
+        new_filter[rules_index:rules_index] = [':%s - [0:0]' % \
+                                               (name,) \
+                                               for name in unwrapped_chains]
+        new_filter[rules_index:rules_index] = [':%s-%s - [0:0]' % \
+                                               (binary_name, name,) \
+                                               for name in chains]
+
+        seen_lines = set()
+
+        def _weed_out_duplicates(line):
+            line = line.strip()
+            if line in seen_lines:
+                return False
+            else:
+                seen_lines.add(line)
+                return True
+
+        # We filter duplicates, letting the *last* occurrence take
+        # precendence.
+        new_filter.reverse()
+        new_filter = filter(_weed_out_duplicates, new_filter)
+        new_filter.reverse()
+        return new_filter
+
+
+iptables_manager = IptablesManager()
+
+
 def metadata_forward():
     """Create forwarding rule for metadata"""
-    _confirm_rule("PREROUTING", "-t nat -s 0.0.0.0/0 "
-             "-d 169.254.169.254/32 -p tcp -m tcp --dport 80 -j DNAT "
-             "--to-destination %s:%s" % (FLAGS.ec2_dmz_host, FLAGS.ec2_port))
+    iptables_manager.ipv4['nat'].add_rule("PREROUTING",
+                                          "-s 0.0.0.0/0 -d 169.254.169.254/32 "
+                                          "-p tcp -m tcp --dport 80 -j DNAT "
+                                          "--to-destination %s:%s" % \
+                                          (FLAGS.ec2_dmz_host, FLAGS.ec2_port))
+    iptables_manager.apply()
 
 
 def init_host():
     """Basic networking setup goes here"""
 
-    if FLAGS.use_nova_chains:
-        _execute("sudo iptables -N nova_input", check_exit_code=False)
-        _execute("sudo iptables -D %s -j nova_input" % FLAGS.input_chain,
-                 check_exit_code=False)
-        _execute("sudo iptables -A %s -j nova_input" % FLAGS.input_chain)
-
-        _execute("sudo iptables -N nova_forward", check_exit_code=False)
-        _execute("sudo iptables -D FORWARD -j nova_forward",
-                 check_exit_code=False)
-        _execute("sudo iptables -A FORWARD -j nova_forward")
-
-        _execute("sudo iptables -N nova_output", check_exit_code=False)
-        _execute("sudo iptables -D OUTPUT -j nova_output",
-                 check_exit_code=False)
-        _execute("sudo iptables -A OUTPUT -j nova_output")
-
-        _execute("sudo iptables -t nat -N nova_prerouting",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -D PREROUTING -j nova_prerouting",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -A PREROUTING -j nova_prerouting")
-
-        _execute("sudo iptables -t nat -N nova_postrouting",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -D POSTROUTING -j nova_postrouting",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -A POSTROUTING -j nova_postrouting")
-
-        _execute("sudo iptables -t nat -N nova_snatting",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -D POSTROUTING -j nova_snatting",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -A POSTROUTING -j nova_snatting")
-
-        _execute("sudo iptables -t nat -N nova_output", check_exit_code=False)
-        _execute("sudo iptables -t nat -D OUTPUT -j nova_output",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -A OUTPUT -j nova_output")
-    else:
-        # NOTE(vish): This makes it easy to ensure snatting rules always
-        #             come after the accept rules in the postrouting chain
-        _execute("sudo iptables -t nat -N SNATTING",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -D POSTROUTING -j SNATTING",
-                 check_exit_code=False)
-        _execute("sudo iptables -t nat -A POSTROUTING -j SNATTING")
-
     # NOTE(devcamcar): Cloud public SNAT entries and the default
     # SNAT rule for outbound traffic.
-    _confirm_rule("SNATTING", "-t nat -s %s "
-             "-j SNAT --to-source %s"
-             % (FLAGS.fixed_range, FLAGS.routing_source_ip), append=True)
-
-    _confirm_rule("POSTROUTING", "-t nat -s %s -d %s -j ACCEPT" %
-                  (FLAGS.fixed_range, FLAGS.dmz_cidr))
-    _confirm_rule("POSTROUTING", "-t nat -s %(range)s -d %(range)s -j ACCEPT" %
-                  {'range': FLAGS.fixed_range})
+    iptables_manager.ipv4['nat'].add_rule("snat",
+                                          "-s %s -j SNAT --to-source %s" % \
+                                           (FLAGS.fixed_range,
+                                            FLAGS.routing_source_ip))
+
+    iptables_manager.ipv4['nat'].add_rule("POSTROUTING",
+                                          "-s %s -d %s -j ACCEPT" % \
+                                          (FLAGS.fixed_range, FLAGS.dmz_cidr))
+
+    iptables_manager.ipv4['nat'].add_rule("POSTROUTING",
+                                          "-s %(range)s -d %(range)s "
+                                          "-j ACCEPT" % \
+                                          {'range': FLAGS.fixed_range})
+    iptables_manager.apply()
 
 
 def bind_floating_ip(floating_ip, check_exit_code=True):
@@ -147,31 +408,36 @@
 
 def ensure_vlan_forward(public_ip, port, private_ip):
     """Sets up forwarding rules for vlan"""
-    _confirm_rule("FORWARD", "-d %s -p udp --dport 1194 -j ACCEPT" %
-                  private_ip)
-    _confirm_rule("PREROUTING",
-                  "-t nat -d %s -p udp --dport %s -j DNAT --to %s:1194"
-            % (public_ip, port, private_ip))
+    iptables_manager.ipv4['filter'].add_rule("FORWARD",
+                                             "-d %s -p udp "
+                                             "--dport 1194 "
+                                             "-j ACCEPT" % private_ip)
+    iptables_manager.ipv4['nat'].add_rule("PREROUTING",
+                                          "-d %s -p udp "
+                                          "--dport %s -j DNAT --to %s:1194" %
+                                          (public_ip, port, private_ip))
+    iptables_manager.apply()
 
 
 def ensure_floating_forward(floating_ip, fixed_ip):
     """Ensure floating ip forwarding rule"""
-    _confirm_rule("PREROUTING", "-t nat -d %s -j DNAT --to %s"
-                           % (floating_ip, fixed_ip))
-    _confirm_rule("OUTPUT", "-t nat -d %s -j DNAT --to %s"
-                           % (floating_ip, fixed_ip))
-    _confirm_rule("SNATTING", "-t nat -s %s -j SNAT --to %s"
-                           % (fixed_ip, floating_ip))
+    for chain, rule in floating_forward_rules(floating_ip, fixed_ip):
+        iptables_manager.ipv4['nat'].add_rule(chain, rule)
+    iptables_manager.apply()
 
 
 def remove_floating_forward(floating_ip, fixed_ip):
     """Remove forwarding for floating ip"""
-    _remove_rule("PREROUTING", "-t nat -d %s -j DNAT --to %s"
-                          % (floating_ip, fixed_ip))
-    _remove_rule("OUTPUT", "-t nat -d %s -j DNAT --to %s"
-                          % (floating_ip, fixed_ip))
-    _remove_rule("SNATTING", "-t nat -s %s -j SNAT --to %s"
-                          % (fixed_ip, floating_ip))
+    for chain, rule in floating_forward_rules(floating_ip, fixed_ip):
+        iptables_manager.ipv4['nat'].remove_rule(chain, rule)
+    iptables_manager.apply()
+
+
+def floating_forward_rules(floating_ip, fixed_ip):
+    return [("PREROUTING", "-d %s -j DNAT --to %s" % (floating_ip, fixed_ip)),
+            ("OUTPUT", "-d %s -j DNAT --to %s" % (floating_ip, fixed_ip)),
+            ("floating-snat",
+             "-s %s -j SNAT --to %s" % (fixed_ip, floating_ip))]
 
 
 def ensure_vlan_bridge(vlan_num, bridge, net_attrs=None):
@@ -258,19 +524,12 @@
                            "enslave it to bridge %s.\n" % (interface, bridge)):
             raise exception.Error("Failed to add interface: %s" % err)
 
-    if FLAGS.use_nova_chains:
-        (out, err) = _execute("sudo iptables -N nova_forward",
-                              check_exit_code=False)
-        if err != 'iptables: Chain already exists.\n':
-            # NOTE(vish): chain didn't exist link chain
-            _execute("sudo iptables -D FORWARD -j nova_forward",
-                     check_exit_code=False)
-            _execute("sudo iptables -A FORWARD -j nova_forward")
-
-    _confirm_rule("FORWARD", "--in-interface %s -j ACCEPT" % bridge)
-    _confirm_rule("FORWARD", "--out-interface %s -j ACCEPT" % bridge)
-    _execute("sudo iptables -N nova-local", check_exit_code=False)
-    _confirm_rule("FORWARD", "-j nova-local")
+    iptables_manager.ipv4['filter'].add_rule("FORWARD",
+                                             "--in-interface %s -j ACCEPT" % \
+                                             bridge)
+    iptables_manager.ipv4['filter'].add_rule("FORWARD",
+                                             "--out-interface %s -j ACCEPT" % \
+                                             bridge)
 
 
 def get_dhcp_hosts(context, network_id):
@@ -390,26 +649,6 @@
     return not err
 
 
-def _confirm_rule(chain, cmd, append=False):
-    """Delete and re-add iptables rule"""
-    if FLAGS.use_nova_chains:
-        chain = "nova_%s" % chain.lower()
-    if append:
-        loc = "-A"
-    else:
-        loc = "-I"
-    _execute("sudo iptables --delete %s %s" % (chain, cmd),
-             check_exit_code=False)
-    _execute("sudo iptables %s %s %s" % (loc, chain, cmd))
-
-
-def _remove_rule(chain, cmd):
-    """Remove iptables rule"""
-    if FLAGS.use_nova_chains:
-        chain = "%s" % chain.lower()
-    _execute("sudo iptables --delete %s %s" % (chain, cmd))
-
-
 def _dnsmasq_cmd(net):
     """Builds dnsmasq command"""
     cmd = ['sudo -E dnsmasq',

=== modified file 'nova/tests/test_misc.py'
--- nova/tests/test_misc.py	2011-02-21 18:55:25 +0000
+++ nova/tests/test_misc.py	2011-02-28 22:36:03 +0000
@@ -14,10 +14,14 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from datetime import datetime
+import errno
 import os
+import select
+import time
 
 from nova import test
-from nova.utils import parse_mailmap, str_dict_replace
+from nova.utils import parse_mailmap, str_dict_replace, synchronized
 
 
 class ProjectTestCase(test.TestCase):
@@ -55,3 +59,34 @@
                                 '%r not listed in Authors' % missing)
             finally:
                 tree.unlock()
+
+
+class LockTestCase(test.TestCase):
+    def test_synchronized(self):
+        rpipe, wpipe = os.pipe()
+        pid = os.fork()
+        if pid > 0:
+            os.close(wpipe)
+
+            @synchronized('testlock')
+            def f():
+                rfds, _, __ = select.select([rpipe], [], [], 1)
+                self.assertEquals(len(rfds), 0, "The other process, which was"
+                                                " supposed to be locked, "
+                                                "wrote on its end of the "
+                                                "pipe")
+                os.close(rpipe)
+
+            f()
+        else:
+            os.close(rpipe)
+
+            @synchronized('testlock')
+            def g():
+                try:
+                    os.write(wpipe, "foo")
+                except OSError, e:
+                    self.assertEquals(e.errno, errno.EPIPE)
+                    return
+            g()
+            os._exit(0)

=== modified file 'nova/tests/test_network.py'
--- nova/tests/test_network.py	2011-02-23 19:20:07 +0000
+++ nova/tests/test_network.py	2011-02-28 22:36:03 +0000
@@ -29,11 +29,153 @@
 from nova import test
 from nova import utils
 from nova.auth import manager
+from nova.network import linux_net
 
 FLAGS = flags.FLAGS
 LOG = logging.getLogger('nova.tests.network')
 
 
+class IptablesManagerTestCase(test.TestCase):
+    sample_filter = ['#Generated by iptables-save on Fri Feb 18 15:17:05 2011',
+                     '*filter',
+                     ':INPUT ACCEPT [2223527:305688874]',
+                     ':FORWARD ACCEPT [0:0]',
+                     ':OUTPUT ACCEPT [2172501:140856656]',
+                     ':nova-compute-FORWARD - [0:0]',
+                     ':nova-compute-INPUT - [0:0]',
+                     ':nova-compute-local - [0:0]',
+                     ':nova-compute-OUTPUT - [0:0]',
+                     ':nova-filter-top - [0:0]',
+                     '-A FORWARD -j nova-filter-top ',
+                     '-A OUTPUT -j nova-filter-top ',
+                     '-A nova-filter-top -j nova-compute-local ',
+                     '-A INPUT -j nova-compute-INPUT ',
+                     '-A OUTPUT -j nova-compute-OUTPUT ',
+                     '-A FORWARD -j nova-compute-FORWARD ',
+                     '-A INPUT -i virbr0 -p udp -m udp --dport 53 -j ACCEPT ',
+                     '-A INPUT -i virbr0 -p tcp -m tcp --dport 53 -j ACCEPT ',
+                     '-A INPUT -i virbr0 -p udp -m udp --dport 67 -j ACCEPT ',
+                     '-A INPUT -i virbr0 -p tcp -m tcp --dport 67 -j ACCEPT ',
+                     '-A FORWARD -s 192.168.122.0/24 -i virbr0 -j ACCEPT ',
+                     '-A FORWARD -i virbr0 -o virbr0 -j ACCEPT ',
+                     '-A FORWARD -o virbr0 -j REJECT --reject-with '
+                     'icmp-port-unreachable ',
+                     '-A FORWARD -i virbr0 -j REJECT --reject-with '
+                     'icmp-port-unreachable ',
+                     'COMMIT',
+                     '# Completed on Fri Feb 18 15:17:05 2011']
+
+    sample_nat = ['# Generated by iptables-save on Fri Feb 18 15:17:05 2011',
+                  '*nat',
+                  ':PREROUTING ACCEPT [3936:762355]',
+                  ':INPUT ACCEPT [2447:225266]',
+                  ':OUTPUT ACCEPT [63491:4191863]',
+                  ':POSTROUTING ACCEPT [63112:4108641]',
+                  ':nova-compute-OUTPUT - [0:0]',
+                  ':nova-compute-floating-ip-snat - [0:0]',
+                  ':nova-compute-SNATTING - [0:0]',
+                  ':nova-compute-PREROUTING - [0:0]',
+                  ':nova-compute-POSTROUTING - [0:0]',
+                  ':nova-postrouting-bottom - [0:0]',
+                  '-A PREROUTING -j nova-compute-PREROUTING ',
+                  '-A OUTPUT -j nova-compute-OUTPUT ',
+                  '-A POSTROUTING -j nova-compute-POSTROUTING ',
+                  '-A POSTROUTING -j nova-postrouting-bottom ',
+                  '-A nova-postrouting-bottom -j nova-compute-SNATTING ',
+                  '-A nova-compute-SNATTING -j nova-compute-floating-ip-snat ',
+                  'COMMIT',
+                  '# Completed on Fri Feb 18 15:17:05 2011']
+
+    def setUp(self):
+        super(IptablesManagerTestCase, self).setUp()
+        self.manager = linux_net.IptablesManager()
+
+    def test_filter_rules_are_wrapped(self):
+        current_lines = self.sample_filter
+
+        table = self.manager.ipv4['filter']
+        table.add_rule('FORWARD', '-s 1.2.3.4/5 -j DROP')
+        new_lines = self.manager._modify_rules(current_lines, table)
+        self.assertTrue('-A run_tests.py-FORWARD '
+                        '-s 1.2.3.4/5 -j DROP' in new_lines)
+
+        table.remove_rule('FORWARD', '-s 1.2.3.4/5 -j DROP')
+        new_lines = self.manager._modify_rules(current_lines, table)
+        self.assertTrue('-A run_tests.py-FORWARD '
+                        '-s 1.2.3.4/5 -j DROP' not in new_lines)
+
+    def test_nat_rules(self):
+        current_lines = self.sample_nat
+        new_lines = self.manager._modify_rules(current_lines,
+                                               self.manager.ipv4['nat'])
+
+        for line in [':nova-compute-OUTPUT - [0:0]',
+                     ':nova-compute-floating-ip-snat - [0:0]',
+                     ':nova-compute-SNATTING - [0:0]',
+                     ':nova-compute-PREROUTING - [0:0]',
+                     ':nova-compute-POSTROUTING - [0:0]']:
+            self.assertTrue(line in new_lines, "One of nova-compute's chains "
+                                               "went missing.")
+
+        seen_lines = set()
+        for line in new_lines:
+            line = line.strip()
+            self.assertTrue(line not in seen_lines,
+                            "Duplicate line: %s" % line)
+            seen_lines.add(line)
+
+        last_postrouting_line = ''
+
+        for line in new_lines:
+            if line.startswith('-A POSTROUTING'):
+                last_postrouting_line = line
+
+        self.assertTrue('-j nova-postrouting-bottom' in last_postrouting_line,
+                        "Last POSTROUTING rule does not jump to "
+                        "nova-postouting-bottom: %s" % last_postrouting_line)
+
+        for chain in ['POSTROUTING', 'PREROUTING', 'OUTPUT']:
+            self.assertTrue('-A %s -j run_tests.py-%s' \
+                            % (chain, chain) in new_lines,
+                            "Built-in chain %s not wrapped" % (chain,))
+
+    def test_filter_rules(self):
+        current_lines = self.sample_filter
+        new_lines = self.manager._modify_rules(current_lines,
+                                               self.manager.ipv4['filter'])
+
+        for line in [':nova-compute-FORWARD - [0:0]',
+                     ':nova-compute-INPUT - [0:0]',
+                     ':nova-compute-local - [0:0]',
+                     ':nova-compute-OUTPUT - [0:0]']:
+            self.assertTrue(line in new_lines, "One of nova-compute's chains"
+                                               " went missing.")
+
+        seen_lines = set()
+        for line in new_lines:
+            line = line.strip()
+            self.assertTrue(line not in seen_lines,
+                            "Duplicate line: %s" % line)
+            seen_lines.add(line)
+
+        for chain in ['FORWARD', 'OUTPUT']:
+            for line in new_lines:
+                if line.startswith('-A %s' % chain):
+                    self.assertTrue('-j nova-filter-top' in line,
+                                    "First %s rule does not "
+                                    "jump to nova-filter-top" % chain)
+                    break
+
+        self.assertTrue('-A nova-filter-top '
+                        '-j run_tests.py-local' in new_lines,
+                        "nova-filter-top does not jump to wrapped local chain")
+
+        for chain in ['INPUT', 'OUTPUT', 'FORWARD']:
+            self.assertTrue('-A %s -j run_tests.py-%s' \
+                            % (chain, chain) in new_lines,
+                            "Built-in chain %s not wrapped" % (chain,))
+
+
 class NetworkTestCase(test.TestCase):
     """Test cases for network code"""
     def setUp(self):

=== modified file 'nova/tests/test_virt.py'
--- nova/tests/test_virt.py	2011-02-23 19:56:37 +0000
+++ nova/tests/test_virt.py	2011-02-28 22:36:03 +0000
@@ -14,6 +14,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import re
 from xml.etree.ElementTree import fromstring as xml_to_tree
 from xml.dom.minidom import parseString as xml_to_dom
 
@@ -234,16 +235,22 @@
         self.manager.delete_user(self.user)
         super(IptablesFirewallTestCase, self).tearDown()
 
-    in_rules = [
+    in_nat_rules = [
+      '# Generated by iptables-save v1.4.10 on Sat Feb 19 00:03:19 2011',
+      '*nat',
+      ':PREROUTING ACCEPT [1170:189210]',
+      ':INPUT ACCEPT [844:71028]',
+      ':OUTPUT ACCEPT [5149:405186]',
+      ':POSTROUTING ACCEPT [5063:386098]'
+    ]
+
+    in_filter_rules = [
       '# Generated by iptables-save v1.4.4 on Mon Dec  6 11:54:13 2010',
       '*filter',
       ':INPUT ACCEPT [969615:281627771]',
       ':FORWARD ACCEPT [0:0]',
       ':OUTPUT ACCEPT [915599:63811649]',
       ':nova-block-ipv4 - [0:0]',
-      '-A INPUT -i virbr0 -p udp -m udp --dport 53 -j ACCEPT ',
-      '-A INPUT -i virbr0 -p tcp -m tcp --dport 53 -j ACCEPT ',
-      '-A INPUT -i virbr0 -p udp -m udp --dport 67 -j ACCEPT ',
       '-A INPUT -i virbr0 -p tcp -m tcp --dport 67 -j ACCEPT ',
       '-A FORWARD -d 192.168.122.0/24 -o virbr0 -m state --state RELATED'
       ',ESTABLISHED -j ACCEPT ',
@@ -255,7 +262,7 @@
       '# Completed on Mon Dec  6 11:54:13 2010',
     ]
 
-    in6_rules = [
+    in6_filter_rules = [
       '# Generated by ip6tables-save v1.4.4 on Tue Jan 18 23:47:56 2011',
       '*filter',
       ':INPUT ACCEPT [349155:75810423]',
@@ -315,23 +322,32 @@
         instance_ref = db.instance_get(admin_ctxt, instance_ref['id'])
 
 #        self.fw.add_instance(instance_ref)
-        def fake_iptables_execute(cmd, process_input=None):
+        def fake_iptables_execute(cmd, process_input=None, attempts=5):
             if cmd == 'sudo ip6tables-save -t filter':
-                return '\n'.join(self.in6_rules), None
+                return '\n'.join(self.in6_filter_rules), None
             if cmd == 'sudo iptables-save -t filter':
-                return '\n'.join(self.in_rules), None
+                return '\n'.join(self.in_filter_rules), None
+            if cmd == 'sudo iptables-save -t nat':
+                return '\n'.join(self.in_nat_rules), None
             if cmd == 'sudo iptables-restore':
-                self.out_rules = process_input.split('\n')
+                lines = process_input.split('\n')
+                if '*filter' in lines:
+                    self.out_rules = lines
                 return '', ''
             if cmd == 'sudo ip6tables-restore':
-                self.out6_rules = process_input.split('\n')
+                lines = process_input.split('\n')
+                if '*filter' in lines:
+                    self.out6_rules = lines
                 return '', ''
-        self.fw.execute = fake_iptables_execute
+
+        from nova.network import linux_net
+        linux_net.iptables_manager.execute = fake_iptables_execute
 
         self.fw.prepare_instance_filter(instance_ref)
         self.fw.apply_instance_filter(instance_ref)
 
-        in_rules = filter(lambda l: not l.startswith('#'), self.in_rules)
+        in_rules = filter(lambda l: not l.startswith('#'),
+                          self.in_filter_rules)
         for rule in in_rules:
             if not 'nova' in rule:
                 self.assertTrue(rule in self.out_rules,
@@ -354,17 +370,18 @@
         self.assertTrue(security_group_chain,
                         "The security group chain wasn't added")
 
-        self.assertTrue('-A %s -p icmp -s 192.168.11.0/24 -j ACCEPT' % \
-                               security_group_chain in self.out_rules,
+        regex = re.compile('-A .* -p icmp -s 192.168.11.0/24 -j ACCEPT')
+        self.assertTrue(len(filter(regex.match, self.out_rules)) > 0,
                         "ICMP acceptance rule wasn't added")
 
-        self.assertTrue('-A %s -p icmp -s 192.168.11.0/24 -m icmp --icmp-type '
-                        '8 -j ACCEPT' % security_group_chain in self.out_rules,
+        regex = re.compile('-A .* -p icmp -s 192.168.11.0/24 -m icmp '
+                           '--icmp-type 8 -j ACCEPT')
+        self.assertTrue(len(filter(regex.match, self.out_rules)) > 0,
                         "ICMP Echo Request acceptance rule wasn't added")
 
-        self.assertTrue('-A %s -p tcp -s 192.168.10.0/24 -m multiport '
-                        '--dports 80:81 -j ACCEPT' % security_group_chain \
-                            in self.out_rules,
+        regex = re.compile('-A .* -p tcp -s 192.168.10.0/24 -m multiport '
+                           '--dports 80:81 -j ACCEPT')
+        self.assertTrue(len(filter(regex.match, self.out_rules)) > 0,
                         "TCP port 80/81 acceptance rule wasn't added")
         db.instance_destroy(admin_ctxt, instance_ref['id'])
 

=== modified file 'nova/utils.py'
--- nova/utils.py	2011-02-23 22:50:33 +0000
+++ nova/utils.py	2011-02-28 22:36:03 +0000
@@ -25,6 +25,7 @@
 import datetime
 import inspect
 import json
+import lockfile
 import os
 import random
 import socket
@@ -43,11 +44,13 @@
 
 from nova import exception
 from nova.exception import ProcessExecutionError
+from nova import flags
 from nova import log as logging
 
 
 LOG = logging.getLogger("nova.utils")
 TIME_FORMAT = "%Y-%m-%dT%H:%M:%SZ"
+FLAGS = flags.FLAGS
 
 
 def import_class(import_str):
@@ -128,32 +131,41 @@
     execute("curl --fail %s -o %s" % (url, target))
 
 
-def execute(cmd, process_input=None, addl_env=None, check_exit_code=True):
-    LOG.debug(_("Running cmd (subprocess): %s"), cmd)
-    env = os.environ.copy()
-    if addl_env:
-        env.update(addl_env)
-    obj = subprocess.Popen(cmd, shell=True, stdin=subprocess.PIPE,
-        stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env)
-    result = None
-    if process_input != None:
-        result = obj.communicate(process_input)
-    else:
-        result = obj.communicate()
-    obj.stdin.close()
-    if obj.returncode:
-        LOG.debug(_("Result was %s") % obj.returncode)
-        if check_exit_code and obj.returncode != 0:
-            (stdout, stderr) = result
-            raise ProcessExecutionError(exit_code=obj.returncode,
-                                        stdout=stdout,
-                                        stderr=stderr,
-                                        cmd=cmd)
-    # NOTE(termie): this appears to be necessary to let the subprocess call
-    #               clean something up in between calls, without it two
-    #               execute calls in a row hangs the second one
-    greenthread.sleep(0)
-    return result
+def execute(cmd, process_input=None, addl_env=None, check_exit_code=True,
+            attempts=1):
+    while attempts > 0:
+        attempts -= 1
+        try:
+            LOG.debug(_("Running cmd (subprocess): %s"), cmd)
+            env = os.environ.copy()
+            if addl_env:
+                env.update(addl_env)
+            obj = subprocess.Popen(cmd, shell=True, stdin=subprocess.PIPE,
+                stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env)
+            result = None
+            if process_input != None:
+                result = obj.communicate(process_input)
+            else:
+                result = obj.communicate()
+            obj.stdin.close()
+            if obj.returncode:
+                LOG.debug(_("Result was %s") % obj.returncode)
+                if check_exit_code and obj.returncode != 0:
+                    (stdout, stderr) = result
+                    raise ProcessExecutionError(exit_code=obj.returncode,
+                                                stdout=stdout,
+                                                stderr=stderr,
+                                                cmd=cmd)
+            # NOTE(termie): this appears to be necessary to let the subprocess
+            #               call clean something up in between calls, without
+            #               it two execute calls in a row hangs the second one
+            greenthread.sleep(0)
+            return result
+        except ProcessExecutionError:
+            if not attempts:
+                raise
+            else:
+                greenthread.sleep(random.randint(20, 200) / 100.0)
 
 
 def ssh_execute(ssh, cmd, process_input=None,
@@ -491,6 +503,17 @@
     return json.loads(s)
 
 
+def synchronized(name):
+    def wrap(f):
+        def inner(*args, **kwargs):
+            lock = lockfile.FileLock(os.path.join(FLAGS.lock_path,
+                                                  'nova-%s.lock' % name))
+            with lock:
+                return f(*args, **kwargs)
+        return inner
+    return wrap
+
+
 def ensure_b64_encoding(val):
     """Safety method to ensure that values expected to be base64-encoded
     actually are. If they are, the value is returned unchanged. Otherwise,

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-01-27 23:58:22 +0000
+++ nova/virt/libvirt_conn.py	2011-02-28 22:36:03 +0000
@@ -44,8 +44,6 @@
 from xml.dom import minidom
 
 
-from eventlet import greenthread
-from eventlet import event
 from eventlet import tpool
 
 import IPy
@@ -56,7 +54,6 @@
 from nova import flags
 from nova import log as logging
 from nova import utils
-#from nova.api import context
 from nova.auth import manager
 from nova.compute import instance_types
 from nova.compute import power_state
@@ -1206,10 +1203,14 @@
 
 class IptablesFirewallDriver(FirewallDriver):
     def __init__(self, execute=None, **kwargs):
-        self.execute = execute or utils.execute
+        from nova.network import linux_net
+        self.iptables = linux_net.iptables_manager
         self.instances = {}
         self.nwfilter = NWFilterFirewall(kwargs['get_connection'])
 
+        self.iptables.ipv4['filter'].add_chain('sg-fallback')
+        self.iptables.ipv4['filter'].add_rule('sg-fallback', '-j DROP')
+
     def setup_basic_filtering(self, instance):
         """Use NWFilter from libvirt for this."""
         return self.nwfilter.setup_basic_filtering(instance)
@@ -1218,127 +1219,98 @@
         """No-op. Everything is done in prepare_instance_filter"""
         pass
 
-    def remove_instance(self, instance):
+    def unfilter_instance(self, instance):
         if instance['id'] in self.instances:
             del self.instances[instance['id']]
+            self.remove_filters_for_instance(instance)
+            self.iptables.apply()
         else:
             LOG.info(_('Attempted to unfilter instance %s which is not '
                        'filtered'), instance['id'])
 
-    def add_instance(self, instance):
+    def prepare_instance_filter(self, instance):
         self.instances[instance['id']] = instance
-
-    def unfilter_instance(self, instance):
-        self.remove_instance(instance)
-        self.apply_ruleset()
-
-    def prepare_instance_filter(self, instance):
-        self.add_instance(instance)
-        self.apply_ruleset()
-
-    def apply_ruleset(self):
-        current_filter, _ = self.execute('sudo iptables-save -t filter')
-        current_lines = current_filter.split('\n')
-        new_filter = self.modify_rules(current_lines, 4)
-        self.execute('sudo iptables-restore',
-                     process_input='\n'.join(new_filter))
-        if(FLAGS.use_ipv6):
-            current_filter, _ = self.execute('sudo ip6tables-save -t filter')
-            current_lines = current_filter.split('\n')
-            new_filter = self.modify_rules(current_lines, 6)
-            self.execute('sudo ip6tables-restore',
-                         process_input='\n'.join(new_filter))
-
-    def modify_rules(self, current_lines, ip_version=4):
+        self.add_filters_for_instance(instance)
+        self.iptables.apply()
+
+    def add_filters_for_instance(self, instance):
+        chain_name = self._instance_chain_name(instance)
+
+        self.iptables.ipv4['filter'].add_chain(chain_name)
+        ipv4_address = self._ip_for_instance(instance)
+        self.iptables.ipv4['filter'].add_rule('local',
+                                              '-d %s -j $%s' %
+                                              (ipv4_address, chain_name))
+
+        if FLAGS.use_ipv6:
+            self.iptables.ipv6['filter'].add_chain(chain_name)
+            ipv6_address = self._ip_for_instance_v6(instance)
+            self.iptables.ipv6['filter'].add_rule('local',
+                                                  '-d %s -j $%s' %
+                                                  (ipv6_address,
+                                                   chain_name))
+
+        ipv4_rules, ipv6_rules = self.instance_rules(instance)
+
+        for rule in ipv4_rules:
+            self.iptables.ipv4['filter'].add_rule(chain_name, rule)
+
+        if FLAGS.use_ipv6:
+            for rule in ipv6_rules:
+                self.iptables.ipv6['filter'].add_rule(chain_name, rule)
+
+    def remove_filters_for_instance(self, instance):
+        chain_name = self._instance_chain_name(instance)
+
+        self.iptables.ipv4['filter'].remove_chain(chain_name)
+        if FLAGS.use_ipv6:
+            self.iptables.ipv6['filter'].remove_chain(chain_name)
+
+    def instance_rules(self, instance):
         ctxt = context.get_admin_context()
-        # Remove any trace of nova rules.
-        new_filter = filter(lambda l: 'nova-' not in l, current_lines)
-
-        seen_chains = False
-        for rules_index in range(len(new_filter)):
-            if not seen_chains:
-                if new_filter[rules_index].startswith(':'):
-                    seen_chains = True
-            elif seen_chains == 1:
-                if not new_filter[rules_index].startswith(':'):
-                    break
-
-        our_chains = [':nova-fallback - [0:0]']
-        our_rules = ['-A nova-fallback -j DROP']
-
-        our_chains += [':nova-local - [0:0]']
-        our_rules += ['-A FORWARD -j nova-local']
-        our_rules += ['-A OUTPUT -j nova-local']
-
-        security_groups = {}
-        # Add our chains
-        # First, we add instance chains and rules
-        for instance_id in self.instances:
-            instance = self.instances[instance_id]
-            chain_name = self._instance_chain_name(instance)
-            if(ip_version == 4):
-                ip_address = self._ip_for_instance(instance)
-            elif(ip_version == 6):
-                ip_address = self._ip_for_instance_v6(instance)
-
-            our_chains += [':%s - [0:0]' % chain_name]
-
-            # Jump to the per-instance chain
-            our_rules += ['-A nova-local -d %s -j %s' % (ip_address,
-                                                         chain_name)]
-
-            # Always drop invalid packets
-            our_rules += ['-A %s -m state --state '
-                          'INVALID -j DROP' % (chain_name,)]
-
-            # Allow established connections
-            our_rules += ['-A %s -m state --state '
-                          'ESTABLISHED,RELATED -j ACCEPT' % (chain_name,)]
-
-            # Jump to each security group chain in turn
-            for security_group in \
-                            db.security_group_get_by_instance(ctxt,
-                                                              instance['id']):
-                security_groups[security_group['id']] = security_group
-
-                sg_chain_name = self._security_group_chain_name(
+
+        ipv4_rules = []
+        ipv6_rules = []
+
+        # Always drop invalid packets
+        ipv4_rules += ['-m state --state ' 'INVALID -j DROP']
+        ipv6_rules += ['-m state --state ' 'INVALID -j DROP']
+
+        # Allow established connections
+        ipv4_rules += ['-m state --state ESTABLISHED,RELATED -j ACCEPT']
+        ipv6_rules += ['-m state --state ESTABLISHED,RELATED -j ACCEPT']
+
+        dhcp_server = self._dhcp_server_for_instance(instance)
+        ipv4_rules += ['-s %s -p udp --sport 67 --dport 68 '
+                       '-j ACCEPT' % (dhcp_server,)]
+
+        #Allow project network traffic
+        if FLAGS.allow_project_net_traffic:
+            cidr = self._project_cidr_for_instance(instance)
+            ipv4_rules += ['-s %s -j ACCEPT' % (cidr,)]
+
+        # We wrap these in FLAGS.use_ipv6 because they might cause
+        # a DB lookup. The other ones are just list operations, so
+        # they're not worth the clutter.
+        if FLAGS.use_ipv6:
+            # Allow RA responses
+            ra_server = self._ra_server_for_instance(instance)
+            if ra_server:
+                ipv6_rules += ['-s %s/128 -p icmpv6 -j ACCEPT' % (ra_server,)]
+
+            #Allow project network traffic
+            if FLAGS.allow_project_net_traffic:
+                cidrv6 = self._project_cidrv6_for_instance(instance)
+                ipv6_rules += ['-s %s -j ACCEPT' % (cidrv6,)]
+
+        security_groups = db.security_group_get_by_instance(ctxt,
+                                                            instance['id'])
+
+        # then, security group chains and rules
+        for security_group in security_groups:
+            rules = db.security_group_rule_get_by_security_group(ctxt,
                                                           security_group['id'])
 
-                our_rules += ['-A %s -j %s' % (chain_name, sg_chain_name)]
-
-            if(ip_version == 4):
-                # Allow DHCP responses
-                dhcp_server = self._dhcp_server_for_instance(instance)
-                our_rules += ['-A %s -s %s -p udp --sport 67 --dport 68 '
-                                    '-j ACCEPT ' % (chain_name, dhcp_server)]
-                #Allow project network traffic
-                if (FLAGS.allow_project_net_traffic):
-                    cidr = self._project_cidr_for_instance(instance)
-                    our_rules += ['-A %s -s %s -j ACCEPT' % (chain_name, cidr)]
-            elif(ip_version == 6):
-                # Allow RA responses
-                ra_server = self._ra_server_for_instance(instance)
-                if ra_server:
-                    our_rules += ['-A %s -s %s -p icmpv6 -j ACCEPT' %
-                                  (chain_name, ra_server + "/128")]
-                #Allow project network traffic
-                if (FLAGS.allow_project_net_traffic):
-                    cidrv6 = self._project_cidrv6_for_instance(instance)
-                    our_rules += ['-A %s -s %s -j ACCEPT' %
-                                        (chain_name, cidrv6)]
-
-            # If nothing matches, jump to the fallback chain
-            our_rules += ['-A %s -j nova-fallback' % (chain_name,)]
-
-        # then, security group chains and rules
-        for security_group_id in security_groups:
-            chain_name = self._security_group_chain_name(security_group_id)
-            our_chains += [':%s - [0:0]' % chain_name]
-
-            rules = \
-              db.security_group_rule_get_by_security_group(ctxt,
-                                                          security_group_id)
-
             for rule in rules:
                 logging.info('%r', rule)
 
@@ -1348,14 +1320,16 @@
                     continue
 
                 version = _get_ip_version(rule.cidr)
-                if version != ip_version:
-                    continue
+                if version == 4:
+                    rules = ipv4_rules
+                else:
+                    rules = ipv6_rules
 
                 protocol = rule.protocol
                 if version == 6 and rule.protocol == 'icmp':
                     protocol = 'icmpv6'
 
-                args = ['-A', chain_name, '-p', protocol, '-s', rule.cidr]
+                args = ['-p', protocol, '-s', rule.cidr]
 
                 if rule.protocol in ['udp', 'tcp']:
                     if rule.from_port == rule.to_port:
@@ -1376,32 +1350,35 @@
                             icmp_type_arg += '/%s' % icmp_code
 
                     if icmp_type_arg:
-                        if(ip_version == 4):
+                        if version == 4:
                             args += ['-m', 'icmp', '--icmp-type',
                                      icmp_type_arg]
-                        elif(ip_version == 6):
+                        elif version == 6:
                             args += ['-m', 'icmp6', '--icmpv6-type',
                                      icmp_type_arg]
 
                 args += ['-j ACCEPT']
-                our_rules += [' '.join(args)]
-
-        new_filter[rules_index:rules_index] = our_rules
-        new_filter[rules_index:rules_index] = our_chains
-        logging.info('new_filter: %s', '\n'.join(new_filter))
-        return new_filter
+                rules += [' '.join(args)]
+
+        ipv4_rules += ['-j $sg-fallback']
+        ipv6_rules += ['-j $sg-fallback']
+
+        return ipv4_rules, ipv6_rules
 
     def refresh_security_group_members(self, security_group):
         pass
 
     def refresh_security_group_rules(self, security_group):
-        self.apply_ruleset()
+        for instance in self.instances.values():
+            self.remove_filters_for_instance(instance)
+            self.add_filters_for_instance(instance)
+        self.iptables.apply()
 
     def _security_group_chain_name(self, security_group_id):
         return 'nova-sg-%s' % (security_group_id,)
 
     def _instance_chain_name(self, instance):
-        return 'nova-inst-%s' % (instance['id'],)
+        return 'inst-%s' % (instance['id'],)
 
     def _ip_for_instance(self, instance):
         return db.instance_get_fixed_address(context.get_admin_context(),