Merge pull request #3486 from anarkiwi/dps

Fix changes to DP level only config are not detected.

clib/mininet_test_base.py

@@ -2080,6 +2080,7 @@ def verify_faucet_reconf(self, timeout=10,
             self.assertEqual(
                 old_count, new_count,
                 msg='%s incremented: %u' % (var, new_count))
+        self.wait_for_prometheus_var('faucet_config_applied', 1, dpid=None, timeout=30)
         self.wait_dp_status(1)
 
     def force_faucet_reload(self, new_config):

faucet/conf.py

@@ -24,7 +24,6 @@
 
 class InvalidConfigError(Exception):
     """This error is thrown when the config file is not valid."""
-    pass
 
 
 def test_config_condition(cond, msg):
@@ -37,8 +36,8 @@ class Conf:
     """Base class for FAUCET configuration."""
 
     mutable_attrs = frozenset() # type: frozenset
-    defaults = None # type: dict
-    defaults_types = None # type: dict
+    defaults = {}  # type: dict
+    defaults_types = {}  # type: dict
     dyn_finalized = False
     dyn_hash = None
 
@@ -55,6 +54,7 @@ def __init__(self, _id, dp_id, conf=None):
             self.update(conf)
             self.set_defaults()
         self.check_config()
+        self.orig_conf = {k: self.__dict__[k] for k in self.defaults}
 
     def __setattr__(self, name, value):
         if not self.dyn_finalized or name.startswith('dyn') or name in self.mutable_attrs:
@@ -116,21 +116,22 @@ def update(self, conf):
         self._check_unknown_conf(conf)
         self._check_conf_types(conf, self.defaults_types)
 
-    def check_config(self):
+    @staticmethod
+    def check_config():
         """Check config at instantiation time for errors, typically via assert."""
-        pass
+        return
 
-    @staticmethod
-    def _conf_keys(conf, dyn=False, subconf=True, ignore_keys=None):
+    def _conf_keys(self, conf, subconf=True, ignore_keys=None):
         """Return a list of key/values of attributes with dyn/Conf attributes/filtered."""
         conf_keys = []
-        for key, value in sorted(conf.__dict__.items()):
-            if not dyn and key.startswith('dyn'):
-                continue
-            if not subconf and isinstance(value, Conf):
-                continue
+        for key, value in sorted(((key, value) for key, value in conf.orig_conf.items() if key in self.defaults)):
             if ignore_keys and key in ignore_keys:
                 continue
+            if not subconf and value:
+                if isinstance(value, Conf):
+                    continue
+                if isinstance(value, (tuple, list, set)) and isinstance(value[0], Conf):
+                    continue
             conf_keys.append((key, value))
         return conf_keys
 
@@ -162,7 +163,7 @@ def _str_conf(self, conf_v):
     def to_conf(self):
         """Return configuration as a dict."""
         conf = {
-            k: self.__dict__[str(k)] for k in self.defaults.keys() if k != 'name'}
+            k: self.orig_conf[str(k)] for k in self.defaults if k != 'name'}
         return json.dumps(self._str_conf(conf), sort_keys=True, indent=4, separators=(',', ': '))
 
     def conf_diff(self, other):
@@ -171,15 +172,15 @@ def conf_diff(self, other):
         return '\n'.join(differ.compare(
             self.to_conf().splitlines(), other.to_conf().splitlines()))
 
-    def conf_hash(self, dyn=False, subconf=True, ignore_keys=None):
+    def conf_hash(self, subconf=True, ignore_keys=None):
         """Return hash of keys configurably filtering attributes."""
         return hash(frozenset(list(map(
-            str, self._conf_keys(self, dyn=dyn, subconf=subconf, ignore_keys=ignore_keys)))))
+            str, self._conf_keys(self, subconf=subconf, ignore_keys=ignore_keys)))))
 
     def __hash__(self):
         if self.dyn_hash is not None:
             return self.dyn_hash
-        dyn_hash = self.conf_hash(dyn=False, subconf=True)
+        dyn_hash = self.conf_hash(subconf=True)
         if self.dyn_finalized:
             self.dyn_hash = dyn_hash
         return dyn_hash
@@ -205,8 +206,9 @@ def finalize(self):
 
     def ignore_subconf(self, other, ignore_keys=None):
         """Return True if this config same as other, ignoring sub config."""
-        return (self.conf_hash(dyn=False, subconf=False, ignore_keys=ignore_keys)
-                == other.conf_hash(dyn=False, subconf=False, ignore_keys=ignore_keys))
+        return (self.conf_hash(
+            subconf=False, ignore_keys=ignore_keys) == other.conf_hash(
+                subconf=False, ignore_keys=ignore_keys))
 
     def __eq__(self, other):
         return self.__hash__() == other.__hash__()
