base override pkg removals #797

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
@jlebon
Member

jlebon commented May 26, 2017

Add basic support for base layer pkg removals. This is still in somewhat rough shape; some shortcuts were taken esp. in the core that I'd like to revisit. There's also a lot of fixes & refactors, with varying degrees of relatedness to base overrides. And of course, tests.

Will split this up into multiple PRs.

Removing base package:

[root@f25-ros-dev ~]# rpm-ostree status
State: idle
Deployments:
* vmcheck
              Timestamp: 2017-05-26 17:28:42
                 Commit: fe6ddc529c161964f5801f4879f8ad5c98a197e24cfd86714874ba1dd02df762
[root@f25-ros-dev ~]# rpm -q strace
strace-4.16-1.fc25.x86_64
[root@f25-ros-dev ~]# strace
strace: must have PROG [ARGS] or -p PID
Try 'strace -h' for more information.
[root@f25-ros-dev ~]# rpm-ostree ex override remove strace
Checking out tree fe6ddc5... done

Downloading metadata: [==========================================================================================] 100%
Resolving dependencies... done
Applying overlays and overrides... done
Writing rpmdb... done
Writing OSTree commit... done
Copying /etc changes: 26 modified, 0 removed, 102 added
Transaction complete; bootconfig swap: yes deployment count change: 1
Removed:
  strace-4.16-1.fc25.x86_64
Run "systemctl reboot" to start a reboot
[root@f25-ros-dev ~]# reboot
...
[root@f25-ros-dev ~]# rpm-ostree status
State: idle
Deployments:
* vmcheck
              Timestamp: 2017-05-26 17:28:42
             BaseCommit: fe6ddc529c161964f5801f4879f8ad5c98a197e24cfd86714874ba1dd02df762
    RemovedBasePackages: strace

  vmcheck
              Timestamp: 2017-05-26 17:28:42
                 Commit: fe6ddc529c161964f5801f4879f8ad5c98a197e24cfd86714874ba1dd02df762
[root@f25-ros-dev ~]# rpm -q strace
package strace is not installed
[root@f25-ros-dev ~]# strace
bash: strace: command not found

And of course, resetting the override takes us back to the same BaseCommit:

[root@f25-ros-dev ~]# rpm-ostree ex override reset strace
Copying /etc changes: 26 modified, 0 removed, 102 added
Transaction complete; bootconfig swap: no deployment count change: 0
Added:
  strace-4.16-1.fc25.x86_64
Run "systemctl reboot" to start a reboot
[root@f25-ros-dev ~]# rpm-ostree status -v
State: idle
Deployments:
  vmcheck
              Timestamp: 2017-05-26 17:28:42
                 Commit: fe6ddc529c161964f5801f4879f8ad5c98a197e24cfd86714874ba1dd02df762
              StateRoot: fedora-atomic

* vmcheck
              Timestamp: 2017-05-26 17:28:42
             BaseCommit: fe6ddc529c161964f5801f4879f8ad5c98a197e24cfd86714874ba1dd02df762
                 Commit: 5fb1929c7136b03e049557d3c84578517618a38ae2baef42a0435fcc5dad9ed5
              StateRoot: fedora-atomic
    RemovedBasePackages: strace

@jlebon jlebon added the WIP label May 26, 2017

This was referenced May 29, 2017

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot May 30, 2017

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

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

@jlebon

This comment has been minimized.

Show comment
Hide comment
@jlebon

jlebon May 31, 2017

Member

OK, rebased with quite a few fixes and corner case handling. Though I still need to write proper tests for this.

Member

jlebon commented May 31, 2017

OK, rebased with quite a few fixes and corner case handling. Though I still need to write proper tests for this.

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Jun 2, 2017

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

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

@jlebon jlebon removed the WIP label Jun 5, 2017

@jlebon

This comment has been minimized.

Show comment
Hide comment
@jlebon

jlebon Jun 5, 2017

Member

OK, rebased and more minor things fixed + tests added!
I think this is ready for review now.

Member

jlebon commented Jun 5, 2017

OK, rebased and more minor things fixed + tests added!
I think this is ready for review now.

@cgwalters

Overall looks excellent, just minor bits.

