attachment
0001-Ticket-48317-SELinux-port-labeling-retry-attempts-ar.patch

Question:

I guess we need some justification for this change... We worries
what if we wait another second, semanage successfully finishes?
{{{

1014 my $retry = 60;
1014 # 60 is a bit excessive, we should fail faster.
1015 my $retry = 5;
}}}
Answer:
This doesn't mean wait a second, it means try it 60 times, and it takes
about 5 seconds each attempt. That's why I reduced it.

Yes, we understand that. Just our question is how we could determine "5 * sleep(5) == 25 seconds" is long enough. Semanage could finish the task in 26 seconds. (that's what I meant by another second...) Always it is tricky to determine the timelimit.

Retry isn't a time out, it's actually how many times we run semanage. Running semanage 60 times isn't really going to change the output, we either succeed the first time, or we fail over, and over again. It just causes us to waste lots of time when we should be reporting an error to the user.

Arguably, we shouldn't retry at all, and should just report back, but I felt that lowering this value was a good starting point.

Semanage calls upon the following code:

{{{
def start(self):
if semanageRecords.transaction:
raise ValueError(_("Semanage transaction already in progress"))
self.begin()
semanageRecords.transaction = True
}}}

So it will fail if a transaction is already taking place. It doesn't appear to set a unique RC, so we could check for the string to improve this?

However, retrying 60 times still seems like an excessive answer. Maybe 10 or so in light of this? Or change the sleep time to 30 seconds [0]? If we really wanted to improve our changes, we could check for the existance of /etc/selinux/targeted/modules/semanage.trans.LOCK ( alternately, sestatus | grep 'SELinux root directory:' | awk '{print $4}'/sestatus | grep 'Loaded policy name:' | awk '{print $4}'/modules/semanage.trans.LOCK ).

Finally, we could submit a patch to semanage to get it to block and wait on the transaction to unlock before proceeding, so we don't need to manage this ourselves.

[0] semanage commands take about 30 seconds generally in my experience, but a quick demo shows:

{{{
sudo time semanage port -a -p tcp -t ldap_port_t 3389
23.32user 0.61system 0:24.07elapsed 99%CPU

sudo time semanage port -d -p tcp -t ldap_port_t 3389
24.82user 0.57system 0:25.50elapsed 99%CPU
}}}

Agreed.

The retry was introduced by this commit:
commit 4999849
Ticket #502 - setup-ds.pl script should wait if "semanage.trans.LOCK" present
and waiting for much shorter time would be good enough.

Thanks for your research, William!

So what is the verdict of what we want to do here to progress? Accept the patch? Tweak the retry time? Check for the log? Patch semanage?

Replying to [comment:5 firstyear]:

So what is the verdict of what we want to do here to progress? Accept the patch? Tweak the retry time? Check for the log? Patch semanage?

I meant "Accept the patch". Please push the patch. Thanks.

Pushed to master:

commit 94ca981598d2ed6addd438988f71a489fe456769

Thanks for your time Noriko!

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1648

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.