@@ -220,8 +222,7 @@ def _check_ip_str(ip_str, ip_method=ipaddress.ip_address):
             # bool type is deprecated by the library ipaddress
             if not isinstance(ip_str, bool):
                 return ip_method(ip_str)
-            else:
-                raise InvalidConfigError('Invalid IP address %s: IP address of type bool' % (ip_str))
+            raise InvalidConfigError('Invalid IP address %s: IP address of type bool' % (ip_str))
         except (ValueError, AttributeError, TypeError) as err:
             raise InvalidConfigError('Invalid IP address %s: %s' % (ip_str, err))
 

faucet/dp.py

@@ -329,6 +329,7 @@ def __init__(self, _id, dp_id, conf):
         self.table_sizes = {}
         self.dyn_up_port_nos = set()
         self.has_externals = None
+        self.stack_graph = None
 
         #tunnel_id: int
         #   ID of the tunnel, for now this will be the VLAN ID
@@ -390,8 +391,6 @@ def check_config(self):
         if self.learn_ban_timeout == 0:
             self.learn_ban_timeout = self.learn_jitter
         if self.stack:
-            if 'graph' in self.stack:
-                del self.stack['graph']
             self._check_conf_types(self.stack, self.stack_defaults_types)
         if self.lldp_beacon:
             self._lldp_defaults()
@@ -864,7 +863,7 @@ def resolve_stack_topology(self, dps, meta_dp_state):
         if graph.size() and self.name in graph:
             if self.stack is None:
                 self.stack = {}
-            self.stack.update({'graph': graph})
+            self.stack_graph = graph
             for dp in graph.nodes():
                 path_to_root_len = len(self.shortest_path(self.stack_root_name, src_dp=dp))
                 test_config_condition(
@@ -878,19 +877,15 @@ def resolve_stack_topology(self, dps, meta_dp_state):
 
     def get_node_link_data(self):
         """Return network stacking graph as a node link representation"""
-        graph = self.stack.get('graph', None)
-        return networkx.json_graph.node_link_data(graph)
+        return networkx.json_graph.node_link_data(self.stack_graph)
 
     def stack_longest_path_to_root_len(self):
         """Return length of the longest path to root in the stack."""
-        if not self.stack or not self.stack_root_name:
-            return None
-        graph = self.stack.get('graph', None)
-        if not graph:
+        if not self.stack_graph or not self.stack_root_name:
             return None
         len_paths_to_root = [
             len(self.shortest_path(self.stack_root_name, src_dp=dp))
-            for dp in graph.nodes()]
+            for dp in self.stack_graph.nodes()]
         if len_paths_to_root:
             return max(len_paths_to_root)
         return None
@@ -924,13 +919,11 @@ def shortest_path(self, dest_dp, src_dp=None):
         """Return shortest path to a DP, as a list of DPs."""
         if src_dp is None:
             src_dp = self.name
-        if self.stack:
-            graph = self.stack.get('graph', None)
-            if graph:
-                try:
-                    return sorted(networkx.all_shortest_paths(graph, src_dp, dest_dp))[0]
-                except (networkx.exception.NetworkXNoPath, networkx.exception.NodeNotFound):
-                    pass
+        if self.stack_graph:
+            try:
+                return sorted(networkx.all_shortest_paths(self.stack_graph, src_dp, dest_dp))[0]
+            except (networkx.exception.NetworkXNoPath, networkx.exception.NodeNotFound):
+                pass
         return []
 
     def shortest_path_to_root(self, src_dp=None):
