Skip to content
This repository was archived by the owner on Oct 10, 2020. It is now read-only.

requires root privileges and touches /var during build #966

Closed
baude wants to merge 6 commits into from
Closed

requires root privileges and touches /var during build #966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fd66e39
util: Don't try to create directories at module import time
baude Apr 12, 2017
e0dfdb3
fixup! util: Don't try to create directories at module import time
cgwalters Apr 12, 2017
65362e6
fixup! util: Don't try to create directories at module import time
cgwalters Apr 12, 2017
60f3d89
fixup! util: Don't try to create directories at module import time
cgwalters Apr 12, 2017
4e6de1f
fixup! util: Don't try to create directories at module import time
cgwalters Apr 13, 2017
8d9f4db
fixup! util: Don't try to create directories at module import time
cgwalters Apr 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 39 additions & 63 deletions Atomic/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import json
import subprocess
import collections
from contextlib import contextmanager
from fnmatch import fnmatch as matches
import os
import selinux
Expand Down Expand Up @@ -33,8 +34,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"
Expand Down Expand Up @@ -802,102 +801,79 @@ 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)))

# 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
:return:
'''
time_out = 0
f_lock = False
@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(install_data_file, fcntl.LOCK_EX | fcntl.LOCK_NB)
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)

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
Copy link
Copy Markdown
Contributor

@jlebon jlebon Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we yield the file handle here, so that... (continued in next comment)

# Now unlock
fcntl.flock(f, fcntl.LOCK_UN)

# 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):
try:
return json.loads(file_handle.read())
except ValueError:
return {}

@classmethod
def _write_install_data(cls, new_data):
install_data = cls._read_install_data(cls.install_file_handle)
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()
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)
return read_data
return {}
with file_lock(ATOMIC_INSTALL_JSON):
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
@file_lock
def write_install_data(cls, new_data):
cls._write_install_data(new_data)
install_data = cls.read_install_data()
with file_lock(ATOMIC_INSTALL_JSON):
Copy link
Copy Markdown
Contributor

@jlebon jlebon Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we can do the whole read, update, write under lock?

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):
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
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(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:
if not ignore:
raise ValueError(str(e))
return
del install_data[id_key]
Copy link
Copy Markdown
Contributor

@jlebon jlebon Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this won't take effect, right? The write methods (both old and new) are merging entries, so can deletes actually occur?

Copy link
Copy Markdown
Member

@cgwalters cgwalters Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, but that's not a regression from this patch.

I could think of many more things to do to improve this, not least of which is moving it to its own file with documentation, and such...but can we do that kind of thing in a separate PR?

I'd really like to get FAHC going again.

(And really, do we even really need this InstallData thing? Can't we (for system containers) use the ostree commit metadata?)

return cls._write_install_data(install_data)
return cls.write_install_data(install_data)

@classmethod
def image_installed(cls, img_object):
Expand Down
7 changes: 0 additions & 7 deletions tests/unit/test_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -187,22 +185,19 @@ 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()
MockIO.grow_data('new_data_fq', 'docker.io/library/centos:latest')
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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down