Copy in additional files before postprocessing #997

jdieter · 2017-09-18T15:13:59Z

Sometimes it's useful to have access to the additional files when running the post script, so this re-orders the compose process to copy the additional files in before the post script runs.

I'm not sure how to create a test for this patch, but if you give me some pointers, I'll put something together.

rh-atomic-bot · 2017-09-18T15:14:05Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

jdieter · 2017-09-18T15:16:56Z

While I'm thinking about this, I did backport this patch to the 2017.8 release and tested locally, and it works as advertised.

cgwalters · 2017-09-19T13:54:57Z

Hi, thanks for the patch! There's a bigger picture issue here; see
#471

I swear there was another issue filed about this; I found #779 but that's not quite the same.

Anyways I have a vague concern about compatibility here - could someone have been using add-files in such a way that they expected it to happen before postprocessing? I can't think offhand of how this would break something though.

cgwalters · 2017-09-19T13:56:49Z

There are tests in tests/compose - I can take care of updating them for this though.

jdieter · 2017-09-19T14:02:04Z

Well, you've read my mind. As I think I mentioned to you at Flock, I'm really interested in using this to run ansible locally. In my testing, I've managed to tar up my ansible playbooks, copy them across using add-files, and run them. I'm hitting some roadblocks in ansible (apparently the service and systemd modules don't like being unable to connect to systemd when enabling and disabling services), so I haven't actually managed to complete a compose yet, but, assuming rpm-ostree will commit after running the post script, it's working perfectly.

The only situation that I thought of where add-files might need to happen after post was if it doesn't create any intermediate directories and the user needs to be manually do that in post.

jdieter · 2017-09-19T14:02:44Z

Just to clarify, for what I'm doing, I don't actually need network access when running the ansible scripts.

cgwalters · 2017-09-25T15:37:31Z

Will look at this and tests for it after the next release.

cgwalters · 2017-09-25T21:28:18Z

bot, add author to whitelist

cgwalters · 2017-09-25T21:30:19Z

This breaks at least the current test-misc-tweaks.sh in the compose tests - the issue seems to be we're basically creating /usr/etc and /etc in the tree. Fixing this seems like it's going to require some plumbing to ensure that we do the /etc → /usr/etc rename before we do add-files. This actually overlaps a bit with work in #940 - I'm going to look at extracting that as a separate PR, then will rework this on top.

Hooray, it's dead (here, but not in ostree). Noticed this while working on <coreos#997>

Lots of confusion in the codebase about this. The basic problem is that in *most* cases, our code doesn't care; it's conceptually operating on `/usr/etc`, which we could maintain as `/etc` and just rename it back at the very end. The exceptions though are the `/etc/passwd` handling and livefs. And of course libostree needs to handle `/usr/etc` vs `/etc` for config merging. I considered trying to keep things the other way, but while I think we have some ugly added here in this patch for things where we need to maintain an external view (`remove-files` and `remove-from-packages`, and boy am I glad we had tests for those), this ends up being mostly more consistent elsewhere. One thing that might help is to maintain a fd for it; but that'd be an even more invasive change. This also ends up rolling in some unified core prep from coreos#940 in the form of `rename_if_exists()` - basically for some minimal rootfs we may not have `/boot`, or for that matter potentially even `etc`. Prep for coreos#997

cgwalters · 2017-09-26T01:17:31Z

OK, reworked this with a prep patch. Lifting WIP.

Hooray, it's dead (here, but not in ostree). Noticed this while working on <#997> Closes: #1012 Approved by: jlebon

jlebon · 2017-09-26T19:45:54Z

src/libpriv/rpmostree-postprocess.c

@@ -1442,6 +1450,12 @@ rpmostree_treefile_postprocessing (int            rootfs_fd,
      if (!glnx_shutil_rm_rf_at (rootfs_fd, val, cancellable, error))
        return FALSE;
    }
+  if (len > 0)


But the array could be specified but empty, right? I'd just shove this whole loop with the pre and post /etc moves in the if statement itself.

jlebon · 2017-09-26T19:52:37Z

src/libpriv/rpmostree-passwd-util.c

+  if (!glnx_fstatat_allow_noent (rootfs_dfd, "usr/etc", &stbuf, 0, error))
+    return FALSE;
+  if (errno == ENOENT)
+    etc_path = "etc";


But this should never happen, right? We've already moved /etc to /usr/etc, so it'd be an error to not find it there, no?

Updated for everything except this...I still need to investigate this bit more.

OK, upon further investigation I think you're right. I haven't yet rebased unified core on top of this to verify but if there's an issue with that we can fix it later.

jlebon · 2017-09-26T20:03:52Z

src/libpriv/rpmostree-postprocess.c

@@ -1559,6 +1556,10 @@ rpmostree_treefile_postprocessing (int            rootfs_fd,
      }
  }

