[AIM] Improve validation output for missing external VRF

Previously, if an external network's AIM L3Outside referenced a VRF
that did not exist, validation would fail with a message that an
exception occurred, but with no indication of what caused the
problem. Now, the backtrace is logged when an exception is caught, a
specific error is reported if any of the resources needed for the
NetworkMapping DB record are missing, and a very specific error is
reported if the external VRF is missing.

Also, a requirement on acitoolkit is added to test-requirements.txt to
ensure that acitoolkit's noiro-lite branch is used. This is intended
to avoid acitoolkit's master branch's requirement on deepdiff, whose
latest version no longer supports python 2.

Similarly, the configuration files for the
legacy-group-based-policy-dsvm-aim CI job are modified to explicitly
install the noiro-lite branch of acitoolkit.

Change-Id: I7955f8e77633d9662a629c8c0628b128be3ae546

devstack/lib/apic_aim

@@ -1,9 +1,10 @@
 function install_apic_aim {
     echo_summary "Installing apic_aim"
 
+    install_acitoolkit
+    install_apicapi
     install_aim
     install_opflex
-    install_apicapi
 }
 
 function configure_apic_aim {
@@ -42,6 +43,20 @@ function configure_apic_aim {
     init_aim
 }
 
+function install_acitoolkit {
+    git_clone $ACITOOLKIT_REPO $ACITOOLKIT_DIR $ACITOOLKIT_BRANCH
+    touch $ACITOOLKIT_DIR/setup.cfg
+    setup_develop $ACITOOLKIT_DIR
+}
+
+function install_apicapi {
+    git_clone $APICAPI_REPO $APICAPI_DIR $APICAPI_BRANCH
+    mv $APICAPI_DIR/test-requirements.txt $APICAPI_DIR/_test-requirements.txt
+    touch $APICAPI_DIR/setup.cfg
+    setup_develop $APICAPI_DIR
+    mv $APICAPI_DIR/_test-requirements.txt $APICAPI_DIR/test-requirements.txt
+}
+
 function install_aim {
     git_clone $AIM_REPO $AIM_DIR $AIM_BRANCH
     mv $AIM_DIR/test-requirements.txt $AIM_DIR/_test-requirements.txt

devstack/lib/group-based-policy

@@ -28,6 +28,8 @@ OPFLEX_REPO=http://github.com/noironetworks/python-opflex-agent.git
 OPFLEX_DIR=$DEST/opflexagent
 APICAPI_REPO=http://github.com/noironetworks/apicapi.git
 APICAPI_DIR=$DEST/apicapi
+ACITOOLKIT_REPO=http://github.com/noironetworks/acitoolkit.git
+ACITOOLKIT_DIR=$DEST/acitoolkit
 
 # Save trace setting
 XTRACE=$(set +o | grep xtrace)
@@ -85,14 +87,6 @@ function install_gbpui {
     mv $GBPUI_DIR/_test-requirements.txt $GBPUI_DIR/test-requirements.txt
 }
 
-function install_apicapi {
-    git_clone $APICAPI_REPO $APICAPI_DIR $APICAPI_BRANCH
-    mv $APICAPI_DIR/test-requirements.txt $APICAPI_DIR/_test-requirements.txt
-    touch $APICAPI_DIR/setup.cfg
-    setup_develop $APICAPI_DIR
-    mv $APICAPI_DIR/_test-requirements.txt $APICAPI_DIR/test-requirements.txt
-}
-
 
 # Restore xtrace
 $XTRACE

devstack/settings

@@ -33,6 +33,7 @@ GBPHEAT_BRANCH=${GBPHEAT_BRANCH:-master}
 AIM_BRANCH=${AIM_BRANCH:-master}
 OPFLEX_BRANCH=${OPFLEX_BRANCH:-master}
 APICAPI_BRANCH=${APICAPI_BRANCH:-master}
+ACITOOLKIT_BRANCH=${ACITOOLKIT_BRANCH:-noiro-lite}
 
 # Enable necessary services, including group-policy (and disable others)
 disable_service n-net

gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py

@@ -4712,7 +4712,6 @@ class ApicMechanismDriver(api_plus.MechanismDriver,
             bd = None
             epg = None
             vrf = None
-            ext_net = None
             if net_db.external:
                 bd, epg, vrf = self._validate_external_network(
                     mgr, net_db, ext_net_routers, router_dbs, router_vrfs,
@@ -4753,11 +4752,19 @@ class ApicMechanismDriver(api_plus.MechanismDriver,
                     # EPG?
                     pass
 
-            # Expect NetworkMapping record if applicable.
+            # Expect NetworkMapping record if applicable. Note that
-            if bd or epg or vrf or ext_net:
+            # when validation of SVI networks is implemented, there
+            # will also be an ext_net resource, and when this is used,
+            # the bd and epg are not used.
+            if bd and epg and vrf:
                 self._add_network_mapping(
-                    mgr.expected_session, net_db.id, bd, epg, vrf, ext_net,
+                    mgr.expected_session, net_db.id, bd, epg, vrf,
                     update_network=False)
+            elif bd or epg or vrf:
+                mgr.validation_failed(
+                    "Missing resource(s) needed to create expected "
+                    "NetworkMapping for network %s - bd: %s, epg: %s, "
+                    "vrf: %s" % (net_db.id, bd, epg, vrf))
 
     def _get_router_ext_contracts(self, mgr):
         # Get external contracts for routers.
@@ -5079,6 +5086,12 @@ class ApicMechanismDriver(api_plus.MechanismDriver,
         cidrs = sorted(self.get_external_cidrs_by_ext_net_dn(
             mgr.actual_session, ext_net.dn, lock_update=False))
         ns.update_external_cidrs(mgr.expected_aim_ctx, ext_net, cidrs)
+
+        # Get the L3Outside resources, and check that there is an
+        # external VRF.
+        bd = None
+        epg = None
+        vrf = None
         for resource in ns.get_l3outside_resources(
                 mgr.expected_aim_ctx, l3out):
             if isinstance(resource, aim_resource.BridgeDomain):
@@ -5087,6 +5100,9 @@ class ApicMechanismDriver(api_plus.MechanismDriver,
                 epg = resource
             elif isinstance(resource, aim_resource.VRF):
                 vrf = resource
+        if not vrf:
+            mgr.validation_failed(
+                "missing external VRF for external network %s" % net_db.id)
 
         for subnet_db in net_db.subnets:
             if subnet_db.gateway_ip:

gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_validation.py

@@ -123,8 +123,10 @@ class ValidationManager(object):
         except RollbackTransaction:
             pass
         except Exception as exc:
-            self.output("Validation failed with exception: %s" % exc)
+            self.output("Validation failed with exception: %s - see log for "
-            return api.VALIDATION_FAILED_UNREPAIRABLE
+                        "details" % exc)
+            LOG.exception(exc)
+            return api.VALIDATION_FAILED_WITH_EXCEPTION
 
         # Bind unbound ports outside transaction.
         if (self.repair and

gbpservice/neutron/services/grouppolicy/group_policy_driver_api.py

@@ -21,8 +21,18 @@ from gbpservice.common import utils
 VALIDATION_PASSED = "passed"
 VALIDATION_REPAIRED = "repaired"
 VALIDATION_FAILED_REPAIRABLE = "failed repairable"
-VALIDATION_FAILED_UNREPAIRABLE = "failed unrepairable"
 VALIDATION_FAILED_BINDING_PORTS = "failed binding ports"
+VALIDATION_FAILED_UNREPAIRABLE = "failed unrepairable"
+VALIDATION_FAILED_WITH_EXCEPTION = "failed with exception"
+
+VALIDATION_RESULT_PRECEDENCE = [
+    VALIDATION_PASSED,
+    VALIDATION_REPAIRED,
+    VALIDATION_FAILED_REPAIRABLE,
+    VALIDATION_FAILED_BINDING_PORTS,
+    VALIDATION_FAILED_UNREPAIRABLE,
+    VALIDATION_FAILED_WITH_EXCEPTION,
+]
 
 
 @six.add_metaclass(abc.ABCMeta)
@@ -1400,8 +1410,10 @@ class PolicyDriver(object):
         Called from validation tool to validate policy driver's
         persistent state. Returns VALIDATION_PASSED,
         VALIDATION_REPAIRED (only if repair == True),
-        VALIDATION_FAILED_REPAIRABLE (only if repair == False) or
+        VALIDATION_FAILED_REPAIRABLE (only if repair == False),
-        VALIDATION_FAILED_UNREPAIRABLE.
+        VALIDATION_FAILED_UNREPAIRABLE,
+        VALIDATION_FAILED_WITH_EXCEPTION, or
+        VALIDATION_FAILED_BINDING_PORTS.
         """
         return VALIDATION_PASSED
 

gbpservice/neutron/services/grouppolicy/policy_driver_manager.py

@@ -499,10 +499,12 @@ class PolicyDriverManager(stevedore.named.NamedExtensionManager):
         result = api.VALIDATION_PASSED
         for driver in self.ordered_policy_drivers:
             this_result = driver.obj.validate_state(repair)
-            if (this_result == api.VALIDATION_FAILED_UNREPAIRABLE or
+            if this_result not in api.VALIDATION_RESULT_PRECEDENCE:
-                (this_result == api.VALIDATION_FAILED_REPAIRABLE and
+                LOG.error("Policy driver %s validate_state returned "
-                 result != api.VALIDATION_FAILED_UNREPAIRABLE) or
+                          "unrecognized result: %s" %
-                (this_result == api.VALIDATION_REPAIRED and
+                          (driver.name, this_result))
-                 result == api.VALIDATION_PASSED)):
+                this_result = api.VALIDATION_FAILED_WITH_EXCEPTION
+            if (api.VALIDATION_RESULT_PRECEDENCE.index(this_result) >
+                api.VALIDATION_RESULT_PRECEDENCE.index(result)):
                 result = this_result
         return result

gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py

@@ -580,6 +580,42 @@ class TestNeutronMapping(AimValidationTestCase):
         self._validate_repair_validate()
         self._test_network_attrs(net)
 
+        # Delete pre-existing AIM VRF and test.
+        self.aim_mgr.delete(self.aim_ctx, vrf)
+        self._validate_unrepairable()
+
+        # Replace pre-existing AIM VRF and test.
+        self.aim_mgr.create(self.aim_ctx, vrf)
+        self._validate()
+
+        # REVISIT: Missing AIM L3Outsides, ExternalNetworks, and
+        # ExternalSubnets that were supposed to be pre-existing all
+        # get silently created by the NAT strategy code, so these are
+        # considered repairable for now, but the repair is likely to
+        # result in referencing the wrong VRF, the resource no longer
+        # being considered monitored, and other configuration
+        # errors. Consider adding validation code to check that the
+        # ExternalNetwork identified by
+        # NetworkExtensionDb.external_network_dn, along with its
+        # parent L3Outside, actually exist before calling the NAT
+        # strategy code, and failing with
+        # VALIDATION_FAILED_UNREPAIRABLE if they don't exist. The
+        # mechanism driver should probably also check that they exist
+        # when the network is created, and fail if they don't, but
+        # that might break existing use cases.
+
+        # Delete pre-existing AIM L3Outside and test.
+        self.aim_mgr.delete(self.aim_ctx, l3out)
+        self._validate_repair_validate()
+
+        # Delete pre-existing AIM ExternalNetwork and test.
+        self.aim_mgr.delete(self.aim_ctx, ext_net)
+        self._validate_repair_validate()
+
+        # Delete pre-existing AIM ExternalSubnet and test.
+        self.aim_mgr.delete(self.aim_ctx, ext_sn)
+        self._validate_repair_validate()
+
     def test_svi_network(self):
         # REVISIT: Test validation of actual mapping once implemented.
 

gbpservice/tests/contrib/devstack/local-aim.conf

@@ -20,3 +20,4 @@ ENABLE_APIC_AIM_GATE=True
 AIM_BRANCH=master
 OPFLEX_BRANCH=master
 APICAPI_BRANCH=master
+ACITOOLKIT_BRANCH=noiro-lite

gbpservice/tools/validate/cli.py

@@ -54,6 +54,7 @@ def main():
 
     result = gbp_plugin.validate_state(cfg.CONF.repair)
     if result in [api.VALIDATION_FAILED_REPAIRABLE,
-                  api.VALIDATION_FAILED_UNREPAIRABLE]:
+                  api.VALIDATION_FAILED_UNREPAIRABLE,
+                  api.VALIDATION_FAILED_WITH_EXCEPTION]:
         sys.exit(result)
     return 0

test-requirements.txt

@@ -44,7 +44,9 @@ pyOpenSSL>=16.2.0  # Apache-2.0
 python-heatclient
 python-keystoneclient
 
-# REVISIT: Until co-gating and/or stable branches are implemented for the
+# REVISIT: Until co-gating and/or stable branches are implemented for
-# aci-integration-module repo, it may be necessary to pin to a working
+# the aci-integration-module repo, it may be necessary to pin to a
-# commit.
+# working commit. Also, specific branches in indirect dependencies
+# seem to be ignored, so we list them here too.
+-e git+https://github.com/noironetworks/acitoolkit.git@noiro-lite#egg=acitoolkit
 -e git+https://github.com/noironetworks/aci-integration-module.git@master#egg=aci-integration-module