feat(nimbus): Load versioned manifests in load_feature_configs (#9814)

Because

- we have versioned feature manifests; and
- we want to use these in Experimenter

This commit

- updates load_feature_configs to import versioned manifests.

Fixes #9809

experimenter/Dockerfile

@@ -176,6 +176,7 @@ COPY --from=dev /experimenter/bin/ /experimenter/bin/
 COPY --from=dev /application-services/megazord/ /application-services/megazord/
 COPY --from=file-loader /experimenter/manage.py /experimenter/manage.py
 COPY --from=file-loader /experimenter/experimenter/ /experimenter/experimenter/
+COPY --from=file-loader /experimenter/manifesttool/ /experimenter/manifesttool/
 COPY --from=ui /experimenter/experimenter/legacy/legacy-ui/assets/ /experimenter/experimenter/legacy/legacy-ui/assets/
 COPY --from=ui /experimenter/experimenter/nimbus-ui/build/ /experimenter/experimenter/nimbus-ui/build/
 COPY --from=ui /experimenter/experimenter/theme/static/ /experimenter/experimenter/theme/static/

experimenter/experimenter/features/__init__.py

@@ -1,8 +1,8 @@
 import json
-import os
+import re
 from dataclasses import dataclass
 from pathlib import Path
-from typing import Union
+from typing import Iterable, Optional, Union
 
 import yaml
 from django.conf import settings
@@ -14,7 +14,8 @@ from mozilla_nimbus_schemas.experiments.feature_manifests import (
     FeatureWithoutExposure,
 )
 
-from experimenter.experiments.constants import NimbusConstants
+from experimenter.experiments.constants import ApplicationConfig, NimbusConstants
+from manifesttool.version import Version
 
 FEATURE_SCHEMA_TYPES = {
     FeatureVariableType.INT: "integer",
@@ -34,6 +35,7 @@ class Feature:
     slug: str
     application_slug: str
     model: Union[FeatureWithExposure, FeatureWithoutExposure]
+    version: Optional[Version] = None
 
     def load_remote_jsonschema(self):
         if self.model.json_schema is not None:
@@ -80,30 +82,53 @@ class Feature:
 
 
 class Features:
-    _features = None
+    _features: Optional[list[Feature]] = None
+
+    @classmethod
+    def _read_manifest(
+        cls,
+        application: ApplicationConfig,
+        manifest_path: Path,
+        version: Version = None,
+    ):
+        with manifest_path.open() as manifest_file:
+            application_data = yaml.safe_load(manifest_file)
+            manifest = FeatureManifest.parse_obj(application_data)
+
+            for feature_slug, feature_model in manifest.__root__.items():
+                yield Feature(
+                    slug=feature_slug,
+                    application_slug=application.slug,
+                    model=feature_model.__root__,
+                    version=version,
+                )
 
     @classmethod
     def _load_features(cls):
         features = []
+        version_re = re.compile(r"^v(?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)")
 
         for application in NimbusConstants.APPLICATION_CONFIGS.values():
-            application_yaml_path = os.path.join(
-                settings.FEATURE_MANIFESTS_PATH, application.slug, "experimenter.yaml"
-            )
+            application_dir: Path = settings.FEATURE_MANIFESTS_PATH / application.slug
+            application_yaml_path = application_dir / "experimenter.yaml"
 
-            if os.path.exists(application_yaml_path):
-                with open(application_yaml_path) as application_yaml_file:
-                    application_data = yaml.safe_load(application_yaml_file)
-                    manifest = FeatureManifest.parse_obj(application_data)
+            if application_yaml_path.exists():
+                features.extend(cls._read_manifest(application, application_yaml_path))
 
-                    for feature_slug, feature_model in manifest.__root__.items():
-                        features.append(
-                            Feature(
-                                slug=feature_slug,
-                                application_slug=application.slug,
-                                model=feature_model.__root__,
+                for child in application_dir.iterdir():
+                    if not child.is_dir():
+                        continue
+
+                    if m := version_re.match(child.name):
+                        version = Version.from_match(m.groupdict())
+
+                        application_yaml_path = child / "experimenter.yaml"
+                        if application_yaml_path.exists():
+                            features.extend(
+                                cls._read_manifest(
+                                    application, application_yaml_path, version
+                                )
                             )
-                        )
 
         return features
 
@@ -112,16 +137,24 @@ class Features:
         cls._features = None
 
     @classmethod
-    def all(cls):
+    def all(cls) -> list[Feature]:
         if cls._features is None:
             cls._features = cls._load_features()
 
         return cls._features
 
     @classmethod
-    def by_application(cls, application):
+    def by_application(cls, application) -> list[Feature]:
         return [f for f in cls.all() if f.application_slug == application]
 
+    @classmethod
+    def unversioned(cls) -> Iterable[Feature]:
+        return (f for f in cls.all() if f.version is None)
+
+    @classmethod
+    def versioned(cls) -> Iterable[Feature]:
+        return (f for f in cls.all() if f.version is not None)
+
 
 @register()
 def check_features(app_configs, **kwargs):

experimenter/experimenter/features/management/commands/load_feature_configs.py

@@ -1,13 +1,18 @@
+import itertools
 import logging
+from typing import Optional
 
 from django.core.management.base import BaseCommand
+from django.db import transaction
 
 from experimenter.experiments.constants import NO_FEATURE_SLUG
 from experimenter.experiments.models import (
     NimbusFeatureConfig,
+    NimbusFeatureVersion,
     NimbusVersionedSchema,
 )
 from experimenter.features import Features
+from manifesttool.version import Version
 
 logger = logging.getLogger()
 
@@ -15,45 +20,88 @@ logger = logging.getLogger()
 class Command(BaseCommand):
     help = "Load Feature Configs from remote sources"
 
+    @transaction.atomic
     def handle(self, *args, **options):
         logger.info("Loading Features")
 
-        features_to_disable = set(
-            NimbusFeatureConfig.objects.values_list("application", "slug")
-        )
+        # A mapping of (application slug, feature slug) to feature configs.
+        #
+        # Features will include multiples of the same feature, each with a
+        # different version. We only need to create a single NimbusFeatureConfig
+        # for each of those, so if we keep track of them we can skip a bunch of
+        # update_or_create() calls.
+        feature_configs: dict[tuple[str, str], NimbusFeatureConfig] = {
+            (fc.application, fc.slug): fc for fc in NimbusFeatureConfig.objects.all()
+        }
 
-        for feature in Features.all():
-            feature_config, created = NimbusFeatureConfig.objects.update_or_create(
-                slug=feature.slug,
-                application=feature.application_slug,
-                defaults={
-                    "name": feature.slug,
-                    "description": feature.model.description,
-                    "enabled": True,
-                },
-            )
+        features_to_disable = set(feature_configs.keys())
 
-            if not created:
-                features_to_disable.discard((feature.application_slug, feature.slug))
+        # Iterate through the unversioned features first for the initial
+        #
+        # Then we can iterate over the versioned feature and create any that
+        # aren't in the unversioned manifests.
+        #
+        # When we are ingesting versioned Features, we want to update the
+        # NimbusFeatureConfig objects with the most up-to-date description.
+        updated: set[tuple[str, str]] = set()
+        for feature in itertools.chain(
+            Features.unversioned(),
+            sorted(Features.versioned(), key=lambda f: f.version, reverse=True),
+        ):
+            key = (feature.application_slug, feature.slug)
+            if key in updated:
+                # We have already processes the unversioned feature OR a
+                # versioned feature at a newer version.
+                #
+                # The unversioned features are the most up-to-date (since they come from
+                # the main branch (or equivalent)), so lets use the descriptions from
+                # there, if they exist.
+                continue
 
-            defaults = {
-                "sets_prefs": [
-                    v.set_pref
-                    for v in feature.model.variables.values()
-                    if v.set_pref is not None
-                ]
-            }
-            if (schema := feature.get_jsonschema()) is not None:
-                defaults["schema"] = schema
+            # Django doesn't keep track of whether or not fields were updated
+            # when you call save(). By default, it will update every field in
+            # the object, whether or not it was dirtied. However, if we are
+            # creating the object, we have to save every field anyway.
+            created = False
+            dirty_fields = []
 
-            NimbusVersionedSchema.objects.update_or_create(
-                feature_config=feature_config,
-                version=None,
-                defaults=defaults,
-            )
+            feature_config = feature_configs.get(key)
+            if feature_config is None:
+                feature_config = NimbusFeatureConfig(
+                    slug=feature.slug,
+                    application=feature.application_slug,
+                    name=feature.slug,
+                )
+                created = True
 
-            logger.info(f"Feature Loaded: {feature.application_slug}/{feature.slug}")
+            if feature_config.name != feature.slug:
+                feature_config.name = feature.slug
+                dirty_fields.append("name")
 
+            if feature_config.description != feature.model.description:
+                feature_config.description = feature.model.description
+                dirty_fields.append("description")
+
+            if not feature_config.enabled:
+                feature_config.enabled = True
+                dirty_fields.append("enabled")
+
+            if created:
+                # We don't have enough feature configs to justify moving this
+                # into a bulk_create, as it does not noticably impact
+                # performance.
+                feature_config.save()
+            elif dirty_fields:
+                feature_config.save(update_fields=dirty_fields)
+
+            updated.add(key)
+
+            if created:
+                feature_configs[key] = feature_config
+            else:
+                features_to_disable.discard(key)
+
+        # Disable any features that we didn't come across.
         for application_slug, feature_slug in features_to_disable:
             if feature_slug not in NO_FEATURE_SLUG:
                 logger.info(f"Feature Not Found in YAML: {feature_slug}")
@@ -61,4 +109,87 @@ class Command(BaseCommand):
                     application=application_slug, slug=feature_slug
                 ).update(enabled=False)
 
+        # Populate the version table and cache the results so we can re-use them
+        # in NimbusVersionedSchema creation.
+        versions: dict[Version, NimbusFeatureVersion] = {
+            Version(v.major, v.minor, v.patch): v
+            for v in NimbusFeatureVersion.objects.all()
+        }
+
+        versions_to_create = []
+        for feature in Features.versioned():
+            assert feature.version is not None
+
+            if feature.version in versions:
+                continue
+
+            version = versions[feature.version] = NimbusFeatureVersion(
+                major=feature.version.major,
+                minor=feature.version.minor,
+                patch=feature.version.patch,
+            )
+            versions_to_create.append(version)
+
+        NimbusFeatureVersion.objects.bulk_create(versions_to_create)
+
+        # A mapping of (feature_config.id, version.id) to NimbusVersionedSchemas.
+        schemas: dict[tuple[int, Optional[int]], NimbusVersionedSchema] = {
+            (schema.feature_config_id, schema.version_id): schema
+            for schema in NimbusVersionedSchema.objects.all()
+        }
+
+        # If we call .save() on a newly created model, Django will not properly
+        # aggregate all the model creations into a single operation. It is
+        # faster to call save() only for updates and use bulk_create() for
+        # inserts.
+        schemas_to_create = []
+        for feature in Features.all():
+            feature_config = feature_configs[(feature.application_slug, feature.slug)]
+
+            feature_version: Optional[NimbusFeatureVersion] = None
+            feature_version_id: Optional[int] = None
+            if feature.version:
+                feature_version = versions[feature.version]
+                feature_version_id = feature_version.id
+
+            key = (feature_config.id, feature_version_id)
+
+            dirty_fields = []
+            created = False
+
+            schema = schemas.get(key)
+            if schema is None:
+                created = True
+                schema = NimbusVersionedSchema(
+                    feature_config=feature_config,
+                    version=feature_version,
+                )
+
+            if (jsonschema := feature.get_jsonschema()) is not None:
+                if schema.schema != jsonschema:
+                    schema.schema = jsonschema
+                    dirty_fields.append("schema")
+
+            sets_prefs = [
+                v.set_pref
+                for v in feature.model.variables.values()
+                if v.set_pref is not None
+            ]
+
+            if schema.sets_prefs != sets_prefs:
+                schema.sets_prefs = sets_prefs
+                dirty_fields.append("sets_prefs")
+
+            if created:
+                schemas_to_create.append(schema)
+            elif dirty_fields:
+                schema.save(update_fields=dirty_fields)
+
+            logger.info(
+                f"Feature Loaded: {feature.application_slug}/{feature.slug} "
+                f"(version {feature.version})"
+            )
+
+        NimbusVersionedSchema.objects.bulk_create(schemas_to_create)
+
         logger.info("Features Updated")

experimenter/experimenter/features/tests/__init__.py

@@ -1,37 +1,25 @@
-import os
-import pathlib
+from pathlib import Path
 
 from django.test import override_settings
 
+FIXTURE_DIR = Path(__file__).parent.absolute() / "fixtures"
+
 mock_valid_features = override_settings(
-    FEATURE_MANIFESTS_PATH=os.path.join(
-        pathlib.Path(__file__).parent.absolute(), "fixtures", "valid_features"
-    )
+    FEATURE_MANIFESTS_PATH=FIXTURE_DIR / "valid_features"
 )
 
 mock_invalid_features = override_settings(
-    FEATURE_MANIFESTS_PATH=os.path.join(
-        pathlib.Path(__file__).parent.absolute(), "fixtures", "invalid_features"
-    )
+    FEATURE_MANIFESTS_PATH=FIXTURE_DIR / "invalid_features"
 )
 
-MOCK_REMOTE_FEATURE_MANIFESTS_PATH = os.path.join(
-    pathlib.Path(__file__).parent.absolute(), "fixtures", "remote_schema_features"
-)
 mock_remote_schema_features = override_settings(
-    FEATURE_MANIFESTS_PATH=MOCK_REMOTE_FEATURE_MANIFESTS_PATH,
-    FEATURE_SCHEMAS_PATH=os.path.join(MOCK_REMOTE_FEATURE_MANIFESTS_PATH, "schemas"),
-)
-
-MOCK_INVALID_REMOTE_FEATURE_MANIFESTS_PATH = os.path.join(
-    pathlib.Path(__file__).parent.absolute(),
-    "fixtures",
-    "invalid_remote_schema_features",
+    FEATURE_MANIFESTS_PATH=FIXTURE_DIR / "remote_schema_features"
 )
 
 mock_invalid_remote_schema_features = override_settings(
-    FEATURE_MANIFESTS_PATH=MOCK_INVALID_REMOTE_FEATURE_MANIFESTS_PATH,
-    FEATURE_SCHEMAS_PATH=os.path.join(
-        MOCK_INVALID_REMOTE_FEATURE_MANIFESTS_PATH, "schemas"
-    ),
+    FEATURE_MANIFESTS_PATH=FIXTURE_DIR / "invalid_remote_schema_features"
+)
+
+mock_versioned_features = override_settings(
+    FEATURE_MANIFESTS_PATH=FIXTURE_DIR / "versioned_features"
 )

experimenter/experimenter/features/tests/fixtures/versioned_features/firefox-desktop/experimenter.yaml

@@ -0,0 +1,8 @@
+feature-1:
+  description: Unversioned Feature 1
+  owner: owner@example.com
+  hasExposure: false
+  variables:
+    value1:
+      type: string
+      description: foo

experimenter/experimenter/features/tests/fixtures/versioned_features/firefox-desktop/v120.0.0/experimenter.yaml

@@ -0,0 +1,13 @@
+feature-1:
+  description: Feature 1 for version 120.0.0
+  owner: owner@example.com
+  hasExposure: false
+  variables:
+    value2:
+      type: string
+      description: bar
+
+feature-2:
+  description: Feature 2 for version 120.0.0
+  hasExposure: false
+  variables: {}

experimenter/experimenter/features/tests/fixtures/versioned_features/firefox-desktop/v120.1.0/experimenter.yaml

@@ -0,0 +1,14 @@
+feature-1:
+  description: Feature 1 for version 120.1.0
+  owner: owner@example.com
+  hasExposure: false
+  variables:
+    value3:
+      type: string
+      description: baz
+
+feature-2:
+  description: Feature 2 for version 120.1.0
+  hasExposure: false
+  variables: {}
+

experimenter/experimenter/features/tests/test_load_feature_configs.py

@@ -3,7 +3,11 @@ import json
 from django.core.management import call_command
 from django.test import TestCase
 
-from experimenter.experiments.models import NimbusExperiment, NimbusFeatureConfig
+from experimenter.experiments.models import (
+    NimbusExperiment,
+    NimbusFeatureConfig,
+    NimbusFeatureVersion,
+)
 from experimenter.experiments.tests.factories import (
     NimbusFeatureConfigFactory,
     NimbusVersionedSchemaFactory,
@@ -12,6 +16,7 @@ from experimenter.features import Features
 from experimenter.features.tests import (
     mock_invalid_remote_schema_features,
     mock_valid_features,
+    mock_versioned_features,
 )
 
 
@@ -257,3 +262,52 @@ class TestLoadInvalidRemoteSchemaFeatureConfigs(TestCase):
 
         feature_config = NimbusFeatureConfig.objects.get(slug="no-feature-fenix")
         self.assertEqual(feature_config.enabled, True)
+
+
+@mock_versioned_features
+class TestLoadVersionedFeatureConfigs(TestCase):
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        Features.clear_cache()
+
+    def test_load_feature_configs(self):
+        NimbusFeatureConfig.objects.all().delete()
+
+        call_command("load_feature_configs")
+
+        self.assertEqual(NimbusFeatureVersion.objects.count(), 2)
+        v120_0_0 = NimbusFeatureVersion.objects.get(major=120, minor=0, patch=0)
+        v120_1_0 = NimbusFeatureVersion.objects.get(major=120, minor=1, patch=0)
+
+        self.assertEqual(NimbusFeatureConfig.objects.count(), 2)
+        feature_1 = NimbusFeatureConfig.objects.get(name="feature-1")
+        self.assertEqual(feature_1.description, "Unversioned Feature 1")
+        self.assertEqual(feature_1.schemas.count(), 3)
+        self.assertEqual(
+            feature_1.schemas.filter(feature_config=feature_1, version=None).count(), 1
+        )
+        self.assertEqual(
+            feature_1.schemas.filter(feature_config=feature_1, version=v120_0_0).count(),
+            1,
+        )
+        self.assertEqual(
+            feature_1.schemas.filter(feature_config=feature_1, version=v120_1_0).count(),
+            1,
+        )
+
+        feature_2 = NimbusFeatureConfig.objects.get(name="feature-2")
+        self.assertIsNotNone(feature_2)
+        self.assertEqual(feature_2.description, "Feature 2 for version 120.1.0")
+        self.assertEqual(feature_2.schemas.count(), 2)
+        self.assertEqual(
+            feature_2.schemas.filter(feature_config=feature_2, version=None).count(), 0
+        )
+        self.assertEqual(
+            feature_2.schemas.filter(feature_config=feature_2, version=v120_0_0).count(),
+            1,
+        )
+        self.assertEqual(
+            feature_2.schemas.filter(feature_config=feature_2, version=v120_1_0).count(),
+            1,
+        )

experimenter/experimenter/settings.py

@@ -12,6 +12,7 @@ https://docs.djangoproject.com/en/1.9/ref/settings/
 import json
 import os
 from importlib import resources
+from pathlib import Path
 from urllib.parse import urljoin
 
 from decouple import config
@@ -491,7 +492,7 @@ JETSTREAM_CONFIG_OUTCOMES_PATH = os.path.join(
 )
 
 # Feature Manifest path
-FEATURE_MANIFESTS_PATH = os.path.join(BASE_DIR, "features", "manifests")
+FEATURE_MANIFESTS_PATH = Path(BASE_DIR, "features", "manifests")
 
 SKIP_REVIEW_ACCESS_CONTROL_FOR_DEV_USER = config(
     "SKIP_REVIEW_ACCESS_CONTROL_FOR_DEV_USER", default=False, cast=bool