From c79aa602dc19ae66ca590cdac6935156e63fbad2 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 15 Jul 2021 07:30:22 -0600 Subject: [PATCH 1/2] Proper fix for setting timezone inappropriately in dryrun Reenable integration test for set of timezone. Don't set it while in dryrun. --- examples/autoinstall.yaml | 2 +- scripts/runtests.sh | 2 +- subiquity/server/controllers/timezone.py | 9 ++++++--- subiquity/tests/test_timezonecontroller.py | 11 ++++++++++- subiquitycore/tests/mocks.py | 3 +++ 5 files changed, 21 insertions(+), 6 deletions(-) diff --git a/examples/autoinstall.yaml b/examples/autoinstall.yaml index 605c86ff..51aff9e2 100644 --- a/examples/autoinstall.yaml +++ b/examples/autoinstall.yaml @@ -41,7 +41,7 @@ snaps: - name: etcd channel: 3.2/stable updates: all -# timezone: Pacific/Guam +timezone: Pacific/Guam storage: config: - {type: disk, ptable: gpt, path: /dev/vdb, wipe: superblock, preserve: false, grub_device: true, id: disk-1} diff --git a/scripts/runtests.sh b/scripts/runtests.sh index 3e175ef4..4ee31e7b 100755 --- a/scripts/runtests.sh +++ b/scripts/runtests.sh @@ -55,8 +55,8 @@ python3 scripts/check-yaml-fields.py .subiquity/subiquity-curtin-install.conf \ storage.config[-1].options='"errors=remount-ro"' python3 scripts/check-yaml-fields.py <(python3 scripts/check-yaml-fields.py .subiquity/etc/cloud/cloud.cfg.d/99-installer.cfg datasource.None.userdata_raw) \ locale='"en_GB.UTF-8"' \ + timezone='"Pacific/Guam"' \ 'snap.commands=[snap install --channel=3.2/stable etcd]' - # timezone='"Pacific/Guam"' grep -q 'finish: subiquity/Install/install/postinstall/install_package1: SUCCESS: installing package1' \ .subiquity/subiquity-server-debug.log grep -q 'finish: subiquity/Install/install/postinstall/install_package2: SUCCESS: installing package2' \ diff --git a/subiquity/server/controllers/timezone.py b/subiquity/server/controllers/timezone.py index 5eb2cec5..edeb73c5 100644 --- a/subiquity/server/controllers/timezone.py +++ b/subiquity/server/controllers/timezone.py @@ -31,8 +31,11 @@ def generate_possible_tzs(): return special_keys + real_tzs -def timedatectl_settz(tz): +def timedatectl_settz(app, tz): tzcmd = ['timedatectl', 'set-timezone', tz] + if app.opts.dry_run: + tzcmd = ['sleep', str(1/app.scale_factor)] + try: subprocess.run(tzcmd, universal_newlines=True) except subprocess.CalledProcessError as cpe: @@ -59,7 +62,7 @@ def timedatectl_gettz(): log.error('Failed to get live system timezone: %r', cpe) except IndexError: log.error('Failed to acquire system time zone') - log.debug('Failed to fine Time zone in timedatectl output') + log.debug('Failed to find Time zone in timedatectl output') return 'Etc/UTC' @@ -102,7 +105,7 @@ class TimeZoneController(SubiquityController): def set_system_timezone(self): if self.model.should_set_tz: - timedatectl_settz(self.model.timezone) + timedatectl_settz(self.app, self.model.timezone) async def GET(self) -> TimeZoneInfo: if self.model.timezone: diff --git a/subiquity/tests/test_timezonecontroller.py b/subiquity/tests/test_timezonecontroller.py index fff2ba53..04d58a9a 100644 --- a/subiquity/tests/test_timezonecontroller.py +++ b/subiquity/tests/test_timezonecontroller.py @@ -14,6 +14,7 @@ # along with this program. If not, see . import mock +import subprocess from subiquity.common.types import TimeZoneInfo from subiquity.models.timezone import TimeZoneModel @@ -81,7 +82,7 @@ class TestTimeZoneController(SubiTestCase): cloudconfig = {} if self.tzc.model.should_set_tz: cloudconfig = {'timezone': tz.timezone} - tdc_settz.assert_called_with(tz.timezone) + tdc_settz.assert_called_with(self.tzc.app, tz.timezone) self.assertEqual(cloudconfig, self.tzc.model.make_cloudconfig(), self.tzc.model) @@ -93,3 +94,11 @@ class TestTimeZoneController(SubiTestCase): for b in bads: with self.assertRaises(ValueError): self.tzc.deserialize(b) + + @mock.patch('subprocess.run') + @mock.patch('subiquity.server.controllers.timezone.timedatectl_gettz') + def test_set_tz_escape_dryrun(self, tdc_gettz, subprocess_run): + tdc_gettz.return_value = tz_utc + self.tzc.app.dry_run = True + self.tzc.deserialize('geoip') + self.assertEqual('sleep', subprocess_run.call_args.args[0][0]) diff --git a/subiquitycore/tests/mocks.py b/subiquitycore/tests/mocks.py index e27b0873..51572b33 100644 --- a/subiquitycore/tests/mocks.py +++ b/subiquitycore/tests/mocks.py @@ -39,4 +39,7 @@ def make_app(model=None): app.next_screen = mock.Mock() app.prev_screen = mock.Mock() app.hub = MessageHub() + app.opts = mock.Mock() + app.opts.dry_run = True + app.scale_factor = 1000 return app From 7808ccac84368552885131f1629dcd674e9c47b4 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 15 Jul 2021 11:07:53 -0600 Subject: [PATCH 2/2] GET /timezone no longer sets to system The original design for timezone intentionally set the system timezone based on geoip results, before a POST /timezone. After the feedback on LP: #1936310 I'm having second thoughts on that and wish for GET to be a simple informational query with no such side effects. --- subiquity/server/controllers/timezone.py | 13 +++++++------ subiquity/tests/test_timezonecontroller.py | 7 ++++++- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/subiquity/server/controllers/timezone.py b/subiquity/server/controllers/timezone.py index edeb73c5..64a0a611 100644 --- a/subiquity/server/controllers/timezone.py +++ b/subiquity/server/controllers/timezone.py @@ -108,16 +108,17 @@ class TimeZoneController(SubiquityController): timedatectl_settz(self.app, self.model.timezone) async def GET(self) -> TimeZoneInfo: + # if someone POSTed before, return that if self.model.timezone: return TimeZoneInfo(self.model.timezone, self.model.got_from_geoip) - # a bare call to GET() is equivalent to autoinstall "timezone: geoip" - self.deserialize('geoip') - tz = self.model.timezone - if not tz: - tz = timedatectl_gettz() - return TimeZoneInfo(tz, self.model.got_from_geoip) + # GET requests geoip results + if self.app.geoip.timezone: + return TimeZoneInfo(self.app.geoip.timezone, True) + + # geoip wasn't ready for some reason, so ask the system + return TimeZoneInfo(timedatectl_gettz(), False) async def POST(self, tz: str): self.deserialize(tz) diff --git a/subiquity/tests/test_timezonecontroller.py b/subiquity/tests/test_timezonecontroller.py index 04d58a9a..5fba2a47 100644 --- a/subiquity/tests/test_timezonecontroller.py +++ b/subiquity/tests/test_timezonecontroller.py @@ -14,7 +14,6 @@ # along with this program. If not, see . import mock -import subprocess from subiquity.common.types import TimeZoneInfo from subiquity.models.timezone import TimeZoneModel @@ -102,3 +101,9 @@ class TestTimeZoneController(SubiTestCase): self.tzc.app.dry_run = True self.tzc.deserialize('geoip') self.assertEqual('sleep', subprocess_run.call_args.args[0][0]) + + @mock.patch('subiquity.server.controllers.timezone.timedatectl_settz') + def test_get_tz_should_not_set(self, tdc_settz): + run_coro(self.tzc.GET()) + self.assertFalse(self.tzc.model.should_set_tz) + tdc_settz.assert_not_called()