From fd66e3961124c9f4c8579f4e073ed01c5f0e08e9 Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Wed, 12 Apr 2017 10:20:55 -0500 Subject: [PATCH 1/6] util: Don't try to create directories at module import time We shouldn't do anything at module import time, as that happens during builds, where we don't want to touch the host system (and may not have privileges to do so). This required some refactoring of the locking code too, as simply instantiating the decorator was creating files too. Closes: https://github.com/projectatomic/atomic/issues/963 --- Atomic/util.py | 64 +++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/Atomic/util.py b/Atomic/util.py index fff87179..eeaa8a66 100644 --- a/Atomic/util.py +++ b/Atomic/util.py @@ -33,8 +33,6 @@ ATOMIC_CONFD = os.environ.get('ATOMIC_CONFD', '/etc/atomic.d/') ATOMIC_LIBEXEC = os.environ.get('ATOMIC_LIBEXEC', '/usr/libexec/atomic') ATOMIC_VAR_LIB = os.environ.get('ATOMIC_VAR_LIB', '/var/lib/atomic') -if not os.path.exists(ATOMIC_VAR_LIB): - os.makedirs(ATOMIC_VAR_LIB) ATOMIC_INSTALL_JSON = os.environ.get('ATOMIC_INSTALL_JSON', os.path.join(ATOMIC_VAR_LIB, 'install.json')) GOMTREE_PATH = "/usr/bin/gomtree" @@ -805,12 +803,6 @@ def load_scan_result_file(file_name): def file_lock(func): lock_file_name = "{}.lock".format(os.path.join(os.path.dirname(ATOMIC_INSTALL_JSON), "." + os.path.basename(ATOMIC_INSTALL_JSON))) - # Create the temporary lockfile if it doesn't exist - if not os.path.exists(lock_file_name): - open(lock_file_name, 'a').close() - - install_data_file = open(lock_file_name, "r") - def get_lock(): ''' Obtains a read-only file lock on the install data @@ -818,19 +810,25 @@ def get_lock(): ''' time_out = 0 f_lock = False - while time_out < 10.5: # Ten second attempt to get a lock - try: - fcntl.flock(install_data_file, fcntl.LOCK_EX | fcntl.LOCK_NB) - f_lock = True - break - except IOError: - time.sleep(.5) - time_out += .5 + # Create the temporary lockfile if it doesn't exist + if not os.path.exists(lock_file_name): + open(lock_file_name, 'a').close() + + with open(lock_file_name, "r") as f: + while time_out < 10.5: # Ten second attempt to get a lock + try: + fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB) + f_lock = True + break + except IOError: + time.sleep(.5) + time_out += .5 if not f_lock: raise ValueError("Unable to get file lock for {}".format(ATOMIC_INSTALL_JSON)) def release_lock(): - fcntl.flock(install_data_file, fcntl.LOCK_UN) + with open(lock_file_name, "r") as f: + fcntl.flock(f, fcntl.LOCK_UN) def wrapper(*args, **kwargs): get_lock() @@ -841,34 +839,40 @@ def wrapper(*args, **kwargs): return wrapper +# Records additional data for containers outside of the native storage (docker/ostree) class InstallData(object): - if not os.path.exists(ATOMIC_INSTALL_JSON): - open(ATOMIC_INSTALL_JSON, 'a').close() - - install_file_handle = open(ATOMIC_INSTALL_JSON, 'r') - @staticmethod - def _read_install_data(file_handle): + @classmethod + def _read_install_data(cls): try: - return json.loads(file_handle.read()) - except ValueError: - return {} + with open(ATOMIC_INSTALL_JSON, 'r') as f: + # Backwards compatibilty - we previously created an empty file explicitly; + # see https://github.com/projectatomic/atomic/pull/966 + if os.fstat(f.fileno()).st_size == 0: + return {} + return json.load(f) + except IOError as e: + if e.errno == errno.ENOENT: + return {} + raise e @classmethod def _write_install_data(cls, new_data): - install_data = cls._read_install_data(cls.install_file_handle) + install_data = cls._read_install_data() for x in new_data: install_data[x] = new_data[x] temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False) json.dump(install_data, temp_file) temp_file.close() + if not os.path.exists(ATOMIC_VAR_LIB): + os.makedirs(ATOMIC_VAR_LIB) shutil.move(temp_file.name, ATOMIC_INSTALL_JSON) @classmethod @file_lock def read_install_data(cls): if os.path.exists(ATOMIC_INSTALL_JSON): - read_data = cls._read_install_data(cls.install_file_handle) + read_data = cls._read_install_data() return read_data return {} @@ -880,7 +884,7 @@ def write_install_data(cls, new_data): @classmethod def get_install_name_by_id(cls, iid, install_data=None): if not install_data: - install_data = cls._read_install_data(cls.install_file_handle) + install_data = cls._read_install_data() for installed_image in install_data: if install_data[installed_image]['id'] == iid: return installed_image @@ -889,7 +893,7 @@ def get_install_name_by_id(cls, iid, install_data=None): @classmethod @file_lock def delete_by_id(cls, iid, ignore=False): - install_data = cls._read_install_data(cls.install_file_handle) + install_data = cls._read_install_data() try: id_key = InstallData.get_install_name_by_id(iid, install_data=install_data) except ValueError as e: From e0dfdb32b55a7157a572cdbe226ee769cf774bb4 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 12 Apr 2017 15:42:37 -0400 Subject: [PATCH 2/6] fixup! util: Don't try to create directories at module import time --- Atomic/util.py | 67 ++++++++++++++++++-------------------------------- 1 file changed, 24 insertions(+), 43 deletions(-) diff --git a/Atomic/util.py b/Atomic/util.py index eeaa8a66..f1f69567 100644 --- a/Atomic/util.py +++ b/Atomic/util.py @@ -6,6 +6,7 @@ import json import subprocess import collections +from contextlib import contextmanager from fnmatch import fnmatch as matches import os import selinux @@ -800,44 +801,27 @@ def load_scan_result_file(file_name): return json.loads(open(os.path.join(file_name), "r").read()) -def file_lock(func): - lock_file_name = "{}.lock".format(os.path.join(os.path.dirname(ATOMIC_INSTALL_JSON), "." + os.path.basename(ATOMIC_INSTALL_JSON))) - - def get_lock(): - ''' - Obtains a read-only file lock on the install data - :return: - ''' - time_out = 0 - f_lock = False - # Create the temporary lockfile if it doesn't exist - if not os.path.exists(lock_file_name): - open(lock_file_name, 'a').close() - - with open(lock_file_name, "r") as f: - while time_out < 10.5: # Ten second attempt to get a lock - try: - fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB) - f_lock = True - break - except IOError: - time.sleep(.5) - time_out += .5 +@contextmanager +def file_lock(path): + lock_file_name = "{}.lock".format(path) + time_out = 0 + f_lock = False + with open(lock_file_name, "a") as f: + while time_out < 10.5: # Ten second attempt to get a lock + try: + fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB) + f_lock = True + break + except IOError: + time.sleep(.5) + time_out += .5 if not f_lock: - raise ValueError("Unable to get file lock for {}".format(ATOMIC_INSTALL_JSON)) - - def release_lock(): - with open(lock_file_name, "r") as f: - fcntl.flock(f, fcntl.LOCK_UN) - - def wrapper(*args, **kwargs): - get_lock() - ret = func(*args, **kwargs) - release_lock() - return ret - - return wrapper + raise ValueError("Unable to get file lock for {}".format(lock_file_name)) + # Call the user code + yield + # Now unlock + fcntl.flock(f, fcntl.LOCK_UN) # Records additional data for containers outside of the native storage (docker/ostree) class InstallData(object): @@ -869,17 +853,14 @@ def _write_install_data(cls, new_data): shutil.move(temp_file.name, ATOMIC_INSTALL_JSON) @classmethod - @file_lock def read_install_data(cls): - if os.path.exists(ATOMIC_INSTALL_JSON): - read_data = cls._read_install_data() - return read_data - return {} + with file_lock(ATOMIC_INSTALL_JSON): + return cls._read_install_data() @classmethod - @file_lock def write_install_data(cls, new_data): - cls._write_install_data(new_data) + with file_lock(ATOMIC_INSTALL_JSON): + cls._write_install_data(new_data) @classmethod def get_install_name_by_id(cls, iid, install_data=None): From 65362e604013333f57d5324aa8b81721b9013a8b Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 12 Apr 2017 15:48:23 -0400 Subject: [PATCH 3/6] fixup! util: Don't try to create directories at module import time --- Atomic/util.py | 49 ++++++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/Atomic/util.py b/Atomic/util.py index f1f69567..6ca2f02c 100644 --- a/Atomic/util.py +++ b/Atomic/util.py @@ -826,41 +826,33 @@ def file_lock(path): # Records additional data for containers outside of the native storage (docker/ostree) class InstallData(object): - @classmethod - def _read_install_data(cls): - try: - with open(ATOMIC_INSTALL_JSON, 'r') as f: - # Backwards compatibilty - we previously created an empty file explicitly; - # see https://github.com/projectatomic/atomic/pull/966 - if os.fstat(f.fileno()).st_size == 0: - return {} - return json.load(f) - except IOError as e: - if e.errno == errno.ENOENT: - return {} - raise e - - @classmethod - def _write_install_data(cls, new_data): - install_data = cls._read_install_data() - for x in new_data: - install_data[x] = new_data[x] - temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False) - json.dump(install_data, temp_file) - temp_file.close() - if not os.path.exists(ATOMIC_VAR_LIB): - os.makedirs(ATOMIC_VAR_LIB) - shutil.move(temp_file.name, ATOMIC_INSTALL_JSON) - @classmethod def read_install_data(cls): with file_lock(ATOMIC_INSTALL_JSON): - return cls._read_install_data() + try: + with open(ATOMIC_INSTALL_JSON, 'r') as f: + # Backwards compatibilty - we previously created an empty file explicitly; + # see https://github.com/projectatomic/atomic/pull/966 + if os.fstat(f.fileno()).st_size == 0: + return {} + return json.load(f) + except IOError as e: + if e.errno == errno.ENOENT: + return {} + raise e @classmethod def write_install_data(cls, new_data): with file_lock(ATOMIC_INSTALL_JSON): - cls._write_install_data(new_data) + install_data = cls._read_install_data() + for x in new_data: + install_data[x] = new_data[x] + temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False) + json.dump(install_data, temp_file) + temp_file.close() + if not os.path.exists(ATOMIC_VAR_LIB): + os.makedirs(ATOMIC_VAR_LIB) + shutil.move(temp_file.name, ATOMIC_INSTALL_JSON) @classmethod def get_install_name_by_id(cls, iid, install_data=None): @@ -872,7 +864,6 @@ def get_install_name_by_id(cls, iid, install_data=None): raise ValueError("Unable to find {} in installed image data ({}). Re-run command with -i to ignore".format(id, ATOMIC_INSTALL_JSON)) @classmethod - @file_lock def delete_by_id(cls, iid, ignore=False): install_data = cls._read_install_data() try: From 60f3d891f12a41a1e05dbe036201edf3093dd761 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 12 Apr 2017 16:17:10 -0400 Subject: [PATCH 4/6] fixup! util: Don't try to create directories at module import time --- Atomic/util.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Atomic/util.py b/Atomic/util.py index 6ca2f02c..c73eb778 100644 --- a/Atomic/util.py +++ b/Atomic/util.py @@ -843,8 +843,8 @@ def read_install_data(cls): @classmethod def write_install_data(cls, new_data): + install_data = cls.read_install_data() with file_lock(ATOMIC_INSTALL_JSON): - install_data = cls._read_install_data() for x in new_data: install_data[x] = new_data[x] temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False) @@ -857,7 +857,7 @@ def write_install_data(cls, new_data): @classmethod def get_install_name_by_id(cls, iid, install_data=None): if not install_data: - install_data = cls._read_install_data() + install_data = cls.read_install_data() for installed_image in install_data: if install_data[installed_image]['id'] == iid: return installed_image @@ -865,7 +865,7 @@ def get_install_name_by_id(cls, iid, install_data=None): @classmethod def delete_by_id(cls, iid, ignore=False): - install_data = cls._read_install_data() + install_data = cls.read_install_data() try: id_key = InstallData.get_install_name_by_id(iid, install_data=install_data) except ValueError as e: @@ -873,7 +873,7 @@ def delete_by_id(cls, iid, ignore=False): raise ValueError(str(e)) return del install_data[id_key] - return cls._write_install_data(install_data) + return cls.write_install_data(install_data) @classmethod def image_installed(cls, img_object): From 4e6de1fe217af9160e3a5e822bde0919e26fa536 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 13 Apr 2017 10:04:36 -0400 Subject: [PATCH 5/6] fixup! util: Don't try to create directories at module import time --- tests/unit/test_util.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/unit/test_util.py b/tests/unit/test_util.py index ea40a8b7..ad8126df 100644 --- a/tests/unit/test_util.py +++ b/tests/unit/test_util.py @@ -170,14 +170,12 @@ def __init__(self): self.image = None @patch('Atomic.util.InstallData.read_install_data', new=MockIO.read_mock) - @patch('Atomic.util.InstallData._read_install_data', new=MockIO.read_mock) @patch('Atomic.util.InstallData.write_install_data', new=MockIO.write_mock) def test_read(self): MockIO.reset_data() self.assertEqual(util.InstallData.read_install_data(), MockIO.install_data) @patch('Atomic.util.InstallData.read_install_data', new=MockIO.read_mock) - @patch('Atomic.util.InstallData._read_install_data', new=MockIO.read_mock) @patch('Atomic.util.InstallData.write_install_data', new=MockIO.write_mock) def test_write(self): MockIO.reset_data() @@ -187,7 +185,6 @@ def test_write(self): self.assertTrue('docker.io/library/centos:latest' in util.InstallData.read_install_data()) @patch('Atomic.util.InstallData.read_install_data', new=MockIO.read_mock) - @patch('Atomic.util.InstallData._read_install_data', new=MockIO.read_mock) @patch('Atomic.util.InstallData.write_install_data', new=MockIO.write_mock) def test_get_install_name_by_id(self): MockIO.reset_data() @@ -195,14 +192,12 @@ def test_get_install_name_by_id(self): self.assertEqual(util.InstallData.get_install_name_by_id('16e9fdecc1febc87fb1ca09271009cf5f28eb8d4aec5515922ef298c145a6726', install_data=MockIO.install_data), 'docker.io/library/centos:latest') @patch('Atomic.util.InstallData.read_install_data', new=MockIO.read_mock) - @patch('Atomic.util.InstallData._read_install_data', new=MockIO.read_mock) @patch('Atomic.util.InstallData.write_install_data', new=MockIO.write_mock) def test_fail_get_install_name_by_id(self): MockIO.reset_data() self.assertRaises(ValueError, util.InstallData.get_install_name_by_id, 1, MockIO.install_data) @patch('Atomic.util.InstallData.read_install_data', new=MockIO.read_mock) - @patch('Atomic.util.InstallData._read_install_data', new=MockIO.read_mock) @patch('Atomic.util.InstallData.write_install_data', new=MockIO.write_mock) def test_image_installed_name(self): MockIO.reset_data() @@ -216,7 +211,6 @@ def test_image_installed_name(self): self.assertTrue(util.InstallData.image_installed(local_image_object)) @patch('Atomic.util.InstallData.read_install_data', new=MockIO.read_mock) - @patch('Atomic.util.InstallData._read_install_data', new=MockIO.read_mock) @patch('Atomic.util.InstallData.write_install_data', new=MockIO.write_mock) def test_image_installed_id(self): MockIO.reset_data() @@ -230,7 +224,6 @@ def test_image_installed_id(self): self.assertTrue(util.InstallData.image_installed(local_image_object)) @patch('Atomic.util.InstallData.read_install_data', new=MockIO.read_mock) - @patch('Atomic.util.InstallData._read_install_data', new=MockIO.read_mock) @patch('Atomic.util.InstallData.write_install_data', new=MockIO.write_mock) def test_image_not_installed(self): MockIO.reset_data() From 8d9f4db3d6e7a2f1bef835fca65126b064fb94df Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 13 Apr 2017 13:49:35 -0400 Subject: [PATCH 6/6] fixup! util: Don't try to create directories at module import time --- Atomic/util.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Atomic/util.py b/Atomic/util.py index c73eb778..7cd9dc60 100644 --- a/Atomic/util.py +++ b/Atomic/util.py @@ -847,12 +847,12 @@ def write_install_data(cls, new_data): with file_lock(ATOMIC_INSTALL_JSON): for x in new_data: install_data[x] = new_data[x] - temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False) - json.dump(install_data, temp_file) - temp_file.close() - if not os.path.exists(ATOMIC_VAR_LIB): - os.makedirs(ATOMIC_VAR_LIB) - shutil.move(temp_file.name, ATOMIC_INSTALL_JSON) + temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False) + json.dump(install_data, temp_file) + temp_file.close() + if not os.path.exists(ATOMIC_VAR_LIB): + os.makedirs(ATOMIC_VAR_LIB) + shutil.move(temp_file.name, ATOMIC_INSTALL_JSON) @classmethod def get_install_name_by_id(cls, iid, install_data=None):