Merge pull request #2128 from Chris-Peterson444/server-variant-handling

Update variant handling in SubiquityServer

subiquity/server/controller.py

@@ -111,7 +111,35 @@ async def apply_autoinstall_config(self, context):
         """
         pass
 
-    def interactive(self):
+    def interactive(self) -> bool:
+        """Return whether this controller is "interactive".
+
+        An "interactive" controller is one the installer client is expected to
+        ask the user questions about.  Why would a controller not be
+        interactive? There are a few reasons:
+
+         1. Some controllers do not have an associated UI and can only be
+            configured via autoinstall (e.g. the kernel controller)
+         2. When autoinstalling, the default is that _no_ controllers are
+            interactive, although this can be controlled via
+            `interactive-sections` in the autoinstall config.
+         3. Some controllers are not relevant when installing a particular
+            variant, e.g., we do not ask the user about timezone when
+            installing a server system.
+
+        Interactive controllers are marked configured when the user moves on
+        from the screen for that controller. Non-interactive controllers are
+        configured at different times:
+
+         1. Controllers with no UI are always marked configured by the
+            SubiquityServer during startup, after running the controller's
+            apply_autoinstall_config method.
+         2. During an autoinstall, controllers not listed under
+            'interactive-sections' become non-interactive and are marked
+            configured at the same time as (1).
+         3. Controllers not applicable for a variant are marked configured
+            when the install is confirmed.
+        """
         if not self.app.autoinstall_config:
             return self._active
         i_sections = self.app.autoinstall_config.get("interactive-sections", [])
@@ -128,6 +156,8 @@ def interactive(self):
         ):
             return self._active
 
+        return False
+
     async def configured(self):
         """Let the world know that this controller's model is now configured."""
         with open(self.app.state_path("states", self.name), "w") as fp:

subiquity/server/controllers/source.py

@@ -87,6 +87,7 @@ def __init__(self, app):
         if os.path.exists(path):
             with open(path) as fp:
                 self.model.load_from_file(fp)
