Parallel relabel #1137

cgwalters · 2017-12-07T19:05:18Z

On top of #1124

jlebon · 2017-12-07T19:30:53Z

src/libpriv/rpmostree-core.c

@@ -2586,6 +2447,7 @@ relabel_in_thread_impl (RpmOstreeContext *self,
                                       NULL, NULL, NULL);

  ostree_repo_commit_modifier_set_devino_cache (modifier, cache);
+  ostree_repo_commit_modifier_set_sepolicy (modifier, self->sepolicy);


To answer your question, I can't think of a good reason right now why it wasn't done this way. Definitely ostreedev/ostree#1165 would've been a blocker, though clearly it could've been fixed back then too. Anyway, this is definitely better!

jlebon · 2017-12-07T19:46:34Z

This was almost straightforward except I spent a lot of time
debugging what turned out to be calling dnf_package_get_nevra()
in the worker threads 😢.

Ouch, I know that pain! :(

[1] ... Write it in Python, where MT isn't useful at all?

For Python at least, it's really easy to do multiprocessing using the module. I make use of it in the PAPR Python rewrite (which I really should pick up again). But yeah, clearly it sucks if you need a lot of coordination.

rh-atomic-bot · 2017-12-07T20:13:40Z

☔ The latest upstream changes (presumably 51c5591) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters · 2017-12-08T20:53:07Z

Rebased 🏄‍♂️ and lifting WIP.

jlebon · 2017-12-11T15:31:11Z

src/libpriv/rpmostree-core.c

+                        GCancellable     *cancellable,
+                        GError          **error)
+{
+  if (g_cancellable_set_error_if_cancelled (cancellable, error))


Don't we also need this in import_in_thread ? Otherwise, we won't even stop new imports, no?

We check the cancellable each iteration of reading the archive, and GTask also checks the cancellable before actually running the thread; see g_task_start_task_thread(), so it's probably simplest to just drop the one in importing.

Hmm no the GTask one is just if one uses g_task_set_return_on_cancel(), let's be conservative.

jlebon · 2017-12-11T20:24:07Z

src/libpriv/rpmostree-core.c

@@ -2093,6 +2095,7 @@ rpmostree_context_import_jigdo (RpmOstreeContext *self,

    self->async_dnfstate = hifstate;
    self->async_running = TRUE;
+    self->async_cancellable = g_cancellable_new ();


Sorry, I'm still not clear on how this works exactly. We create a new GCancellable here, but when do we actually consult it? We're still passing our own cancellable to rpmostree_importer_run_async so the g_cancellable_set_error_if_cancelled importer fixup you added is not actually using this, right? So, aren't we still going to end up trying to import all the pkgs even if the very first one failed? Or should we be passing self->async_cancellable to rpmostree_importer_run_async down below?

No you're totally right, it doesn't work 😉. I was throwing code at the wall here a bit. Let's do the obvious thing here and reuse the source cancellable, which is what ostree-pull does too.

Pushed a fixup ⬇️ which I tested by adding:

if (g_random_boolean ()) return glnx_throw (...)

in the relabel_in_thread() code.

jlebon

Hmm, it looks like we're not preserving the original error correctly:

# + ssh -o User=root -o ControlMaster=auto -o ControlPath=/var/tmp/ssh-vmcheck2-1513030065068962438.sock -o ControlPersist=yes vmcheck2 env ASAN_OPTIONS=detect_leaks=false rpm-ostree install test-opt-1.0
# error: Operation was cancelled
+ fatal 'File '\''err.txt'\'' doesn'\''t match regexp '\''See https://github.com/projectatomic/rpm-ostree/issues/233'\'''
+ echo File ''\''err.txt'\''' 'doesn'\''t' match regexp ''\''See' 'https://github.com/projectatomic/rpm-ostree/issues/233'\'''
File 'err.txt' doesn't match regexp 'See https://github.com/projectatomic/rpm-ostree/issues/233'

jlebon · 2017-12-11T23:34:49Z

src/libpriv/rpmostree-core.c

@@ -2095,7 +2096,7 @@ rpmostree_context_import_jigdo (RpmOstreeContext *self,

    self->async_dnfstate = hifstate;
    self->async_running = TRUE;
-    self->async_cancellable = g_cancellable_new ();
+    self->async_cancellable = cancellable;


Ahh OK, that makes more sense now! :)

To exit earlier if we've been cancelled. Came up in review for parallel relabeling.

I believe this is a leftover vestige, and it was adding confusion when I was debugging `rpmostree-core.c` async ops and cancellation. Now the only cancellables in the daemon are created by transaction ops.

Basically since we're doing internal async ops which set the cancellable on failure, we still want the first error to win since it'll be more useful. See the docs for `g_task_set_check_cancellable()` for more.

This is another big task just like importing that greatly benefits from being parallel. While here I hit the issue that on error we didn't wait for pending async tasks to complete; I changed things for importing so that we do that, and used it here too. This was almost straightforward except I spent a *lot* of time debugging what turned out to be calling `dnf_package_get_nevra()` in the worker threads 😢. I'm mostly writing this to speed up unified core/jigdo, but it's also obviously relevant on the client side.

cgwalters · 2017-12-12T18:03:14Z

Hmm, it looks like we're not preserving the original error correctly:

Yep, added a prep commit to fix that ⬆️

jlebon · 2017-12-12T19:21:00Z

@rh-atomic-bot r+ f7e9992

rh-atomic-bot · 2017-12-12T19:21:09Z

⚡ Test exempted: merge already tested.

I believe this is a leftover vestige, and it was adding confusion when I was debugging `rpmostree-core.c` async ops and cancellation. Now the only cancellables in the daemon are created by transaction ops. Closes: #1137 Approved by: jlebon

Basically since we're doing internal async ops which set the cancellable on failure, we still want the first error to win since it'll be more useful. See the docs for `g_task_set_check_cancellable()` for more. Closes: #1137 Approved by: jlebon

This is another big task just like importing that greatly benefits from being parallel. While here I hit the issue that on error we didn't wait for pending async tasks to complete; I changed things for importing so that we do that, and used it here too. This was almost straightforward except I spent a *lot* of time debugging what turned out to be calling `dnf_package_get_nevra()` in the worker threads 😢. I'm mostly writing this to speed up unified core/jigdo, but it's also obviously relevant on the client side. Closes: #1137 Approved by: jlebon

Basically the `rpmostree_context_relabel()` call we had in the treecompose path for unified core didn't actually have any effect as the core code did a relabel and unset the array. I think this may actually be a regression from: coreos#1137 though I didn't verify. Anyways looking at this, the code is a lot simpler if we change the API so that the "normal" relabeling is folded into `rpmostree_context_assemble()`. Then we change the public relabel API to be "force relabel" which we use in the unified core 🌐 treecompose path. This shrinks the jigdoRPM for FAH from 90MB to 68MB. Closes: coreos#1172

Basically the `rpmostree_context_relabel()` call we had in the treecompose path for unified core didn't actually have any effect as the core code did a relabel and unset the array. I think this may actually be a regression from: #1137 though I didn't verify. Anyways looking at this, the code is a lot simpler if we change the API so that the "normal" relabeling is folded into `rpmostree_context_assemble()`. Then we change the public relabel API to be "force relabel" which we use in the unified core 🌐 treecompose path. This shrinks the jigdoRPM for FAH from 90MB to 68MB. Closes: #1172 Closes: #1173 Approved by: jlebon

jlebon reviewed Dec 7, 2017

View reviewed changes

cgwalters mentioned this pull request Dec 8, 2017

Parallel relabel prep #1138

Closed

cgwalters force-pushed the parallel-relabel branch from 7c95875 to 2586e39 Compare December 8, 2017 20:51

cgwalters changed the title ~~WIP: Parallel relabel~~ Parallel relabel Dec 8, 2017

jlebon reviewed Dec 11, 2017

View reviewed changes

cgwalters force-pushed the parallel-relabel branch from 2586e39 to a060f4e Compare December 11, 2017 19:38

jlebon reviewed Dec 11, 2017

View reviewed changes

jlebon reviewed Dec 12, 2017

View reviewed changes

cgwalters added 4 commits December 12, 2017 13:02

importer: Check cancellable early on

4ff273f

To exit earlier if we've been cancelled. Came up in review for parallel relabeling.

daemon/sysroot: Drop internal cancellable

c1c5a51

I believe this is a leftover vestige, and it was adding confusion when I was debugging `rpmostree-core.c` async ops and cancellation. Now the only cancellables in the daemon are created by transaction ops.

txn: Let first error win vs cancellation

02972b2

Basically since we're doing internal async ops which set the cancellable on failure, we still want the first error to win since it'll be more useful. See the docs for `g_task_set_check_cancellable()` for more.

cgwalters force-pushed the parallel-relabel branch from 3f9595a to f7e9992 Compare December 12, 2017 18:02

jlebon added the reviewed/approved label Dec 12, 2017

rh-atomic-bot closed this in 52ce16d Dec 12, 2017

cgwalters mentioned this pull request Jan 2, 2018

core,compose: Fix unified core pkgcache labeling #1173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parallel relabel #1137

Parallel relabel #1137

cgwalters commented Dec 7, 2017

jlebon Dec 7, 2017

jlebon commented Dec 7, 2017

rh-atomic-bot commented Dec 7, 2017

cgwalters commented Dec 8, 2017

jlebon Dec 11, 2017

cgwalters Dec 11, 2017

cgwalters Dec 11, 2017 •

edited

Loading

jlebon Dec 11, 2017

cgwalters Dec 11, 2017

cgwalters Dec 11, 2017

jlebon left a comment

jlebon Dec 11, 2017

cgwalters commented Dec 12, 2017

jlebon commented Dec 12, 2017

rh-atomic-bot commented Dec 12, 2017

Parallel relabel #1137

Parallel relabel #1137

Conversation

cgwalters commented Dec 7, 2017

jlebon Dec 7, 2017

Choose a reason for hiding this comment

jlebon commented Dec 7, 2017

rh-atomic-bot commented Dec 7, 2017

cgwalters commented Dec 8, 2017

jlebon Dec 11, 2017

Choose a reason for hiding this comment

cgwalters Dec 11, 2017

Choose a reason for hiding this comment

cgwalters Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

jlebon Dec 11, 2017

Choose a reason for hiding this comment

cgwalters Dec 11, 2017

Choose a reason for hiding this comment

cgwalters Dec 11, 2017

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

jlebon Dec 11, 2017

Choose a reason for hiding this comment

cgwalters commented Dec 12, 2017

jlebon commented Dec 12, 2017

rh-atomic-bot commented Dec 12, 2017

cgwalters Dec 11, 2017 •

edited

Loading