+  /* Copy in additional files before postprocessing */
+  if (!copy_additional_files (rootfs_fd, context_directory, treefile, cancellable, error))


I just realized now that we handle add-files after the /etc to /usr/etc move. What this patch does is correct since it maintains that, though are we sure that's what we want to begin with? Seems like users would expect to dump things in e.g. /etc, not /usr/etc, right?

FWIW, I'd prefer the /etc to /usr/etc move happen after both add-files and the post script, but it's easy enough to work around if that's not possible.

I'm not totally opposed to changing it but on the other hand we had a test case that did it, and I've definitely seen people talk about using add-files here and I'd like not to break them.

Perhaps the simplest thing is to support both; we could do that by dropping a temporary symlink, or translating paths.

Translating paths seems reasonable, yeah.

Lots of confusion in the codebase about this. The basic problem is that in *most* cases, our code doesn't care; it's conceptually operating on `/usr/etc`, which we could maintain as `/etc` and just rename it back at the very end. The exceptions though are the `/etc/passwd` handling and livefs. And of course libostree needs to handle `/usr/etc` vs `/etc` for config merging. I considered trying to keep things the other way, but while I think we have some ugly added here in this patch for things where we need to maintain an external view (`remove-files` and `remove-from-packages`, and boy am I glad we had tests for those), this ends up being mostly more consistent elsewhere. One thing that might help is to maintain a fd for it; but that'd be an even more invasive change. This also ends up rolling in some unified core prep from coreos#940 in the form of `rename_if_exists()` - basically for some minimal rootfs we may not have `/boot`, or for that matter potentially even `etc`. Prep for coreos#997

Even though it's really `/usr/etc`. This is for greater consistency with `postprocess-script` where it appears as `/etc`.

Sometimes it's useful to have access to the additional files when running the post script, so this re-orders the compose process to copy the additional files in before the post script runs Signed-off-by: Jonathan Dieter <[email protected]>

jlebon · 2017-09-28T15:38:53Z

Looks good! @rh-atomic-bot r+ 3596d11

rh-atomic-bot · 2017-09-28T15:38:57Z

⌛ Testing commit 3596d11 with merge fd6109a...

Even though it's really `/usr/etc`. This is for greater consistency with `postprocess-script` where it appears as `/etc`. Closes: #997 Approved by: jlebon

Sometimes it's useful to have access to the additional files when running the post script, so this re-orders the compose process to copy the additional files in before the post script runs Signed-off-by: Jonathan Dieter <[email protected]> Closes: #997 Approved by: jlebon

rh-atomic-bot · 2017-09-28T15:56:48Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing fd6109a to master...

tjim · 2017-10-01T16:37:18Z

Just got burned by this (add-files to /etc, ostree fails due to existence of both /etc and /usr/etc). I realize this pull will address this but what happens now if I add-files to /usr/etc (my workaround for the current release)? Will there be a similar error?

Consider this a request for documentation and a check/error message at rpm-ostree compose time.

cgwalters · 2017-10-02T13:36:32Z

Hi @tjim - it should work in both cases now in git master.

cgwalters self-assigned this Sep 25, 2017

cgwalters added the for-next-release label Sep 25, 2017

cgwalters added WIP and removed for-next-release labels Sep 25, 2017

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Sep 25, 2017

