app: add 'makecache' command #1035

jlebon · 2017-10-04T16:55:25Z

This is essentially the dnf/yum equivalent for rpm-ostree. To
complete the picture, we're missing the -C equivalent as well for all
the commands that make use of this metadata. Though makecache on its
own should be particularly helpful in test environment provisioning
where one may have cached metadata available to inject and just wants
rpm-ostree to sanity check it.

Marking as WIP for now. It works, but it does... unsavory things that I'd like to clean up first.

cgwalters · 2017-10-04T16:57:56Z

src/daemon/rpmostreed-daemon.c

@@ -451,7 +451,7 @@ update_status (RpmostreedDaemon *self)
      guint64 readytime = g_source_get_ready_time (self->idle_exit_source);
      guint64 curtime = g_source_get_time (self->idle_exit_source);
      guint64 timeout_micros = readytime - curtime;
-      if (timeout_micros < 0)
+      if (readytime < curtime)


I'd probably vote to split this out into a separate commit.

cgwalters · 2017-10-04T16:58:07Z

src/daemon/rpmostree-sysroot-upgrader.c

-  /* point the core to the passwd & group of the merge deployment */
-  rpmostree_context_set_passwd_dir (self->ctx, passwddir);
+  /* make sure yum repos and passwd used are from our cfg merge */
+  rpmostree_context_configure_from_deployment (self->ctx, self->sysroot,


This is a nice change!

cgwalters · 2017-10-04T17:08:03Z

src/daemon/rpmostreed-transaction-types.c

+  g_autofree char *origin_deployment_dirpath =
+    ostree_sysroot_get_deployment_dirpath (sysroot, origin_merge_deployment);
+  g_autofree char *origin_deployment_root =
+    g_build_filename (sysroot_path, origin_deployment_dirpath, NULL);


This is a bit ugly too, but we can fix up the core API later.

cgwalters · 2017-10-04T17:13:16Z

src/daemon/rpmostreed-transaction-types.c

+  MakeCacheTransaction *self = (MakeCacheTransaction *) transaction;
+  OstreeSysroot *sysroot = rpmostreed_transaction_get_sysroot (transaction);
+  g_autoptr(OstreeRepo) repo = NULL;
+  if (!ostree_sysroot_get_repo (sysroot, &repo, cancellable, error))


In new code this can just be:
OstreeRepo *repo = ostree_sysroot_repo (sysroot);.

jlebon · 2017-10-05T17:04:39Z

Now with support for --force to always expire repos (essentially saves us a cleanup -m call), and also the corresponding -C flag in the relevant commands to never expire cache. Split out minor prep in #1038. Just need to add some tests!

jlebon · 2017-10-05T19:35:39Z

Rebased and added tests! ⬆️ Lifting WIP.

It's no longer being built and is now older than the latest CentOS AH release. This should help us no longer see messages like: (rpm-ostree pkg-add:5662): GLib-CRITICAL **: g_variant_dict_lookup: assertion 'is_valid_dict (dict)' failed which happen because in coreos#1034, we started using `G_VARIANT_DICT_INIT`, whose special magic values only make sense in glib2 >= 2.50. (The alpha image stopped at 2.46). Saw this while debugging coreos#1035.

It's no longer being built and is now older than the latest CentOS AH release. This should help us no longer see messages like: (rpm-ostree pkg-add:5662): GLib-CRITICAL **: g_variant_dict_lookup: assertion 'is_valid_dict (dict)' failed which happen because in #1034, we started using `G_VARIANT_DICT_INIT`, whose special magic values only make sense in glib2 >= 2.50. (The alpha image stopped at 2.46). Saw this while debugging #1035. Closes: #1040 Approved by: cgwalters

cgwalters

High level comments:

I personally never liked the makecache name...there's too many things that can be a cache, and the make bit seems redundant.

refresh-mdcache
mdcache-refresh
md-refresh
rpmmd-refresh

Or something?

Anyways I'd also be OK if we just went with it.

The only other high level comment here is I think we should consider how this also affects the ostree side.

cgwalters · 2017-10-06T00:45:47Z

src/daemon/org.projectatomic.rpmostree1.policy

@@ -30,6 +30,17 @@
    </defaults>
  </action>

+  <action id="org.projectatomic.rpmostree1.makecache">


We already have org.projectatomic.rpmostree1.repo-refresh...one thing we need to keep in mind is that it's not necessary for polkit actions to be 1-1 with dbus calls. The more polkit actions we have, the more anyone locking down a system needs to think about them.

Good point. I'll merge it with repo-refresh.

cgwalters · 2017-10-06T00:48:00Z

src/app/rpmostree-builtin-upgrade.c

@@ -46,6 +47,7 @@ static GOptionEntry option_entries[] = {
  { "check-diff", 0, G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_NONE, &opt_preview, "Check for upgrades and print package diff only", NULL },
  { "preview", 0, 0, G_OPTION_ARG_NONE, &opt_preview, "Just preview package differences", NULL },
  { "check", 0, 0, G_OPTION_ARG_NONE, &opt_check, "Just check if an upgrade is available", NULL },
+  { "cache-only", 'C', 0, G_OPTION_ARG_NONE, &opt_cache_only, "Do not update repo metadata cache", NULL },


Yeah...but what about the ostree side? The option makes sense there too.

Sure. I was mostly going off of the --cache-only naming from the compose side. I suppose we'll have to have a new switch there too for unified core re. ostree caching? Would be nice to keep it consistent across those commands.

cgwalters · 2017-10-06T00:51:28Z

tests/vmcheck/test-basic.sh

+  cat > vmcheck-httpd.service << EOF
+[Service]
+# use py2 for CentOS
+ExecStart=/usr/bin/python -m SimpleHTTPServer 8888


There's also just systemd-run FWIW.

Ouhhh, that's much better!

Move the logging of yum repo information from `prepare` to `download_metadata`, since the latter could be called without necessarily calling the former, as is the case with `makecache`.

jlebon · 2017-10-06T13:29:04Z

Addressed the comments! I had to rebase to change the commit messages. Hmm, actually, I could have kept the fixups still. Sorry about that!

This is essentially the `dnf/yum makecache` equivalent for rpm-ostree. To complete the picture, this goes hand in hand with the `-C` equivalent, which is added in the next patch.

This is the equivalent version of `yum/dnf -C`. It goes together with the new `makecache` command to allow completely asynchronous cache update and usage.

I've been lazy about actually using using rsync instead of scp when copying new RPMs over to the VM. We do this here. Also make `vm_send_test_repo` take a mode argument that allows callers to completely skip the sending of the repo file itself. This will be needed for the `makecache` test, in which we *don't* want the repo to be local. It looks cleaner anyway for the gpgcheck use case as well.

jlebon · 2017-10-06T13:32:31Z

Fixups restored! ⬆️

cgwalters · 2017-10-06T14:25:00Z

I'm cool with refresh-md, though it feels a bit weird then to leave the DBus API etc. all named that way...I know renaming is annoying though.

So then the other issue is the --cache-only cmdline and whether that should do just rpmmd, or ostree as well? It's OK to me if we ship now with just rpmmd and then later teach it to do ostree too?

I can't think of a case offhand where I wouldn't want it to do both. The main wrinkle here is around things like COPR repos...but we already have the skip_if_unavailable=True flag.

jlebon · 2017-10-06T15:31:48Z

Alrighty, renamed everything to refresh-md. ⬆️
One tricky bit that took me some time was:

-  vm_raw_rsync --delete ${test_tmpdir}/yumrepo $VM:/tmp/vmcheck
+  # note we use -c here because we might be called twice within a second
+  vm_raw_rsync -c --delete ${test_tmpdir}/yumrepo $VM:/tmp/vmcheck

which was causing the sporadic errors seen in the testers. Basically, if we didn't pass another second within the next repo regen, to rsync the repomd.xml files just looked unchanged, so it didn't actually transfer it over. 👿

So then the other issue is the --cache-only cmdline and whether that should do just rpmmd, or ostree as well? It's OK to me if we ship now with just rpmmd and then later teach it to do ostree too?

That's fine with me as well. I'm not sure what it even means in the ostree context. I suppose you mean e.g. fail if we don't have the RPM already in our pkgcache?

cgwalters · 2017-10-06T16:06:34Z

For --cache-only I mean that e.g. rpm-ostree upgrade wouldn't try to fetch the latest ref. This does make a difference if one has a pending state. In the end of course right now we'd just redo the /etc merge...and down the line once we implement #40 then it'd be a noop again.

jlebon · 2017-10-06T16:15:02Z

Ahh OK, that makes more sense. Let's do that as a follow-up? With e.g. an associated rpm-ostree pull command that does just the pull?

cgwalters · 2017-10-06T16:20:34Z

@rh-atomic-bot r+ 4eec99e

rh-atomic-bot · 2017-10-06T16:20:41Z

⌛ Testing commit 4eec99e with merge f1cf1fe...

Move the logging of yum repo information from `prepare` to `download_metadata`, since the latter could be called without necessarily calling the former, as is the case with `makecache`. Closes: #1035 Approved by: cgwalters

This is essentially the `dnf/yum makecache` equivalent for rpm-ostree. To complete the picture, this goes hand in hand with the `-C` equivalent, which is added in the next patch. Closes: #1035 Approved by: cgwalters

This is the equivalent version of `yum/dnf -C`. It goes together with the new `makecache` command to allow completely asynchronous cache update and usage. Closes: #1035 Approved by: cgwalters

I've been lazy about actually using using rsync instead of scp when copying new RPMs over to the VM. We do this here. Also make `vm_send_test_repo` take a mode argument that allows callers to completely skip the sending of the repo file itself. This will be needed for the `makecache` test, in which we *don't* want the repo to be local. It looks cleaner anyway for the gpgcheck use case as well. Closes: #1035 Approved by: cgwalters

Closes: #1035 Approved by: cgwalters

rh-atomic-bot · 2017-10-06T17:23:16Z

💔 Test failed - status-atomicjenkins

cgwalters · 2017-10-06T18:20:55Z

@rh-atomic-bot r+ cb328b5

rh-atomic-bot · 2017-10-06T18:21:02Z

⚡ Test exempted: merge already tested.

This is essentially the `dnf/yum makecache` equivalent for rpm-ostree. To complete the picture, this goes hand in hand with the `-C` equivalent, which is added in the next patch. Closes: #1035 Approved by: cgwalters

This is the equivalent version of `yum/dnf -C`. It goes together with the new `makecache` command to allow completely asynchronous cache update and usage. Closes: #1035 Approved by: cgwalters

I've been lazy about actually using using rsync instead of scp when copying new RPMs over to the VM. We do this here. Also make `vm_send_test_repo` take a mode argument that allows callers to completely skip the sending of the repo file itself. This will be needed for the `makecache` test, in which we *don't* want the repo to be local. It looks cleaner anyway for the gpgcheck use case as well. Closes: #1035 Approved by: cgwalters

Closes: #1035 Approved by: cgwalters

Just taking what I learned from coreos#1035 and applying it here. What's nice about this is that there's no cleanup needed. Once the process is killed (or worst case, we reboot the VM), there's no traces left at all. Also added a few extra "ok" outputs.

Just taking what I learned from #1035 and applying it here. What's nice about this is that there's no cleanup needed. Once the process is killed (or worst case, we reboot the VM), there's no traces left at all. Also added a few extra "ok" outputs. Closes: #1043 Approved by: cgwalters

@cgwalters

As @cgwalters mentioned in coreos#1035, the new `--cache-only` implemented only the rpmmd half of the story. Here we complete that story by also ensuring that when in cache-only mode, we don't download new ostree data nor new packages. We try to complete the requested operation with what we have. To do this, we add support for the same `SYNTHETIC` pull that was added in ostree[1] so that we don't actually pull, but still perform timestamp checking. On the pkgcache side, we disable all remote repos and instead insert all our cached RPMs into the `DnfSack`. Care is taken to still perform SHA256 verification for local pkg installs/replacements. [1] ostreedev/ostree#642

As Colin mentioned in coreos#1035, the new `--cache-only` implemented only the rpmmd half of the story. Here we complete that story by also ensuring that when in cache-only mode, we don't download new ostree data nor new packages. We try to complete the requested operation with what we have. To do this, we add support for the same `SYNTHETIC` pull that was added in ostree[1] so that we don't actually pull, but still perform timestamp checking. On the pkgcache side, we disable all remote repos and instead insert all our cached RPMs into the `DnfSack`. Care is taken to still perform SHA256 verification for local pkg installs/replacements. [1] ostreedev/ostree#642

As Colin mentioned in coreos#1035, the new `--cache-only` implemented only the rpmmd half of the story. Here we complete that story by also ensuring that when in cache-only mode, we don't download new ostree data nor new packages. We try to complete the requested operation with what we have. To do this, we add support for the same `SYNTHETIC` pull that was added in ostree[1] so that we don't actually pull, but still perform timestamp checking. On the pkgcache side, we disable all remote repos and instead insert all our cached RPMs into the `DnfSack`. Care is taken to still perform SHA256 verification for local pkg installs/replacements. [1] ostreedev/ostree#642 Closes: coreos#687

As Colin mentioned in #1035, the new `--cache-only` implemented only the rpmmd half of the story. Here we complete that story by also ensuring that when in cache-only mode, we don't download new ostree data nor new packages. We try to complete the requested operation with what we have. To do this, we add support for the same `SYNTHETIC` pull that was added in ostree[1] so that we don't actually pull, but still perform timestamp checking. On the pkgcache side, we disable all remote repos and instead insert all our cached RPMs into the `DnfSack`. Care is taken to still perform SHA256 verification for local pkg installs/replacements. [1] ostreedev/ostree#642 Closes: #687 Closes: #1049 Approved by: cgwalters

jlebon added the WIP label Oct 4, 2017

cgwalters reviewed Oct 4, 2017

View reviewed changes

jlebon mentioned this pull request Oct 5, 2017

Pr/config ctx from depl #1038

Closed

jlebon force-pushed the pr/makecache branch from b1d4ec3 to 1e61778 Compare October 5, 2017 17:02

jlebon force-pushed the pr/makecache branch from 1e61778 to 87b3097 Compare October 5, 2017 19:35

jlebon removed the WIP label Oct 5, 2017

jlebon changed the title ~~WIP: app: add 'makecache' command~~ app: add 'makecache' command Oct 5, 2017

jlebon mentioned this pull request Oct 5, 2017

ci: don't use CentOS Alpha anymore #1040

Closed

cgwalters reviewed Oct 6, 2017

View reviewed changes

libpriv/core: log repo info when downloading rpmmd

d62abc4

Move the logging of yum repo information from `prepare` to `download_metadata`, since the latter could be called without necessarily calling the former, as is the case with `makecache`.

jlebon force-pushed the pr/makecache branch from 87b3097 to e9730d1 Compare October 6, 2017 13:25

jlebon added 6 commits October 6, 2017 13:30

app: add 'refresh-md' command

09949bd

This is essentially the `dnf/yum makecache` equivalent for rpm-ostree. To complete the picture, this goes hand in hand with the `-C` equivalent, which is added in the next patch.

app: add -C flag to always use cached metadata

1f0b9f2

This is the equivalent version of `yum/dnf -C`. It goes together with the new `makecache` command to allow completely asynchronous cache update and usage.

tests/vmcheck: add test for refresh-md and --cache-only

14dd63a

fixup! app: add 'refresh-md' command

76d0481

fixup! tests/vmcheck: add test for refresh-md and --cache-only

fff9a14

jlebon force-pushed the pr/makecache branch from e9730d1 to fff9a14 Compare October 6, 2017 13:31

jlebon added 3 commits October 6, 2017 14:36

fixup! tests/vmcheck: add test for refresh-md and --cache-only

1302079

fixup! app: add 'refresh-md' command

9ef939f

fixup! tests/vmcheck: add test for refresh-md and --cache-only

3f85d1a

fixup! tests/vmcheck: add test for refresh-md and --cache-only

4eec99e

jlebon mentioned this pull request Oct 6, 2017

rpm-ostree pull 🖧 #713

Closed

cgwalters added the reviewed/approved label Oct 6, 2017

rh-atomic-bot pushed a commit that referenced this pull request Oct 6, 2017

tests/vmcheck: add test for refresh-md and --cache-only

f1cf1fe

Closes: #1035 Approved by: cgwalters

fixup! tests/vmcheck: add test for refresh-md and --cache-only

cb328b5

rh-atomic-bot closed this in 5601a60 Oct 6, 2017

rh-atomic-bot pushed a commit that referenced this pull request Oct 6, 2017

tests/vmcheck: add test for refresh-md and --cache-only

dd7c748

Closes: #1035 Approved by: cgwalters

jlebon deleted the pr/makecache branch October 6, 2017 18:34

jlebon mentioned this pull request Oct 6, 2017

vmcheck: use systemd-run for running post-that-hangs #1043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

app: add 'makecache' command #1035

app: add 'makecache' command #1035

jlebon commented Oct 4, 2017

cgwalters Oct 4, 2017

cgwalters Oct 4, 2017

cgwalters Oct 4, 2017

cgwalters Oct 4, 2017

jlebon commented Oct 5, 2017

jlebon commented Oct 5, 2017

cgwalters left a comment

cgwalters Oct 6, 2017

jlebon Oct 6, 2017

cgwalters Oct 6, 2017

jlebon Oct 6, 2017

cgwalters Oct 6, 2017

jlebon Oct 6, 2017

jlebon commented Oct 6, 2017

jlebon commented Oct 6, 2017

cgwalters commented Oct 6, 2017

jlebon commented Oct 6, 2017

cgwalters commented Oct 6, 2017

jlebon commented Oct 6, 2017

cgwalters commented Oct 6, 2017

rh-atomic-bot commented Oct 6, 2017

rh-atomic-bot commented Oct 6, 2017

cgwalters commented Oct 6, 2017

rh-atomic-bot commented Oct 6, 2017

app: add 'makecache' command #1035

app: add 'makecache' command #1035

Conversation

jlebon commented Oct 4, 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

jlebon commented Oct 5, 2017

jlebon commented Oct 5, 2017

cgwalters left a 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 Oct 6, 2017

jlebon commented Oct 6, 2017

cgwalters commented Oct 6, 2017

jlebon commented Oct 6, 2017

cgwalters commented Oct 6, 2017

jlebon commented Oct 6, 2017

cgwalters commented Oct 6, 2017

rh-atomic-bot commented Oct 6, 2017

rh-atomic-bot commented Oct 6, 2017

cgwalters commented Oct 6, 2017

rh-atomic-bot commented Oct 6, 2017