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

Wed Aug 26 06:03:47 UTC 2015

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?

-- 
Jan Cholasta