sync: Add GetApprovedManifestV2 with named request options

Smart sync calls GetApprovedManifest(branch[, target]) over XML-RPC,
which only supports positional arguments.  Adding new request options
that way does not scale.

Add a GetApprovedManifestV2 method that takes a single struct of named
options and returns a struct:

  request:  {branch, target?, best_effort?}
  response: {success, manifest_xml?, error_message?}

Servers must ignore unknown options, so new options (e.g. criteria for
selecting a good build) can be added without another method.

The first new option is `best_effort`.  repo sets it when smart sync was
enabled implicitly by the manifest's sync-smartsync="true" default
rather than by -s.  The server may decline a best_effort request (e.g.
for rollout or cohort gating) by returning success=false.  The client
still owns the fallback policy: declined implicit requests fall back to
a regular sync, while explicit -s requests fail as before.

If the server does not implement GetApprovedManifestV2, repo falls back
to the legacy GetApprovedManifest call.

Bug: 545137090
Test: ./run_tests
Change-Id: I9297b728df26516012cb93dffc91d39c1fb0458f
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/636623
Reviewed-by: Mike Frysinger <vapier@google.com>
Commit-Queue: Rahul Yadav <yadavrah@google.com>
Tested-by: Rahul Yadav <yadavrah@google.com>

docs/smart-sync.md

@@ -95,6 +95,36 @@ example would do.  Google's internal server uses Python's
 
 The manifest server should implement the following RPC methods.
 
