Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

base override pkg removals #797

Closed
wants to merge 5 commits into from
Closed

Conversation

@jlebon
Copy link
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
Copy link

rh-atomic-bot commented May 30, 2017

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

@jlebon jlebon force-pushed the jlebon:pr/base-override branch from 3cb96da to 4912b4b May 31, 2017
@jlebon
Copy link
Member Author

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
Copy link

rh-atomic-bot commented Jun 2, 2017

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

@jlebon jlebon force-pushed the jlebon:pr/base-override branch from bf34de0 to 0adcd46 Jun 5, 2017
@jlebon jlebon removed the WIP label Jun 5, 2017
@jlebon
Copy link
Member Author

jlebon commented Jun 5, 2017

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

Copy link
Member

cgwalters left a comment

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.

Copy link
@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.

Copy link
@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.

* 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.

Copy link
@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.

Copy link
@cgwalters

cgwalters Jun 5, 2017

Member

return

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.

Copy link
@cgwalters

cgwalters Jun 5, 2017

Member

g_autoptr()

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.

Copy link
@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.

Copy link
@jlebon

jlebon Jun 5, 2017

Author 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.

Copy link
@cgwalters

cgwalters Jun 5, 2017

Member

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


{ DECLARE_RPMSIGHANDLER_RESET;
rpmtsOrder (ordering_ts);
}

rpmostree_output_task_begin ("Overlaying");
rpmostree_output_task_begin ("Applying overrides and overlays");

This comment has been minimized.

Copy link
@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.

Copy link
@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.

Copy link
@jlebon

jlebon Jun 5, 2017

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

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.

Copy link
@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.

Copy link
@jlebon

jlebon Jun 5, 2017

Author Member

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

jlebon added 5 commits Jun 5, 2017
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
And move tests from test-layering-basic.sh that aren't related to
pkglayering there.
More efficient *and* prettier! So much win!
@jlebon jlebon force-pushed the jlebon:pr/base-override branch from 0adcd46 to 6de0066 Jun 5, 2017
@jlebon
Copy link
Member Author

jlebon commented Jun 5, 2017

Rebased and fixups added! ⬆️

@cgwalters
Copy link
Member

cgwalters commented Jun 5, 2017

@rh-atomic-bot
Copy link

rh-atomic-bot commented Jun 5, 2017

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

@cgwalters
Copy link
Member

cgwalters commented Jun 5, 2017

@dustymabe
Copy link
Member

dustymabe commented Jun 5, 2017

@rh-atomic-bot wtfbbq

remove "WIP" from the title of the PR

@cgwalters cgwalters changed the title WIP: base override pkg removals base override pkg removals Jun 5, 2017
@cgwalters
Copy link
Member

cgwalters commented Jun 5, 2017

@rh-atomic-bot
Copy link

rh-atomic-bot commented Jun 5, 2017

Testing commit 6de0066 with merge 4e2936f...

rh-atomic-bot added a commit that referenced this pull request Jun 5, 2017
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
Closes: #797
Approved by: cgwalters
rh-atomic-bot added a commit that referenced this pull request Jun 5, 2017
More efficient *and* prettier! So much win!

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

rh-atomic-bot commented Jun 5, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.