[Freeipa-devel] [PATCH 477] spec file: Add Requires(pre) on selinux-policy

[Alexander Bokovoy]

On Wed, 26 Aug 2015, Jan Cholasta wrote:
>On 25.8.2015 21:25, Martin Kosek wrote:
>>On 08/25/2015 09:22 PM, Alexander Bokovoy wrote:
>>>On Tue, 25 Aug 2015, Martin Kosek wrote:
>>>>On 08/25/2015 08:59 PM, Alexander Bokovoy wrote:
>>>>>On Tue, 25 Aug 2015, Martin Kosek wrote:
>>>>>>On 08/25/2015 05:37 PM, Alexander Bokovoy wrote:
>>>>>>>On Tue, 25 Aug 2015, Martin Kosek wrote:
>>>>>>>>On 08/25/2015 04:37 PM, Jan Cholasta wrote:
>>>>>>>>>On 25.8.2015 14:50, Alexander Bokovoy wrote:
>>>>>>>>>>On Tue, 25 Aug 2015, Jan Cholasta wrote:
>>>>>>>>>>>On 25.8.2015 14:23, Alexander Bokovoy wrote:
>>>>>>>>>>>>On Tue, 25 Aug 2015, Jan Cholasta wrote:
>>>>>>>>>>>>>Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>>the attached patch fixes
>>>>>>>>>>>>><https://fedorahosted.org/freeipa/ticket/5256>.
>>>>>>>>>>>>>
>>>>>>>>>>>>>Honza
>>>>>>>>>>>>>
>>>>>>>>>>>>>--
>>>>>>>>>>>>>Jan Cholasta
>>>>>>>>>>>>
>>>>>>>>>>>>>From 216be8de30747f80f490d4e91a7cca4af3e767d6 Mon Sep 17
>>>>>>>>>>>>>00:00:00 2001
>>>>>>>>>>>>>From: Jan Cholasta <jcholast at redhat.com>
>>>>>>>>>>>>>Date: Tue, 25 Aug 2015 14:14:25 +0200
>>>>>>>>>>>>>Subject: [PATCH] spec file: Add Requires(pre) on selinux-policy
>>>>>>>>>>>>>
>>>>>>>>>>>>>This prevents ipa-server-upgrade failures on SELinux AVCs
>>>>>>>>>>>>>because of
>>>>>>>>>>>>>old
>>>>>>>>>>>>>selinux-policy version.
>>>>>>>>>>>>>
>>>>>>>>>>>>>https://fedorahosted.org/freeipa/ticket/5256
>>>>>>>>>>>>>---
>>>>>>>>>>>>>freeipa.spec.in | 1 +
>>>>>>>>>>>>>1 file changed, 1 insertion(+)
>>>>>>>>>>>>>
>>>>>>>>>>>>>diff --git a/freeipa.spec.in b/freeipa.spec.in
>>>>>>>>>>>>>index cba91fe..fd73cda 100644
>>>>>>>>>>>>>--- a/freeipa.spec.in
>>>>>>>>>>>>>+++ b/freeipa.spec.in
>>>>>>>>>>>>>@@ -139,6 +139,7 @@ Requires: systemd-units >= 38
>>>>>>>>>>>>>Requires(pre): shadow-utils
>>>>>>>>>>>>>Requires(pre): systemd-units
>>>>>>>>>>>>>Requires(post): systemd-units
>>>>>>>>>>>>>+Requires(pre): selinux-policy >= %{selinux_policy_version}
>>>>>>>>>>>>>Requires: selinux-policy >= %{selinux_policy_version}
>>>>>>>>>>>>>Requires(post): selinux-policy-base
>>>>>>>>>>>>>Requires: slapi-nis >= 0.54.2-1
>>>>>>>>>>>>If we have it in Requires(pre), we don't need it in Requires, as
>>>>>>>>>>>>Requires(pre) is a superset of guarantees that Requires gives
>>>>>>>>>>>>you.
>>>>>>>>>>>
>>>>>>>>>>>Martin (CCed) told me Requires(pre) does not imply Requires.
>>>>>>>>>>See http://rpm.org/api/4.4.2.2/tsort.html (available since 2007):
>>>>>>>>>>----------------
>>>>>>>>>>Since the only way out of a dependency loop is to snip the loop
>>>>>>>>>>somewhere, rpm uses hints from Requires: dependencies to
>>>>>>>>>>distinguish
>>>>>>>>>>co-requisite (these are not needed to install, only to use, a
>>>>>>>>>>package)
>>>>>>>>>>from pre-requisite (these are guaranteed to be installed before
>>>>>>>>>>the
>>>>>>>>>>package that includes the dependency) relations.
>>>>>>>>>>----------------
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>Requires(pre) ensures that selinux-policy of specific version is
>>>>>>>>>>>>installed before pre scripts of freeipa-server would run, be
>>>>>>>>>>>>it in the
>>>>>>>>>>>>same transaction or in a previous one.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>Hmm, ipa-server-upgrade is run in posttrans. Should the
>>>>>>>>>>>Requires(pre)
>>>>>>>>>>>be changed to Required(posttrans)?
>>>>>>>>>>I don't think there is posttrans target. Perhaps, we can just
>>>>>>>>>>make sure
>>>>>>>>>>Requires(post) is enough.
>>>>>>>>>
>>>>>>>>>OK, let's try that. Updated patch attached.
>>>>>>>>>
>>>>>>>>
>>>>>>>>Will this really make a difference? I thought the problem is
>>>>>>>>caused by
>>>>>>>>selinux-policy being installed after freeipa-server package
>>>>>>>>upgrade. We
>>>>>>>>already
>>>>>>>>have Requires on selinux-policy, so I am not sure what is actually
>>>>>>>>changed by
>>>>>>>>this patch.
>>>>>>>The change is that with Requires(pre) or Requires(post) we are
>>>>>>>guaranteed that selinux-policy is installed and available before
>>>>>>>our pre
>>>>>>>or post scriptlets are run. With Requires only we are not
>>>>>>>guaranteed to
>>>>>>>be installed after selinux-policy, only that it would be available as
>>>>>>>part of the same transaction we are installed in.
>>>>>>>
>>>>>>>We don't really need to have Requires(pre) because we don't rely on
>>>>>>>selinux-policy being available in pre scriptlet. Forcing
>>>>>>>Requires(pre)
>>>>>>>doesn't help anyone else (rpm/yum/dnf need to solve dependency
>>>>>>>loops and
>>>>>>>we are only complicating with Requires(pre) if we don't actually need
>>>>>>>it). Thus, choosing Require(post) is more correct from distribution
>>>>>>>point of view.
>>>>>>
>>>>>>Sure, but given that FreeIPA upgrade is run in the posttrans phase:
>>>>>>
>>>>>>%posttrans server
>>>>>># This must be run in posttrans so that updates from previous
>>>>>># execution that may no longer be shipped are not applied.
>>>>>>/usr/sbin/ipa-server-upgrade --quiet >/dev/null || :
>>>>>>
>>>>>>I am now not sure how Requires(pre) or Requires(post) help here, in
>>>>>>all
>>>>>>cases, the right selinux-policy should be there before all the
>>>>>>posttrans
>>>>>>scripts are being run.
>>>>>I've looked at the rpm source code and here is the list of all
>>>>>supported
>>>>>requires/dependencies types:
>>>>>https://github.com/rpm-software-management/rpm/blob/140744377b019e0de81d76d0931c32228d2ed57e/build/rpmfc.c#L1056
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>Requires(posttrans) is there so we could use this one too but it was
>>>>>added only in 4.12-alpha which means it is missing in RHEL/CentOS 7,
>>>>>for
>>>>>example, as they are only up to 4.11.
>>>>>>Maybe the new selinux-policy is required for certmonger itself or
>>>>>>some other
>>>>>>event during upgrade?
>>>>>No, I don't think so. However, we cannot set Requires(posttrans), thus
>>>>>we should be using closest target before it, i.e. Requires(post).
>>>>
>>>>Thank you, but I think I still did not get an answer for my question.
>>>>
>>>>IIUC, the rough rpm process with regards to freeipa-server package
>>>>upgrade,
>>>>it should be in this order:
>>>>
>>>>_
>>>>|
>>>>v
>>>>RPM installs some dependencies of freeipa-server
>>>>|
>>>>V
>>>>RPM installs "Requires(pre)" of freeipa-server
>>>>freeipa-server pre scriptlet runs
>>>>|
>>>>v
>>>>RPM installs freeipa-server
>>>>|
>>>>v
>>>>RPM installs "Requires(post)" of freeipa-server
>>>>freeipa-server post scriptlet runs
>>>>|
>>>>v
>>>>RPM installs some dependencies of freeipa-server
>>>>|
>>>>v
>>>>RPM executes posttrans scriptlets, including "ipa-server-upgrade".
>>>The flow above is not correct. Each scriptlet of the package is executed
>>>when package is installed. In particular, there is no period of waiting
>>>until end of whole transaction to start executing %posttrans scriptlet of
>>>a specific package. RPM only guarantees you that %posttrans scriptlet is
>>>executed as the last thing of this package intall, after all
>>>%post/%postun scriptlets were executed for this package and all triggers
>>>for affected packages were executed.
>>
>>Ah, so this was the key misunderstanding I was having. I thought that
>>all posttrans scripts for all packages are run in the end.
>>
>>>
>>>This does not guarantee that selinux-policy processing would be before
>>>freeipa-server processing unless we explicitly ask for the ordering via
>>>Requires(<stage>) tag. The order of processing packages is affected by
>>>Requires(<stage>) tag, but processing of each single package is still
>>>sequential.
>>>
>>>>My question is, if all the magic happens in the last step, how does
>>>>adding
>>>>(pre) or (post) Requires help, given we already have the "normal"
>>>>Requires?
>>>Because with Requires nothing guarantees selinux-policy is
>>>installed before freeipa-server in the same transaction, we have to use
>>>Requires(<stage>) that would ensure we have the selinux-policy
>>>installation/upgrade completed by the time that stage is reached in
>>>freeipa-server installation/upgrade process. In reality Requires(pre) is
>>>often enough too but not recommended as it also could cause loop
>>>breaking issues.
>>>
>>
>>Thanks for the explanation. If this is the case, the patch should be OK
>>as is indeed and Requires on post should be enough.
>
>OK. Is this an ACK?
At least from my side it is ACK.
-- 
/ Alexander Bokovoy