+### GetApprovedManifestV2
+
+> `GetApprovedManifestV2(request: dict) -> dict`
+
+Like [GetApprovedManifest](#GetApprovedManifest), but takes a single struct of
+named options so new options can be added without changing the method signature.
+Unset options are omitted from the struct. Servers should ignore options they do
+not understand.
+
+Request options:
+
+*   `branch` (`str`, required): Same as in `GetApprovedManifest`.
+*   `target` (`str`, optional): Same as in `GetApprovedManifest`.
+*   `best_effort` (`bool`, optional, default `False`): The client can proceed
+    without a manifest, so the server may decline the request (e.g. during a
+    staged rollout or to shed load). Repo sets this for Smart Syncs enabled by
+    the manifest's `sync-smartsync` default, and falls back to a regular ToT
+    sync if the request is declined. Servers must not decline requests without
+    this option for these reasons.
+
+Response fields:
+
+*   `success` (`bool`): Whether a manifest was returned.
+*   `manifest_xml` (`str`): The manifest. Required if `success` is true.
+*   `error_message` (`str`): Why the request failed or was declined. Repo shows
+    it to the user like `GetApprovedManifest` errors.
+
+If the server does not implement this method, repo calls `GetApprovedManifest`
+instead and drops the options it cannot express.
+
 ### GetApprovedManifest
 
 > `GetApprovedManifest(branch: str, target: Optional[str]) -> str`
@@ -166,16 +196,19 @@ Sync unless explicitly overridden on the command line. Since the user did not
 ask for a Smart Sync, any failure to obtain a manifest from the manifest server
 (e.g. the server is unreachable, or it returns a manifest that cannot be parsed)
 is silently ignored and `repo sync` falls back to a regular ToT sync; pass
-`--verbose` to see the reason. This is only a fallback for the manifest-driven
-default: an explicit `-s`/`--smart-sync` or `-t`/`--smart-tag` still fails the
-sync, as before. Options that specify an explicit target or manifest source
-(such as `-t`/`--smart-tag`, `-m`/`--manifest-name` or
+`--verbose` to see the reason. Repo marks these requests as `best_effort` (see
+[GetApprovedManifestV2](#GetApprovedManifestV2)), so the server may also decline
+them, e.g. during a staged rollout. This is only a fallback for the
+manifest-driven default: an explicit `-s`/`--smart-sync` or `-t`/`--smart-tag`
+still fails the sync, as before. Options that specify an explicit target or
+manifest source (such as `-t`/`--smart-tag`, `-m`/`--manifest-name` or
 `--superproject-revision`) will also disable the default Smart Sync behavior.
 
 ### --smart-sync / -s
 
 Explicitly enables Smart Sync. Repo will call
-`GetApprovedManifest(branch[, target])`.
+`GetApprovedManifestV2({branch[, target]})`, or
+`GetApprovedManifest(branch[, target])` if the server does not implement it.
 
 The `branch` is determined by the current manifest branch as specified by
 `--manifest-branch=BRANCH` when running `repo init`.  If `${SYNC_BRANCH}` is

subcmds/sync.py

@@ -98,6 +98,69 @@ _BLOAT_SIZE_GARBAGE_THRESHOLD_KB = 1 * 1024 * 1024  # 1 GiB in KiB
 
 logger = RepoLogger(__file__)
 
+# Manifest server RPC that takes a single struct of named options.  Servers
+# that do not implement it are called via the positional GetApprovedManifest.
+_GET_APPROVED_MANIFEST_V2 = "GetApprovedManifestV2"
+
+
+def _IsMethodNotFoundFault(fault: xmlrpc.client.Fault, method: str) -> bool:
+    """Whether |fault| means the server does not implement |method|."""
+    if fault.faultCode == xmlrpc.client.METHOD_NOT_FOUND:
+        return True
+    # Python's SimpleXMLRPCDispatcher uses faultCode 1 with this message.
+    return f'method "{method}" is not supported' in str(fault.faultString)
+
+
+def _GetApprovedManifest(
+    server: xmlrpc.client.ServerProxy,
+    branch: str,
+    target: Optional[str],
+    best_effort: bool,
+) -> Tuple[bool, str]:
+    """Requests the approved manifest from the manifest server.
+
+    Args:
+        server: The manifest server proxy.
+        branch: The manifest branch.
+        target: The build target, if any.
+        best_effort: The client can proceed without a manifest (e.g. by
+            falling back to ToT), so the server may decline the request.
+
+    Returns:
+        A (success, manifest XML or error message) tuple.
+    """
+    request = {"branch": branch}
+    if target:
+        request["target"] = target
+    if best_effort:
+        request["best_effort"] = True
+
+    try:
+        response = server.GetApprovedManifestV2(request)
+    except xmlrpc.client.Fault as e:
+        if not _IsMethodNotFoundFault(e, _GET_APPROVED_MANIFEST_V2):
+            raise
+        # Older server: |best_effort| has no equivalent and is dropped.
+        if target:
+            success, manifest_str = server.GetApprovedManifest(branch, target)
+        else:
+            success, manifest_str = server.GetApprovedManifest(branch)
+        return success, manifest_str
+
+    if not isinstance(response, dict):
+        raise SmartSyncError(
+            f"error: invalid {_GET_APPROVED_MANIFEST_V2} response: {response!r}"
+        )
+    if response.get("success"):
+        manifest_xml = response.get("manifest_xml")
+        if not isinstance(manifest_xml, str):
+            raise SmartSyncError(
+                f"error: {_GET_APPROVED_MANIFEST_V2} response has invalid "
+                f"manifest_xml: {manifest_xml!r}"
+            )
+        return True, manifest_xml
+    return False, response.get("error_message", "unknown error")
+
 
 def _SafeCheckoutOrder(checkouts: List[Project]) -> List[List[Project]]:
     """Generate a sequence of checkouts that is safe to perform.
@@ -2094,6 +2157,7 @@ later is required to fix a server side protocol bug.
         opt: optparse.Values,
         smart_sync_manifest_path: str,
         manifest: XmlManifest,
+        best_effort: bool = False,
     ) -> str:
         if not manifest.manifest_server:
             raise SmartSyncError(
@@ -2140,12 +2204,9 @@ later is required to fix a server side protocol bug.
                         os.environ["TARGET_BUILD_VARIANT"],
                     )
 
-                if target:
-                    [success, manifest_str] = server.GetApprovedManifest(
-                        branch, target
-                    )
-                else:
-                    [success, manifest_str] = server.GetApprovedManifest(branch)
+                success, manifest_str = _GetApprovedManifest(
+                    server, branch, target, best_effort
+                )
             else:
                 assert opt.smart_tag
                 [success, manifest_str] = server.GetManifest(opt.smart_tag)
@@ -2519,7 +2580,10 @@ later is required to fix a server side protocol bug.
         if opt.smart_sync or opt.smart_tag:
             try:
                 manifest_name = self._SmartSyncSetup(
-                    opt, smart_sync_manifest_path, manifest
+                    opt,
+                    smart_sync_manifest_path,
+                    manifest,
+                    best_effort=implicit_smart_sync,
                 )
             except (SmartSyncError, ManifestParseError) as e:
                 if not implicit_smart_sync:

tests/test_subcmds_sync.py

@@ -20,10 +20,13 @@ import os
 from pathlib import Path
 import shutil
 import tempfile
+import threading
 import time
-from typing import Dict, List, Optional, Tuple, Type
+from typing import Any, Dict, Iterator, List, Optional, Tuple, Type
 import unittest
 from unittest import mock
+import xmlrpc.client
+import xmlrpc.server
 
 import pytest
 
@@ -1653,6 +1656,7 @@ class SyncCommand(unittest.TestCase):
                             ) as mock_warn:
                                 self.cmd.Execute(self.opt, [])
         mock_setup.assert_called_once()
+        self.assertTrue(mock_setup.call_args.kwargs.get("best_effort"))
         self.assertFalse(self.opt.smart_sync)
         self.assertFalse(mock_warn.called)
 
@@ -1689,9 +1693,11 @@ class SyncCommand(unittest.TestCase):
             self.cmd,
             "_SmartSyncSetup",
             side_effect=sync.SmartSyncError("unreachable"),
-        ):
+        ) as mock_setup:
             with self.assertRaises(sync.SmartSyncError):
                 self.cmd.Execute(self.opt, [])
+            mock_setup.assert_called_once()
+            self.assertFalse(mock_setup.call_args.kwargs.get("best_effort"))
 
     def test_implicit_smart_sync_unparsable_manifest(self) -> None:
         """Ensure an unparsable smart sync manifest falls back to ToT."""
@@ -2830,10 +2836,10 @@ class TestSmartSyncSetupRemoteHelper(unittest.TestCase):
 
         # Mock XML-RPC server call
         mock_server = mock.MagicMock()
-        mock_server.GetApprovedManifest.return_value = [
-            True,
-            "<manifest></manifest>",
-        ]
+        mock_server.GetApprovedManifestV2.return_value = {
+            "success": True,
+            "manifest_xml": "<manifest></manifest>",
+        }
         mock_server_class.return_value = mock_server
 
         # Mock manifest project branch
@@ -2885,7 +2891,10 @@ class TestSmartSyncSetupRemoteHelper(unittest.TestCase):
         self.manifest.manifest_server_helper = None
 
         mock_server = mock.MagicMock()
-        mock_server.GetApprovedManifest.return_value = [True, "<manifest>"]
+        mock_server.GetApprovedManifestV2.return_value = {
+            "success": True,
+            "manifest_xml": "<manifest>",
+        }
         mock_server_class.return_value = mock_server
 
         self.cmd._GetBranch = mock.MagicMock(return_value="main")
@@ -3065,10 +3074,10 @@ class TestSmartSyncSetupBranch(unittest.TestCase):
         self.manifest.manifest_server_helper = None
         self.smart_sync_manifest_path = "/fake/path/to/manifest.xml"
         self.mock_server = mock.MagicMock()
-        self.mock_server.GetApprovedManifest.return_value = [
-            True,
-            "<manifest></manifest>",
-        ]
+        self.mock_server.GetApprovedManifestV2.return_value = {
+            "success": True,
+            "manifest_xml": "<manifest></manifest>",
+        }
         self.cmd._GetBranch = mock.MagicMock(return_value="main")
         self.cmd._ReloadManifest = mock.MagicMock()
         self.cmd._ResolveManifestServerTransport = mock.MagicMock(
@@ -3089,14 +3098,16 @@ class TestSmartSyncSetupBranch(unittest.TestCase):
         self.cmd._GetBranch.assert_called_once_with(
             self.manifest.manifestProject
         )
-        self.mock_server.GetApprovedManifest.assert_called_once_with("main")
+        self.mock_server.GetApprovedManifestV2.assert_called_once_with(
+            {"branch": "main"}
+        )
 
     def test_sync_branch(self) -> None:
         """SYNC_BRANCH replaces the manifest branch."""
         self._RunSetup({"SYNC_BRANCH": "git_main-abfs"})
         self.cmd._GetBranch.assert_not_called()
-        self.mock_server.GetApprovedManifest.assert_called_once_with(
-            "git_main-abfs"
+        self.mock_server.GetApprovedManifestV2.assert_called_once_with(
+            {"branch": "git_main-abfs"}
         )
 
     def test_sync_branch_with_target(self) -> None:
@@ -3107,8 +3118,11 @@ class TestSmartSyncSetupBranch(unittest.TestCase):
                 "SYNC_TARGET": "cf_x86_64_phone-trunk_staging-eng",
             }
         )
-        self.mock_server.GetApprovedManifest.assert_called_once_with(
-            "git_main-abfs", "cf_x86_64_phone-trunk_staging-eng"
+        self.mock_server.GetApprovedManifestV2.assert_called_once_with(
+            {
+                "branch": "git_main-abfs",
+                "target": "cf_x86_64_phone-trunk_staging-eng",
+            }
         )
 
     def test_empty_sync_branch(self) -> None:
@@ -3117,4 +3131,138 @@ class TestSmartSyncSetupBranch(unittest.TestCase):
         self.cmd._GetBranch.assert_called_once_with(
             self.manifest.manifestProject
         )
-        self.mock_server.GetApprovedManifest.assert_called_once_with("main")
+        self.mock_server.GetApprovedManifestV2.assert_called_once_with(
+            {"branch": "main"}
+        )
+
+
+class GetApprovedManifestTests(unittest.TestCase):
+    """Tests for _GetApprovedManifest."""
+
+    def test_request_with_target_and_best_effort(self) -> None:
+        server = mock.MagicMock()
+        server.GetApprovedManifestV2.return_value = {
+            "success": True,
+            "manifest_xml": "<manifest/>",
+        }
+        result = sync._GetApprovedManifest(server, "main", "tgt", True)
+        self.assertEqual(result, (True, "<manifest/>"))
+        server.GetApprovedManifestV2.assert_called_once_with(
+            {"branch": "main", "target": "tgt", "best_effort": True}
+        )
+        server.GetApprovedManifest.assert_not_called()
+
+    def test_request_omits_unset_options(self) -> None:
+        server = mock.MagicMock()
+        server.GetApprovedManifestV2.return_value = {
+            "success": True,
+            "manifest_xml": "<manifest/>",
+        }
+        sync._GetApprovedManifest(server, "main", None, False)
+        server.GetApprovedManifestV2.assert_called_once_with({"branch": "main"})
+
+    def test_declined(self) -> None:
+        server = mock.MagicMock()
+        server.GetApprovedManifestV2.return_value = {
+            "success": False,
+            "error_message": "not in rollout",
+        }
+        result = sync._GetApprovedManifest(server, "main", None, True)
+        self.assertEqual(result, (False, "not in rollout"))
+
+    def test_invalid_response(self) -> None:
+        server = mock.MagicMock()
+        server.GetApprovedManifestV2.return_value = [True, "<manifest/>"]
+        with self.assertRaises(sync.SmartSyncError):
+            sync._GetApprovedManifest(server, "main", None, False)
+
+    def test_success_missing_manifest(self) -> None:
+        server = mock.MagicMock()
+        server.GetApprovedManifestV2.return_value = {"success": True}
+        with self.assertRaises(sync.SmartSyncError):
+            sync._GetApprovedManifest(server, "main", None, False)
+
+    def test_success_non_string_manifest(self) -> None:
+        server = mock.MagicMock()
+        for value in (None, 42, ["<manifest/>"]):
+            with self.subTest(manifest_xml=value):
+                server.GetApprovedManifestV2.return_value = {
+                    "success": True,
+                    "manifest_xml": value,
+                }
+                with self.assertRaises(sync.SmartSyncError):
+                    sync._GetApprovedManifest(server, "main", None, False)
+
+    def test_fallback_on_method_not_found_code(self) -> None:
+        server = mock.MagicMock()
+        server.GetApprovedManifestV2.side_effect = xmlrpc.client.Fault(
+            -32601, "no such method"
+        )
+        server.GetApprovedManifest.return_value = [True, "<manifest/>"]
+        result = sync._GetApprovedManifest(server, "main", "tgt", True)
+        self.assertEqual(result, (True, "<manifest/>"))
+        server.GetApprovedManifest.assert_called_once_with("main", "tgt")
+
+    def test_other_fault_is_not_retried(self) -> None:
+        server = mock.MagicMock()
+        server.GetApprovedManifestV2.side_effect = xmlrpc.client.Fault(
+            1, "internal error"
+        )
+        with self.assertRaises(xmlrpc.client.Fault):
+            sync._GetApprovedManifest(server, "main", "tgt", False)
+        server.GetApprovedManifest.assert_not_called()
+
+
+@contextlib.contextmanager
+def _LiveManifestServer(**funcs: Any) -> Iterator[xmlrpc.client.ServerProxy]:
+    """Runs a local XML-RPC server exposing |funcs|."""
+    server = xmlrpc.server.SimpleXMLRPCServer(
+        ("127.0.0.1", 0), logRequests=False
+    )
+    for name, func in funcs.items():
+        server.register_function(func, name)
+    thread = threading.Thread(target=server.serve_forever, daemon=True)
+    thread.start()
+    try:
+        host, port = server.server_address
+        yield xmlrpc.client.ServerProxy(f"http://{host}:{port}/")
+    finally:
+        server.shutdown()
+        server.server_close()
+        thread.join()
+
+
+class GetApprovedManifestLiveTests(unittest.TestCase):
+    """Tests _GetApprovedManifest against a real XML-RPC server."""
+
+    def test_legacy_server_fallback(self) -> None:
+        calls = []
+
+        def legacy(*args: str) -> List[Any]:
+            calls.append(args)
+            return [True, "<manifest/>"]
+
+        with _LiveManifestServer(GetApprovedManifest=legacy) as proxy:
+            self.assertEqual(
+                sync._GetApprovedManifest(proxy, "main", "tgt", True),
+                (True, "<manifest/>"),
+            )
+            self.assertEqual(
+                sync._GetApprovedManifest(proxy, "main", None, True),
+                (True, "<manifest/>"),
+            )
+        self.assertEqual(calls, [("main", "tgt"), ("main",)])
+
+    def test_v2_server(self) -> None:
+        requests = []
+
+        def v2(request: Dict[str, Any]) -> Dict[str, Any]:
+            requests.append(request)
+            return {"success": False, "error_message": "declined"}
+
+        with _LiveManifestServer(GetApprovedManifestV2=v2) as proxy:
+            self.assertEqual(
+                sync._GetApprovedManifest(proxy, "main", None, True),
+                (False, "declined"),
+            )
+        self.assertEqual(requests, [{"branch": "main", "best_effort": True}])