From f159ce0f9ef5bce5a4866a677f0534e3b45a80e2 Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Wed, 7 Dec 2022 09:41:47 -0500 Subject: [PATCH] sync: fix manifest sync-j handling Since --jobs defaults to 0, not None, we never pull the value out of the manifest. Treat values of 0 and None the same to fix. Bug: http://b/239712300 Bug: http://b/260908907 Change-Id: I9b1026682072366616825fd72f90bd90c10a252f Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/354254 Tested-by: Mike Frysinger Reviewed-by: Josip Sokcevic Reviewed-by: Sam Saccone --- subcmds/sync.py | 71 ++++++++++++++++++++++---------------- tests/test_subcmds_sync.py | 47 +++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 29 deletions(-) diff --git a/subcmds/sync.py b/subcmds/sync.py index 0c0f0cf3..e4e7a971 100644 --- a/subcmds/sync.py +++ b/subcmds/sync.py @@ -1190,6 +1190,45 @@ later is required to fix a server side protocol bug. 'release. Use `--auto-gc` instead.', file=sys.stderr) opt.auto_gc = True + def _ValidateOptionsWithManifest(self, opt, mp): + """Like ValidateOptions, but after we've updated the manifest. + + Needed to handle sync-xxx option defaults in the manifest. + + Args: + opt: The options to process. + mp: The manifest project to pull defaults from. + """ + if not opt.jobs: + # If the user hasn't made a choice, use the manifest value. + opt.jobs = mp.manifest.default.sync_j + if opt.jobs: + # If --jobs has a non-default value, propagate it as the default for + # --jobs-xxx flags too. + if not opt.jobs_network: + opt.jobs_network = opt.jobs + if not opt.jobs_checkout: + opt.jobs_checkout = opt.jobs + else: + # Neither user nor manifest have made a choice, so setup defaults. + if not opt.jobs_network: + opt.jobs_network = 1 + if not opt.jobs_checkout: + opt.jobs_checkout = DEFAULT_LOCAL_JOBS + opt.jobs = os.cpu_count() + + # Try to stay under user rlimit settings. + # + # Since each worker requires at 3 file descriptors to run `git fetch`, use + # that to scale down the number of jobs. Unfortunately there isn't an easy + # way to determine this reliably as systems change, but it was last measured + # by hand in 2011. + soft_limit, _ = _rlimit_nofile() + jobs_soft_limit = max(1, (soft_limit - 5) // 3) + opt.jobs = min(opt.jobs, jobs_soft_limit) + opt.jobs_network = min(opt.jobs_network, jobs_soft_limit) + opt.jobs_checkout = min(opt.jobs_checkout, jobs_soft_limit) + def Execute(self, opt, args): manifest = self.outer_manifest if not opt.outer_manifest: @@ -1240,35 +1279,9 @@ later is required to fix a server side protocol bug. else: print('Skipping update of local manifest project.') - # Now that the manifests are up-to-date, setup the jobs value. - if opt.jobs is None: - # User has not made a choice, so use the manifest settings. - opt.jobs = mp.default.sync_j - if opt.jobs is not None: - # Neither user nor manifest have made a choice. - if opt.jobs_network is None: - opt.jobs_network = opt.jobs - if opt.jobs_checkout is None: - opt.jobs_checkout = opt.jobs - # Setup defaults if jobs==0. - if not opt.jobs: - if not opt.jobs_network: - opt.jobs_network = 1 - if not opt.jobs_checkout: - opt.jobs_checkout = DEFAULT_LOCAL_JOBS - opt.jobs = os.cpu_count() - - # Try to stay under user rlimit settings. - # - # Since each worker requires at 3 file descriptors to run `git fetch`, use - # that to scale down the number of jobs. Unfortunately there isn't an easy - # way to determine this reliably as systems change, but it was last measured - # by hand in 2011. - soft_limit, _ = _rlimit_nofile() - jobs_soft_limit = max(1, (soft_limit - 5) // 3) - opt.jobs = min(opt.jobs, jobs_soft_limit) - opt.jobs_network = min(opt.jobs_network, jobs_soft_limit) - opt.jobs_checkout = min(opt.jobs_checkout, jobs_soft_limit) + # Now that the manifests are up-to-date, setup options whose defaults might + # be in the manifest. + self._ValidateOptionsWithManifest(opt, mp) superproject_logging_data = {} self._UpdateProjectsRevisionId(opt, args, superproject_logging_data, diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py index 13f3f873..236d54e5 100644 --- a/tests/test_subcmds_sync.py +++ b/tests/test_subcmds_sync.py @@ -13,11 +13,13 @@ # limitations under the License. """Unittests for the subcmds/sync.py module.""" +import os import unittest from unittest import mock import pytest +import command from subcmds import sync @@ -43,6 +45,51 @@ def test_get_current_branch_only(use_superproject, cli_args, result): assert cmd._GetCurrentBranchOnly(opts, cmd.manifest) == result +# Used to patch os.cpu_count() for reliable results. +OS_CPU_COUNT = 24 + +@pytest.mark.parametrize('argv, jobs_manifest, jobs, jobs_net, jobs_check', [ + # No user or manifest settings. + ([], None, OS_CPU_COUNT, 1, command.DEFAULT_LOCAL_JOBS), + # No user settings, so manifest settings control. + ([], 3, 3, 3, 3), + # User settings, but no manifest. + (['--jobs=4'], None, 4, 4, 4), + (['--jobs=4', '--jobs-network=5'], None, 4, 5, 4), + (['--jobs=4', '--jobs-checkout=6'], None, 4, 4, 6), + (['--jobs=4', '--jobs-network=5', '--jobs-checkout=6'], None, 4, 5, 6), + (['--jobs-network=5'], None, OS_CPU_COUNT, 5, command.DEFAULT_LOCAL_JOBS), + (['--jobs-checkout=6'], None, OS_CPU_COUNT, 1, 6), + (['--jobs-network=5', '--jobs-checkout=6'], None, OS_CPU_COUNT, 5, 6), + # User settings with manifest settings. + (['--jobs=4'], 3, 4, 4, 4), + (['--jobs=4', '--jobs-network=5'], 3, 4, 5, 4), + (['--jobs=4', '--jobs-checkout=6'], 3, 4, 4, 6), + (['--jobs=4', '--jobs-network=5', '--jobs-checkout=6'], 3, 4, 5, 6), + (['--jobs-network=5'], 3, 3, 5, 3), + (['--jobs-checkout=6'], 3, 3, 3, 6), + (['--jobs-network=5', '--jobs-checkout=6'], 3, 3, 5, 6), + # Settings that exceed rlimits get capped. + (['--jobs=1000000'], None, 83, 83, 83), + ([], 1000000, 83, 83, 83), +]) +def test_cli_jobs(argv, jobs_manifest, jobs, jobs_net, jobs_check): + """Tests --jobs option behavior.""" + mp = mock.MagicMock() + mp.manifest.default.sync_j = jobs_manifest + + cmd = sync.Sync() + opts, args = cmd.OptionParser.parse_args(argv) + cmd.ValidateOptions(opts, args) + + with mock.patch.object(sync, '_rlimit_nofile', return_value=(256, 256)): + with mock.patch.object(os, 'cpu_count', return_value=OS_CPU_COUNT): + cmd._ValidateOptionsWithManifest(opts, mp) + assert opts.jobs == jobs + assert opts.jobs_network == jobs_net + assert opts.jobs_checkout == jobs_check + + class GetPreciousObjectsState(unittest.TestCase): """Tests for _GetPreciousObjectsState."""