+        self._update_variant(self.model.current.variant)
 
     def make_autoinstall(self):
         return {
@@ -148,10 +149,13 @@ def get_handler(
             handler = TrivialSourceHandler("/")
         return handler
 
+    def _update_variant(self, variant: str) -> None:
+        self.app.set_source_variant(variant)
+
     async def configured(self):
         await super().configured()
         self._configured = True
-        self.app.base_model.set_source_variant(self.model.current.variant)
+        self._update_variant(self.model.current.variant)
 
     async def POST(self, source_id: str, search_drivers: bool = False) -> None:
         # Marking the source model configured has an effect on many of the

subiquity/server/controllers/tests/test_source.py

@@ -13,14 +13,19 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import functools
 import unittest
 
 from subiquity.common.serialize import Serializer
 from subiquity.models.source import CatalogEntry
 from subiquity.models.subiquity import SubiquityModel
 from subiquity.models.tests.test_source import make_entry as make_raw_entry
 from subiquity.server.controllers.source import SourceController, convert_source
-from subiquity.server.server import INSTALL_MODEL_NAMES, POSTINSTALL_MODEL_NAMES
+from subiquity.server.server import (
+    INSTALL_MODEL_NAMES,
+    POSTINSTALL_MODEL_NAMES,
+    SubiquityServer,
+)
 from subiquitycore.pubsub import MessageHub
 from subiquitycore.tests import SubiTestCase
 from subiquitycore.tests.mocks import make_app
@@ -58,6 +63,12 @@ def setUp(self):
             "test", MessageHub(), INSTALL_MODEL_NAMES, POSTINSTALL_MODEL_NAMES
         )
         self.app = make_app(model=self.base_model)
+        self.app.set_source_variant = functools.partial(
+            SubiquityServer.set_source_variant, self.app
+        )
+        self.app._set_source_variant = functools.partial(
+            SubiquityServer._set_source_variant, self.app
+        )
         self.app.opts.source_catalog = "examples/sources/install.yaml"
         self.controller = SourceController(self.app)
 
@@ -92,3 +103,28 @@ def test_install_source_detection__autoinstall(self, catalog, ai_data, expected)
         self._set_source_catalog(catalog)
         self.controller.load_autoinstall_data(ai_data)
         self.assertEqual(self.controller.model.current.variant, expected)
+        self.assertEqual(
+            self.controller.app.base_model.source.current.variant, expected
+        )
+
+    def test_update_variant_through_server(self):
+        """Test update variant through server on configure."""
+        app = self.controller.app = unittest.mock.Mock()
+        model = self.controller.app.base_model = unittest.mock.Mock()
+
+        self.controller._update_variant("mock-variant")
+
+        app.set_source_variant.assert_called_with("mock-variant")
+        model.set_source_variant.assert_not_called()
+
+    async def test_on_configure_update_variant(self):
+        """Test variant is updated on configure."""
+        self.controller.model.current.variant = "mock-variant"
+        with (
+            unittest.mock.patch(
+                "subiquity.server.controller.SubiquityController.configured"
+            ),
+            unittest.mock.patch.object(self.controller, "_update_variant"),
+        ):
+            await self.controller.configured()
+            self.controller._update_variant.assert_called_with("mock-variant")

subiquity/server/server.py

@@ -119,7 +119,6 @@ async def mark_configured_POST(self, endpoint_names: List[str]) -> None:
     async def client_variant_POST(self, variant: str) -> None:
         if variant not in self.app.supported_variants:
             raise ValueError(f"unrecognized client variant {variant}")
-        self.app.base_model.set_source_variant(variant)
         self.app.set_source_variant(variant)
 
     async def client_variant_GET(self) -> str:
@@ -291,6 +290,10 @@ def make_model(self):
         root = "/"
         if self.opts.dry_run:
             root = os.path.abspath(self.opts.output_base)
+        # TODO: Set the model source variant before returning it?
+        #       This _will_ eventually get set by the source controller,
+        #       but before then it's in a state that only requires the
+        #       "default" models i.e., the base set all variants require.
         return SubiquityModel(
             root,
             self.hub,
@@ -302,7 +305,7 @@ def make_model(self):
     def __init__(self, opts, block_log_dir):
         super().__init__(opts)
         self.dr_cfg: Optional[DRConfig] = None
-        self.set_source_variant(self.supported_variants[0])
+        self._set_source_variant(self.supported_variants[0])
         self.block_log_dir = block_log_dir
         self.cloud_init_ok = None
         self.state_event = asyncio.Event()
@@ -353,9 +356,26 @@ def __init__(self, opts, block_log_dir):
 
         self.geoip = GeoIP(self, strategy=geoip_strategy)
 
-    def set_source_variant(self, variant):
+    def _set_source_variant(self, variant):
         self.variant = variant
 
+    def set_source_variant(self, variant):
+        """Set the source variant for the install.
+
+        This is the public interface for setting the variant for the install.
+        This ensures that both the server and the model's understanding of the
+        variant is updated in one place.
+
+        Any extra logic for updating the variant in the server should go into
+        the private method _set_source_variant. This is separated out because
+        the sever needs to seed the initial variant state during __init__
+        but the base_model isn't attached to the server object until the .Run()
+        method is called.
+        """
+        self._set_source_variant(variant)
+
+        self.base_model.set_source_variant(variant)
+
     def load_serialized_state(self):
         for controller in self.controllers.instances:
             controller.load_state()

subiquity/server/tests/test_controller.py

@@ -14,12 +14,14 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import contextlib
-from unittest.mock import patch
+from unittest.mock import AsyncMock, patch
 
 from subiquity.server.autoinstall import AutoinstallValidationError
-from subiquity.server.controller import SubiquityController
+from subiquity.server.controller import NonInteractiveController, SubiquityController
+from subiquity.server.types import InstallerChannels
 from subiquitycore.tests import SubiTestCase
 from subiquitycore.tests.mocks import make_app
+from subiquitycore.tests.parameterized import parameterized
 
 
 class TestController(SubiTestCase):
@@ -89,3 +91,122 @@ def test_autoinstall_validation(self):
         # This only checks that controllers do not manually create an apport
         # report on validation. Should also be tested in Server
         self.controller.app.make_apport_report.assert_not_called()
+
+
+class TestControllerInteractive(SubiTestCase):
+    def setUp(self):
+        self.controller = SubiquityController(make_app())
+        self.controller.autoinstall_key = "mock"
+        self.controller.autoinstall_key_alias = "mock-alias"
+
+    def test_interactive_with_no_autoinstall(self):
+        """Test the controller is interactive when not autoinstalling."""
+        self.controller.app.autoinstall_config = {}
+        self.assertTrue(self.controller.interactive())
+
+    @parameterized.expand(
+        (
+            (True, {"interactive-sections": ["mock"]}),
+            (True, {"interactive-sections": ["mock-alias"]}),
+            (True, {"interactive-sections": ["*"]}),
+            (False, {"interactive-sections": ["not-mock"]}),
+            (False, {"interactive-sections": []}),
+        )
+    )
+    def test_interactive_sections(self, interactive, config):
+        """Test controller interactivity honors interactive-sections."""
+        self.controller.app.autoinstall_config = config
+        self.assertEqual(interactive, self.controller.interactive())
+
+    def test_interactive_returns_state_variable(self):
+        """Test interactive returns the _active state variable."""
+        self.controller._active = False
+        # By default _active is True, but can be modified by _confirmed for
+        # interactive_for_variants functionality.
+        self.controller.app.autoinstall_config = {}
+        self.assertFalse(self.controller.interactive())
+
+    def test_interactive_fallthrough_is_false(self):
+        """Test interactive returns False as the fallthrough return.
+
+        bool(None) == False, but None != False. This feels like a bug waiting
+        to happen. Let's really make sure it's False.
+        """
+        self.controller.app.autoinstall_config = {"mocked": "stuff"}
+        self.assertEqual(False, self.controller.interactive())
+        self.assertNotEqual(None, self.controller.interactive())
+
+
+class TestNonInteractiveControllerInteractive(SubiTestCase):
+    def setUp(self):
+        self.controller = NonInteractiveController(make_app())
+        self.controller.autoinstall_key = "mock"
+        self.controller.autoinstall_key_alias = "mock-alias"
+
+    @parameterized.expand(
+        (
+            ({},),
+            ({"interactive-sections": ["mock"]},),
+            ({"interactive-sections": ["mock-alias"]},),
+            ({"interactive-sections": ["*"]},),
+            ({"interactive-sections": ["not-mock"]},),
+            ({"interactive-sections": []},),
+        )
+    )
+    def test_always_non_interactive(self, config):
+        """Test NonInteractiveControllers are always non-interactive"""
+        self.controller.app.autoinstall_config = config
+        self.assertFalse(self.controller.interactive())
+
+
+class TestInteractiveForVariant(SubiTestCase):
+    async def test_no_variant_integration(self):
+        """Test no interactive for variant integration by default."""
+
+        class MockController(SubiquityController):
+            _confirmed = AsyncMock()
+
+        controller = MockController(make_app())
+        self.assertIsNone(controller.interactive_for_variants)
+
+        # Use abroadcast to explicitly block until this broadcast is done
+        await controller.app.hub.abroadcast(InstallerChannels.INSTALL_CONFIRMED)
+        controller._confirmed.assert_not_awaited()
+
+    async def test_variant_integration_subscription(self):
+        """Test _confirmed is called when interative for variants is supported."""
+
+        class MockController(SubiquityController):
+            _confirmed = AsyncMock()
+            interactive_for_variants = ["mock-server"]
+
+        controller = MockController(make_app())
+
+        # Use abroadcast to explicitly block until this broadcast is done
+        await controller.app.hub.abroadcast(InstallerChannels.INSTALL_CONFIRMED)
+        controller._confirmed.assert_awaited()
+
+    @parameterized.expand(
+        (
+            (True, "mock-server", ["mock-server", "mock-desktop"]),
+            (True, "mock-desktop", ["mock-server", "mock-desktop"]),
+            (False, "mock-core", ["mock-server", "mock-desktop"]),
+        )
+    )
+    async def test_variant_confirmed_call(self, active, app_variant, variant_support):
+        """Test no interactive for variant integration by default."""
+
+        class MockController(SubiquityController):
+            configured = AsyncMock()
+            interactive_for_variants = variant_support
+
+        controller = MockController(make_app())
+        controller.app.base_model.source.current.variant = app_variant
+
+        await controller._confirmed()
+        if active:
+            controller.configured.assert_not_awaited()
+            self.assertTrue(controller._active)
+        else:
+            controller.configured.assert_awaited()
+            self.assertFalse(controller._active)

subiquity/server/tests/test_server.py

@@ -744,3 +744,18 @@ def test_push_error_events(self, interactive):
         (message,) = journal_send_mock.call_args.args
         self.assertIn("message", message)
         self.assertNotIn("description", message)
+
+
+class TestVariantHandling(SubiTestCase):
+    async def asyncSetUp(self):
+        opts = Mock()
+        opts.dry_run = True
+        opts.output_base = self.tmp_dir()
+        opts.machine_config = NOPROBERARG
+        self.server = SubiquityServer(opts, None)
+
+    def test_set_source_variant(self):
+        self.server.base_model = Mock()
+        self.server.set_source_variant("mock-variant")
+        self.assertEqual(self.server.variant, "mock-variant")
+        self.server.base_model.set_source_variant.assert_called_with("mock-variant")

subiquity/tests/api/test_api.py

@@ -2330,3 +2330,19 @@ async def test_supported_variants(self, variant, is_supported):
                     "unrecognized client variant foo-bar",
                     json.loads(cre.headers["x-error-msg"]),
                 )
+
+    async def test_post_source_update_server_variant(self):
+        """Test POSTing to source will correctly update Server variant."""
+
+        extra_args = ["--source-catalog", "examples/sources/mixed.yaml"]
+        async with start_server(
+            "examples/machines/simple.json",
+            extra_args=extra_args,
+        ) as inst:
+            resp = await inst.get("/meta/client_variant")
+            self.assertEqual(resp, "server")
+
+            await inst.post("/source", source_id="ubuntu-desktop")
+
+            resp = await inst.get("/meta/client_variant")
+            self.assertEqual(resp, "desktop")