+static RpmOstreeCommand override_subcommands[] = {
+ { "replace", RPM_OSTREE_BUILTIN_FLAG_REQUIRES_ROOT |
+ RPM_OSTREE_BUILTIN_FLAG_SUPPORTS_PKG_INSTALLS |
+ RPM_OSTREE_BUILTIN_FLAG_HIDDEN, /* XXX UNDER CONSTRUCTION XXX */

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

It's an experimental experimental command! rpm-ostree ex ex replace? 😄

@cgwalters

cgwalters Jun 5, 2017

Member

It's an experimental experimental command! rpm-ostree ex ex replace? 😄

@@ -125,7 +125,8 @@ rpmostree_builtin_upgrade (int argc,
opt_allow_downgrade,
FALSE, /* skip-purge */
FALSE, /* no-pull-base */
- FALSE); /* dry-run */
+ FALSE, /* dry-run */
+ FALSE); /* no-overrides */

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

Not necessary to redo in this patch, but at this point I'm wondering if it'd be saner to have the command lines construct GVariants directly or so.

@cgwalters

cgwalters Jun 5, 2017

Member

Not necessary to redo in this patch, but at this point I'm wondering if it'd be saner to have the command lines construct GVariants directly or so.

src/app/rpmostree-dbus-helpers.c
+ * split out local pkgs */
+ vardict_insert_strv (&dict, "override-replace-packages", override_replace_pkgs);
+
+ vardict_insert_strv (&dict, "uninstall-packages", uninstall_pkgs);

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

Out of curiosity, why is uninstall here in the middle of the override- section?

@cgwalters

cgwalters Jun 5, 2017

Member

Out of curiosity, why is uninstall here in the middle of the override- section?

+
+ if (g_hash_table_contains (removals, name))
+ {
+ glnx_throw (error, "Cannot request '%s' provided by removed package '%s'",

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

return

@cgwalters

cgwalters Jun 5, 2017

Member

return

src/daemon/rpmostree-sysroot-upgrader.c
- if (!rpmostree_sack_has_subject (rsack->sack, itkey))
- g_ptr_array_add (ret_missing_pkgs, g_strdup (itkey));
+ const char *pattern = itkey;
+ GPtrArray *matches = rpmostree_get_matching_packages (self->rsack->sack, pattern);

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

g_autoptr()

@cgwalters

cgwalters Jun 5, 2017

Member

g_autoptr()

src/libpriv/rpmostree-core.c
+ return glnx_null_throw (error, "Failed to find package '%s' in rpmdb",
+ dnf_package_get_nevra (pkg));
+
+ return headerLink (g_steal_pointer (&hdr));

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

Do we need headerLink? Aren't we returning our ref to it? Either that or the g_auto() on the Header is wrong, correct?

@cgwalters

cgwalters Jun 5, 2017

Member

Do we need headerLink? Aren't we returning our ref to it? Either that or the g_auto() on the Header is wrong, correct?

This comment has been minimized.

@jlebon

jlebon Jun 5, 2017

Member

Good catch!

@jlebon

jlebon Jun 5, 2017

Member

Good catch!

+ continue;
+
+ /* avoiding the stat syscall is worth a bit of userspace computation */
+ if (rpmostree_str_has_prefix_in_ptrarray (fn, deleted_dirs))

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

Yeah...though at some point we should look for a radix tree implementation for this.

@cgwalters

cgwalters Jun 5, 2017

Member

Yeah...though at some point we should look for a radix tree implementation for this.

src/libpriv/rpmostree-core.c
{ DECLARE_RPMSIGHANDLER_RESET;
rpmtsOrder (ordering_ts);
}
- rpmostree_output_task_begin ("Overlaying");
+ rpmostree_output_task_begin ("Applying overrides and overlays");

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

Can we tweak this to only show overrides if there are any? Maybe something like:

if (n_overrides > 0 && n_overlays > 0)
  task_begin ("Applying %u overrides, %u overlays", n_overrides, n_overlays)
else if (n_overlays > 0)
  task_begin ("Applying %u overlays", n_overlays);
else
  task_begin ("Applying %u overrides", n_overrides")

?

@cgwalters

cgwalters Jun 5, 2017

Member

Can we tweak this to only show overrides if there are any? Maybe something like:

if (n_overrides > 0 && n_overlays > 0)
  task_begin ("Applying %u overrides, %u overlays", n_overrides, n_overlays)
else if (n_overlays > 0)
  task_begin ("Applying %u overlays", n_overlays);
else
  task_begin ("Applying %u overrides", n_overrides")

?

}
rpmostree_output_task_end ("done");
if (!rpmostree_rootfs_prepare_links (tmprootfs_dfd, cancellable, error))
return FALSE;
- if (!noscripts)
+ /* NB: we're not running scripts right now for removals, so this is only for
+ * overlays */

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

It's probably worth an analysis at some point of what packages do in %postun; my offhand guess is that the vast majority are systemctl stop foo and/or some-cache-update (like ldconfig) - only the latter of which applies to us.

(Random question - you seem to use NB a lot - it feels to me like comments in code are implicitly NB? I don't really mind though)

@cgwalters

cgwalters Jun 5, 2017

Member

It's probably worth an analysis at some point of what packages do in %postun; my offhand guess is that the vast majority are systemctl stop foo and/or some-cache-update (like ldconfig) - only the latter of which applies to us.

(Random question - you seem to use NB a lot - it feels to me like comments in code are implicitly NB? I don't really mind though)

This comment has been minimized.

@jlebon

jlebon Jun 5, 2017

Member

That is correct! I did have a look when writing this code and indeed the great majority was systemd, ldconfig, and icon cache.

Hehe, good point about NB. I'm actually not sure myself how I decide when to preface by NB and when not to. I think maybe when something is especially tricky?

@jlebon

jlebon Jun 5, 2017

Member

That is correct! I did have a look when writing this code and indeed the great majority was systemd, ldconfig, and icon cache.

Hehe, good point about NB. I'm actually not sure myself how I decide when to preface by NB and when not to. I think maybe when something is especially tricky?

src/libpriv/rpmostree-origin.c
+ if (!g_hash_table_contains (origin->cached_overrides_remove, pkg))
+ return glnx_throw (error, "No overrides for package '%s'", pkg);
+
+ g_hash_table_remove (origin->cached_overrides_remove, pkg);

This comment has been minimized.

@cgwalters

cgwalters Jun 5, 2017

Member

g_hash_table_remove returns a boolean, so we can avoid looking things up twice. Obviously doesn't matter here, but seems like general best practice.

@cgwalters

cgwalters Jun 5, 2017

Member

g_hash_table_remove returns a boolean, so we can avoid looking things up twice. Obviously doesn't matter here, but seems like general best practice.

This comment has been minimized.

@jlebon

jlebon Jun 5, 2017

Member

That makes way more sense. I made a separate commit for it to make use of that in the whole file.

@jlebon

jlebon Jun 5, 2017

Member

That makes way more sense. I made a separate commit for it to make use of that in the whole file.

jlebon added some commits Jun 5, 2017

app: add experimental support for pkg removals
This is one more step towards making rpm-ostree more powerful in its
quest to be the ultimate *hybrid* image/package system. Package layering
allows us to add packages on top of the base package set received from
the content provider. However, we're not able to remove or replace
packages in the base set itself.

This patch introduces a new `override` command, which is for now nested
under the experimental `ex` command. The `override` command will allow
users to modify the base package set itself. The first implemented
subcommands are `remove` and `reset`.

A stub has been provided for the more useful `replace` subcommand,
though much of the needed logic for that operation are implemented in
this patch as part of the `remove` subcommand.

Part of: #485
vmcheck: create new test-basic.sh test
And move tests from test-layering-basic.sh that aren't related to
pkglayering there.
origin: avoid double lookup on hash table removals
More efficient *and* prettier! So much win!
@jlebon

This comment has been minimized.

Show comment
Hide comment
@jlebon

jlebon Jun 5, 2017

Member

Rebased and fixups added! ⬆️

Member

jlebon commented Jun 5, 2017

Rebased and fixups added! ⬆️

@cgwalters

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Jun 5, 2017

📋 Looks like this PR is still in progress, ignoring approval

📋 Looks like this PR is still in progress, ignoring approval

@cgwalters

This comment has been minimized.

Show comment
Hide comment
Member

cgwalters commented Jun 5, 2017

@dustymabe

This comment has been minimized.

Show comment
Hide comment
@dustymabe

dustymabe Jun 5, 2017

Contributor

@rh-atomic-bot wtfbbq

remove "WIP" from the title of the PR

Contributor

dustymabe commented Jun 5, 2017

@rh-atomic-bot wtfbbq

remove "WIP" from the title of the PR

@cgwalters cgwalters changed the title from WIP: base override pkg removals to base override pkg removals Jun 5, 2017

@cgwalters

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Jun 5, 2017

⌛️ Testing commit 6de0066 with merge 4e2936f...

⌛️ Testing commit 6de0066 with merge 4e2936f...

rh-atomic-bot added a commit that referenced this pull request Jun 5, 2017

vmcheck: create new test-basic.sh test
And move tests from test-layering-basic.sh that aren't related to
pkglayering there.

Closes: #797
Approved by: cgwalters

rh-atomic-bot added a commit that referenced this pull request Jun 5, 2017

vmcheck: add new test for override remove
Closes: #797
Approved by: cgwalters

rh-atomic-bot added a commit that referenced this pull request Jun 5, 2017

origin: avoid double lookup on hash table removals
More efficient *and* prettier! So much win!

Closes: #797
Approved by: cgwalters
@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Jun 5, 2017

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 4e2936f to master...

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 4e2936f to master...

@jlebon jlebon deleted the jlebon:pr/base-override branch Jun 6, 2017

@cgwalters cgwalters referenced this pull request in rpm-software-management/libdnf Jun 8, 2017

Closed

Add DNF_ALLOW_DOWNGRADE for install and downgrade operations #293

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment