requires root privileges and touches /var during build #966

baude · 2017-04-06T18:47:45Z

Fixes issue #963

#963

baude · 2017-04-06T19:22:11Z

bot, retest this please

baude · 2017-04-06T20:13:49Z

bot, retest this please

rhatdan · 2017-04-10T15:22:28Z

Tests still failing?

rhatdan · 2017-04-10T15:24:11Z

LGTM if you fix tests failing.

baude · 2017-04-11T17:28:49Z

bot, retest this please

cgwalters · 2017-04-11T18:17:40Z

Atomic/util.py

+            os.makedirs(ATOMIC_VAR_LIB)
+
+        if not os.path.exists(ATOMIC_INSTALL_JSON):
+            open(ATOMIC_INSTALL_JSON, 'a').close()


I don't understand why we create an empty file explicitly. It's not valid JSON. Why not simply have "absence of file" = empty? Basically we don't explicitly write it when we're just opening for read.

jlebon · 2017-04-11T19:13:05Z

Atomic/util.py

+        if not os.path.exists(lock_file_name):
+            open(lock_file_name, 'a').close()
+
+        return open(lock_file_name, "r")


Won't this leak though? Don't we want to close() in release_lock()?

jlebon · 2017-04-11T19:13:33Z

Atomic/util.py

+        if not os.path.exists(ATOMIC_INSTALL_JSON):
+            open(ATOMIC_INSTALL_JSON, 'a').close()
+
+        return open(ATOMIC_INSTALL_JSON, 'r')


jlebon · 2017-04-12T16:33:22Z

Atomic/util.py

-        install_data = cls._read_install_data(cls.install_file_handle)
+        try:
+            install_data = cls._read_install_data()
+        except IOError:


This is already caught by _read_install_data, no? So you should already be getting {} back in that case.

jlebon · 2017-04-12T16:44:03Z

Atomic/util.py


    @staticmethod
-    def _read_install_data(file_handle):
+    def get_install_file_handle():


Does this need to be a function? Seems like we can just merge it into _read_install_data() so that you can just use context managers there to open and close, e.g.:

try: with open(ATOMIC_INSTALL_JSON, 'r') as f: return json.load(f) except IOError as e: if e.errno == errno.ENOENT: return {} raise e

?

Agree with this, except I think for backcompat with the previous "empty file" model we should make it something like:

try: with open(ATOMIC_INSTALL_JSON, 'r') as f: 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

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: projectatomic#963

cgwalters · 2017-04-12T18:44:03Z

I went ahead and patched things up as above - there were more fixes required in the decorator, and I didn't run any tests other than simply doing as non-root:

$ python setup.py build

We clearly need to tweak the redhat-ci default build process to be more mock-like.

jlebon · 2017-04-12T19:00:58Z

Atomic/util.py

+        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)


Won't this lock be released when f is closed though?
Just tried this out:

with open("lockfile", "r") as f: fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB) with open("lockfile", "r") as f: fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB)

which works without issue. Whereas

f = open("lockfile", "r") fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB) g = open("lockfile", "r") fcntl.flock(g, fcntl.LOCK_EX | fcntl.LOCK_NB)

throws a BlockingIOError as expected.

OK, let's try making it a context manager. There is also https://pypi.python.org/pypi/flockcontext ...dunno about adding the dep.

cgwalters · 2017-04-13T14:31:31Z

OK, this passes the tests now.

jlebon · 2017-04-13T15:29:19Z

Atomic/util.py


+        # Call the user code
+        yield


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

jlebon · 2017-04-13T15:31:13Z

Atomic/util.py

    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):


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

jlebon · 2017-04-13T15:33:14Z

Atomic/util.py

    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]


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

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?)

jlebon · 2017-04-13T15:34:28Z

Atomic/util.py

+        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)


Seems like this line and the rest below are supposed to be unindented by one level?

Pushed a fixup for that ⬇️

cgwalters · 2017-04-13T18:02:41Z

As far as I can tell right now, the only place that reads (not writes) InstallData right now is:

        if iobject.get_label('INSTALL') and not args.ignore and not util.InstallData.image_installed(iobject):

And that's just for a warning. How about we just delete this code? I'd like to deprecate the INSTALL label anyways in favor of the system container approach.

rhatdan · 2017-04-13T18:27:07Z

I am fine with moving that way.

baude · 2017-04-13T18:34:16Z

@rhatdan you are in favor not honoring the INSTALL label? What's going to happen to all the images already out there?

This reverts: projectatomic#950 See: projectatomic#966 The idea here originally was good, but we have to be a *lot* more careful in introducing data stores than this. They need "schema design", documentation, and most importantly IMO: lifecycle coupling. If I e.g. `rm -rf /var/lib/docker` (as a lot of documentation tells people to do, this install data would stick around. Similarly for deleting content directly in `/ostree/repo`. You could say that's a bug, but it'll happen. The best way to do metadata is to use the underlying store. OSTree in particular already has an API for storing metadata. Any containers we write there should use that. I'm not sure offhand if we can do something similar with Docker. But basically I don't think it's worth carrying another data store *just* to show a warning.

rhatdan · 2017-04-13T18:36:26Z

I am fine with de-emphasizing it, not removing it.

cgwalters · 2017-04-13T18:36:43Z

#969

cgwalters · 2017-04-13T18:42:12Z

A good chunk of the INSTALL use cases should be system containers. The rest I think are more in the system extensions case - and there the underlying data store tracking content is RPM. So in neither case do we need our own data store. Right?

baude · 2017-04-13T18:45:00Z

so the current RHEL rsyslog container is not a use case? same with the openscap image? We have spent years trying to get people to use labels. Maybe I am missing something.

cgwalters · 2017-04-13T18:55:28Z

Yeah, we need to make the rsyslog container a system container.

rhatdan · 2017-04-13T18:58:15Z

I think we will need to continue to support INSTALL Labels but we really want to move most cases where an app is dropping content on the host to use something like system containers. INSTALL Labels begin to fall apart when you don't require docker.

cgwalters · 2017-04-13T19:03:07Z

INSTALL also forces the container author to own problems like SELinux labeling, handling any conflicts with files they're trying to install. And the existence of the UNINSTALL label gets into deep issues around things like "what happens if I pull a newer version of the container with a different UNINSTALL?

Basically INSTALL was a good start at having something but I think we're past the prototype stage and need to have a more rigorous model. Basically, rigorous = using RPM to track installed files and handle config files in /etc. That way one centralized place is also taking care of SELinux labeling as well. We already have a database for content installed on the host - RPM.

cgwalters · 2017-04-13T19:06:15Z

There were things blocking this before, like rpm-ostree not supporting local RPM installs, etc. that we've fixed now. Though to really complete that picture we need livefs which I'm working on.

cgwalters · 2017-04-13T19:06:48Z

Anyways I think this PR is in an OK state - it could be a lot better but I'd really like to just get FAHC going and revisit this in later PRs.

baude · 2017-04-13T19:23:05Z

LGTM

rhatdan · 2017-04-13T19:29:19Z

@rh-atomic-bot r+ 4e6de1f

rh-atomic-bot · 2017-04-13T19:29:20Z

🙀 4e6de1f is not a valid commit SHA. Please try again with 8d9f4db.

rhatdan · 2017-04-13T19:29:37Z

@rh-atomic-bot r+ 8d9f4db

rh-atomic-bot · 2017-04-13T19:29:41Z

⌛ Testing commit 8d9f4db with merge 12d674f...

cgwalters · 2017-04-13T19:33:32Z

Note CI is currently down projectatomic/papr#33

rh-atomic-bot · 2017-04-13T19:51:11Z

☀️ Test successful - status-redhatci
Approved by: rhatdan
Pushing 12d674f to master...

baude mentioned this pull request Apr 6, 2017

requires root privileges and touches /var during build #963

Closed

cgwalters reviewed Apr 11, 2017

View reviewed changes

jlebon reviewed Apr 11, 2017

View reviewed changes

baude force-pushed the root_issue branch from 47950df to f5a97d4 Compare April 12, 2017 15:23

jlebon reviewed Apr 12, 2017

View reviewed changes

cgwalters force-pushed the root_issue branch from f5a97d4 to fd66e39 Compare April 12, 2017 18:43

jlebon reviewed Apr 12, 2017

View reviewed changes

cgwalters added 4 commits April 12, 2017 15:42

fixup! util: Don't try to create directories at module import time

e0dfdb3

fixup! util: Don't try to create directories at module import time

65362e6

fixup! util: Don't try to create directories at module import time

60f3d89

fixup! util: Don't try to create directories at module import time

4e6de1f

jlebon reviewed Apr 13, 2017

View reviewed changes

fixup! util: Don't try to create directories at module import time

8d9f4db

cgwalters mentioned this pull request Apr 13, 2017

Stop reading/writing "install data" in /var/lib/atomic #969

Closed

jlebon mentioned this pull request Apr 13, 2017

PR testing doesn't happen if the message bus is down projectatomic/papr#33

Closed

rh-atomic-bot closed this in 12d674f Apr 13, 2017

requires root privileges and touches /var during build #966

requires root privileges and touches /var during build #966

Conversation

baude commented Apr 6, 2017

baude commented Apr 6, 2017

baude commented Apr 6, 2017

rhatdan commented Apr 10, 2017

rhatdan commented Apr 10, 2017

baude commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Apr 13, 2017

rhatdan commented Apr 13, 2017

baude commented Apr 13, 2017

rhatdan commented Apr 13, 2017

cgwalters commented Apr 13, 2017

cgwalters commented Apr 13, 2017

baude commented Apr 13, 2017

cgwalters commented Apr 13, 2017

rhatdan commented Apr 13, 2017

cgwalters commented Apr 13, 2017

cgwalters commented Apr 13, 2017

cgwalters commented Apr 13, 2017

baude commented Apr 13, 2017

rhatdan commented Apr 13, 2017

rh-atomic-bot commented Apr 13, 2017

rhatdan commented Apr 13, 2017

rh-atomic-bot commented Apr 13, 2017

cgwalters commented Apr 13, 2017

rh-atomic-bot commented Apr 13, 2017