libpriv: Remove unused OSTREE_GIO_FAST_QUERYINFO

d204322

Hooray, it's dead (here, but not in ostree). Noticed this while working on <coreos#997>

cgwalters mentioned this pull request Sep 25, 2017

libpriv: Remove unused OSTREE_GIO_FAST_QUERYINFO #1012

Closed

cgwalters force-pushed the master branch from 7097880 to b73f308 Compare September 26, 2017 01:17

cgwalters removed the WIP label Sep 26, 2017

cgwalters force-pushed the master branch from b73f308 to 73f46bf Compare September 26, 2017 01:17

rh-atomic-bot pushed a commit that referenced this pull request Sep 26, 2017

libpriv: Remove unused OSTREE_GIO_FAST_QUERYINFO

f5e158b

Hooray, it's dead (here, but not in ostree). Noticed this while working on <#997> Closes: #1012 Approved by: jlebon

rh-atomic-bot pushed a commit that referenced this pull request Sep 26, 2017

libpriv: Remove unused OSTREE_GIO_FAST_QUERYINFO

57fcd9e

Hooray, it's dead (here, but not in ostree). Noticed this while working on <#997> Closes: #1012 Approved by: jlebon

cgwalters requested a review from jlebon September 26, 2017 19:17

jlebon reviewed Sep 26, 2017

View reviewed changes

cgwalters and others added 3 commits September 27, 2017 21:56

postprocess: Allow add-files into /etc

15b9b2b

Even though it's really `/usr/etc`. This is for greater consistency with `postprocess-script` where it appears as `/etc`.

cgwalters force-pushed the master branch from 73f46bf to 71ec612 Compare September 28, 2017 02:07

fixup! lib/compose: Maintain /etc as /usr/etc more consistently

3596d11

rh-atomic-bot pushed a commit that referenced this pull request Sep 28, 2017

postprocess: Allow add-files into /etc

2e59a27

Even though it's really `/usr/etc`. This is for greater consistency with `postprocess-script` where it appears as `/etc`. Closes: #997 Approved by: jlebon

jlebon added the reviewed/approved label Sep 28, 2017

rh-atomic-bot closed this in 4528782 Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy in additional files before postprocessing #997

Copy in additional files before postprocessing #997

jdieter commented Sep 18, 2017

rh-atomic-bot commented Sep 18, 2017

jdieter commented Sep 18, 2017

cgwalters commented Sep 19, 2017

cgwalters commented Sep 19, 2017

jdieter commented Sep 19, 2017

jdieter commented Sep 19, 2017

cgwalters commented Sep 25, 2017

cgwalters commented Sep 25, 2017

cgwalters commented Sep 25, 2017

cgwalters commented Sep 26, 2017

jlebon Sep 26, 2017

jlebon Sep 26, 2017

cgwalters Sep 28, 2017

cgwalters Sep 28, 2017

jlebon Sep 26, 2017

jdieter Sep 27, 2017

cgwalters Sep 27, 2017

jlebon Sep 27, 2017

jlebon commented Sep 28, 2017

rh-atomic-bot commented Sep 28, 2017

rh-atomic-bot commented Sep 28, 2017

tjim commented Oct 1, 2017

cgwalters commented Oct 2, 2017

Copy in additional files before postprocessing #997

Copy in additional files before postprocessing #997

Conversation

jdieter commented Sep 18, 2017

rh-atomic-bot commented Sep 18, 2017

jdieter commented Sep 18, 2017

cgwalters commented Sep 19, 2017

cgwalters commented Sep 19, 2017

jdieter commented Sep 19, 2017

jdieter commented Sep 19, 2017

cgwalters commented Sep 25, 2017

cgwalters commented Sep 25, 2017

cgwalters commented Sep 25, 2017

cgwalters commented Sep 26, 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

Choose a reason for hiding this comment

jlebon commented Sep 28, 2017

rh-atomic-bot commented Sep 28, 2017

rh-atomic-bot commented Sep 28, 2017

tjim commented Oct 1, 2017

cgwalters commented Oct 2, 2017