From 3448df395478c2375d880e33bfc705a9ed36272b Mon Sep 17 00:00:00 2001 From: Rahul Yadav Date: Thu, 1 Oct 2026 16:34:47 +0000 Subject: [PATCH] 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 Commit-Queue: Rahul Yadav Tested-by: Rahul Yadav --- docs/smart-sync.md | 43 ++++++++- subcmds/sync.py | 78 ++++++++++++++-- tests/test_subcmds_sync.py | 182 +++++++++++++++++++++++++++++++++---- 3 files changed, 274 insertions(+), 29 deletions(-) diff --git a/docs/smart-sync.md b/docs/smart-sync.md index 2da4b90e5..24f37461b 100644 --- a/docs/smart-sync.md +++ b/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 diff --git a/subcmds/sync.py b/subcmds/sync.py index 8d4896d0f..e83f42345 100644 --- a/subcmds/sync.py +++ b/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: diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py index 8beb6b8e2..e7e8013e0 100644 --- a/tests/test_subcmds_sync.py +++ b/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, - "", - ] + mock_server.GetApprovedManifestV2.return_value = { + "success": True, + "manifest_xml": "", + } 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, ""] + mock_server.GetApprovedManifestV2.return_value = { + "success": True, + "manifest_xml": "", + } 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, - "", - ] + self.mock_server.GetApprovedManifestV2.return_value = { + "success": True, + "manifest_xml": "", + } 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": "", + } + result = sync._GetApprovedManifest(server, "main", "tgt", True) + self.assertEqual(result, (True, "")) + 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": "", + } + 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, ""] + 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, [""]): + 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, ""] + result = sync._GetApprovedManifest(server, "main", "tgt", True) + self.assertEqual(result, (True, "")) + 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, ""] + + with _LiveManifestServer(GetApprovedManifest=legacy) as proxy: + self.assertEqual( + sync._GetApprovedManifest(proxy, "main", "tgt", True), + (True, ""), + ) + self.assertEqual( + sync._GetApprovedManifest(proxy, "main", None, True), + (True, ""), + ) + 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}])