@@ -1360,9 +1353,11 @@ def _get_conf_changes(logger, conf_name, subconf, new_subconf, diff=False, ignor
         for conf_id, new_conf in new_subconf.items():
             old_conf = subconf.get(conf_id, None)
             if old_conf:
-                if old_conf.ignore_subconf(new_conf, ignore_keys=ignore_keys):
+                if old_conf.ignore_subconf(
+                        new_conf, ignore_keys=ignore_keys):
                     same_confs.add(conf_id)
-                elif old_conf.ignore_subconf(new_conf, ignore_keys=(ignore_keys.union(['description']))):
+                elif old_conf.ignore_subconf(
+                        new_conf, ignore_keys=(ignore_keys.union(['description']))):
                     same_confs.add(conf_id)
                     description_only_confs.add(conf_id)
                     logger.info('%s %s description only changed' % (
@@ -1393,7 +1388,8 @@ def _get_conf_changes(logger, conf_name, subconf, new_subconf, diff=False, ignor
         else:
             logger.info('no %s changes' % conf_name)
 
-        return (changes, deleted_confs, added_confs, changed_confs, same_confs, description_only_confs)
+        return (
+            changes, deleted_confs, added_confs, changed_confs, same_confs, description_only_confs)
 
     def _get_acl_config_changes(self, logger, new_dp):
         """Detect any config changes to ACLs.
@@ -1423,20 +1419,23 @@ def _get_vlan_config_changes(self, logger, new_dp):
             logger, 'VLAN', self.vlans, new_dp.vlans)
         return (deleted_vlans, added_vlans.union(changed_vlans))
 
-    def _get_port_config_changes(self, logger, new_dp, changed_vlans, changed_acls):
+    def _get_port_config_changes(self, logger, new_dp, changed_vlans, deleted_vlans, changed_acls):
         """Detect any config changes to ports.
 
         Args:
             logger (ValveLogger): logger instance.
             new_dp (DP): new dataplane configuration.
             changed_vlans (set): changed/added VLAN IDs.
+            deleted_vlans (set): deleted VLAN IDs.
             changed_acls (set): changed/added ACL IDs.
         Returns:
             changes (tuple) of:
                 all_ports_changed (bool): True if all ports changed.
                 deleted_ports (set): deleted port numbers.
-                changed_ports (set): changed/added port numbers.
+                changed_ports (set): changed port numbers.
+                added_ports (set): added port numbers.
                 changed_acl_ports (set): changed ACL only port numbers.
+                changed_vlans (set): changed/added VLAN IDs.
         """
         _, deleted_ports, added_ports, changed_ports, same_ports, _ = self._get_conf_changes(
             logger, 'port', self.ports, new_dp.ports,
@@ -1445,20 +1444,58 @@ def _get_port_config_changes(self, logger, new_dp, changed_vlans, changed_acls):
         changed_acl_ports = set()
         all_ports_changed = False
 
+        if not same_ports:
+            all_ports_changed = True
         # TODO: optimize case where only VLAN ACL changed.
-        if changed_vlans:
+        elif changed_vlans:
             all_ports = frozenset(new_dp.ports.keys())
-            for vid in changed_vlans:
-                changed_port_nums = {port.number for port in new_dp.vlans[vid].get_ports()}
+            new_changed_vlans = {
+                vlan for vlan in new_dp.vlans.values() if vlan.vid in changed_vlans}
+            for vlan in new_changed_vlans:
+                changed_port_nums = {port.number for port in vlan.get_ports()}
                 changed_ports.update(changed_port_nums)
             all_ports_changed = changed_ports == all_ports
 
-        # TODO: optimize for only mirror options changed on a port
-        # Optimize for case where only the ACL changed on a port.
+        # Detect changes to VLANs and ACLs based on port changes.
         if not all_ports_changed:
+            def _add_changed_vlan_port(port):
+                if port.native_vlan:
+                    changed_vlans.add(port.native_vlan.vid)
+                if port.tagged_vlans:
+                    changed_vlans.update({vlan.vid for vlan in port.tagged_vlans})
+
+            for port_no in changed_ports:
+                old_port = self.ports[port_no]
+                new_port = new_dp.ports[port_no]
+                # native_vlan changed.
+                if old_port.native_vlan != new_port.native_vlan:
+                    for port in (old_port, new_port):
+                        if port.native_vlan:
+                            changed_vlans.add(port.native_vlan.vid)
+                # tagged vlans changed.
+                if old_port.tagged_vlans != new_port.tagged_vlans:
+                    for port in (old_port, new_port):
+                        if port.tagged_vlans:
+                            changed_vlans.update({vlan.vid for vlan in port.tagged_vlans})
+
+            # ports deleted or added.
+            for port_no in deleted_ports:
+                port = self.ports[port_no]
+                _add_changed_vlan_port(port)
+            for port_no in added_ports:
+                port = new_dp.ports[port_no]
+                _add_changed_vlan_port(port)
             for port_no in same_ports:
                 old_port = self.ports[port_no]
                 new_port = new_dp.ports[port_no]
+                # mirror options changed.
+                if old_port.mirror != new_port.mirror:
+                    logger.info('port %s mirror options changed: %s' % (
+                        port_no, new_port.mirror))
+                    for mirrored_port in (old_port, new_port):
+                        if mirrored_port.mirror:
+                            _add_changed_vlan_port(mirrored_port)
+                # ACL changes
                 new_acl_ids = new_port.acls_in
                 port_acls_changed = set()
                 if new_acl_ids:
@@ -1479,8 +1516,20 @@ def _get_port_config_changes(self, logger, new_dp, changed_vlans, changed_acls):
             if changed_acl_ports:
                 same_ports -= changed_acl_ports
                 logger.info('ports where ACL only changed: %s' % changed_acl_ports)
+
+        changed_vlans -= deleted_vlans
+        # TODO: limit scope to only routers that have affected VLANs.
+        changed_vlans_with_vips = []
+        for vid in changed_vlans:
+            vlan = new_dp.vlans[vid]
+            if vlan.faucet_vips:
+                changed_vlans_with_vips.append(vlan)
+        if changed_vlans_with_vips:
+            logger.info('forcing cold start because %s has routing' % changed_vlans_with_vips)
+            all_ports_changed = True
+
         return (all_ports_changed, deleted_ports,
-                added_ports.union(changed_ports), changed_acl_ports)
+                changed_ports, added_ports, changed_acl_ports, changed_vlans)
 
     def _get_meter_config_changes(self, logger, new_dp):
         """Detect any config changes to meters.
@@ -1497,6 +1546,9 @@ def _get_meter_config_changes(self, logger, new_dp):
 
         return (all_meters_changed, deleted_meters, added_meters, changed_meters)
 
+    def _table_configs(self):
+        return frozenset([table.table_config for table in self.tables.values()])
+
     def get_config_changes(self, logger, new_dp):
         """Detect any config changes.
 
@@ -1507,40 +1559,37 @@ def get_config_changes(self, logger, new_dp):
             (tuple): changes tuple containing:
 
                 deleted_ports (set): deleted port numbers.
-                changed_ports (set): changed/added port numbers.
+                changed_ports (set): changed port numbers.
+                added_ports (set): added port numbers.
                 changed_acl_ports (set): changed ACL only port numbers.
                 deleted_vlans (set): deleted VLAN IDs.
                 changed_vlans (set): changed/added VLAN IDs.
                 all_ports_changed (bool): True if all ports changed.
                 deleted_meters (set): deleted meter numbers
                 changed_meters (set): changed/added meter numbers
         """
-        def _table_configs(dp):
-            return frozenset([
-                table.table_config for table in dp.tables.values()])
-
-        if self.ignore_subconf(new_dp):
-            logger.info('DP base level config changed - requires cold start')
-        elif _table_configs(self) != _table_configs(new_dp):
+        if new_dp._table_configs() != self._table_configs():
             logger.info('pipeline table config change - requires cold start')
-        elif new_dp.routers != self.routers:
-            logger.info('DP routers config changed - requires cold start')
         elif new_dp.stack_root_name != self.stack_root_name:
             logger.info('Stack root change - requires cold start')
+        elif new_dp.routers != self.routers:
+            logger.info('DP routers config changed - requires cold start')
+        elif not self.ignore_subconf(new_dp, ignore_keys=['interfaces', 'interfaces_range', 'routers']):
+            logger.info('DP config changed - requires cold start: %s' % self.conf_diff(new_dp))
         else:
             changed_acls = self._get_acl_config_changes(logger, new_dp)
             deleted_vlans, changed_vlans = self._get_vlan_config_changes(logger, new_dp)
-            (all_ports_changed, deleted_ports, changed_ports,
-             changed_acl_ports) = self._get_port_config_changes(
-                 logger, new_dp, changed_vlans, changed_acls)
             (all_meters_changed, deleted_meters,
              added_meters, changed_meters) = self._get_meter_config_changes(logger, new_dp)
-            return (deleted_ports, changed_ports, changed_acl_ports,
+            (all_ports_changed, deleted_ports, changed_ports, added_ports,
+             changed_acl_ports, changed_vlans) = self._get_port_config_changes(
+                 logger, new_dp, changed_vlans, deleted_vlans, changed_acls)
+            return (deleted_ports, changed_ports, added_ports, changed_acl_ports,
                     deleted_vlans, changed_vlans, all_ports_changed,
                     all_meters_changed, deleted_meters,
                     added_meters, changed_meters)
         # default cold start
-        return (set(), set(), set(), set(), set(), True, True, set(), set(), set())
+        return (set(), set(), set(), set(), set(), set(), True, True, set(), set(), set())
 
     def get_tables(self):
         """Return tables as dict for API call."""

faucet/port.py

@@ -133,7 +133,7 @@ class Port(Conf):
         # If true, this port cannot send non-ARP/IPv6 ND broadcasts to other restricted_bcast_arpnd ports.
         'coprocessor': {},
         # If defined, this port is attached to a packet coprocessor.
-        'count_untag_vlan_miss': {},
+        'count_untag_vlan_miss': False,
         # If defined, this port will explicitly count unconfigured native VLAN packets.
     }