On 06/21/2016 04:40 PM, Jakub Hrozek wrote:
On Tue, Jun 21, 2016 at 04:10:41PM +0200, Jakub Hrozek wrote:
> On Tue, Jun 21, 2016 at 03:50:17PM +0200, Jakub Hrozek wrote:
>> On Mon, Jun 20, 2016 at 07:13:10PM +0200, Jakub Hrozek wrote:
>>> Hi,
>>>
>>> Pavel's sssctl tool is at:
>>>
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sssctl
>>>
>>> I'll try to have a first pass at review today in the evening, but I
>>> thought I'll pass on the branch in case anyone else is interested..
>>
>> Hi, I went through the patches today. I pushed some of my review
>> comments to my branch:
>>
https://github.com/jhrozek/sssd/tree/sssctl
>> these are mostly trivial. The only important part is that sss_simpleifp
>> changes are no longer backwards-incompatible to make enterprise distros
>> happy.
>>
>> Other comments:
>> 1) The sssctl tool restarts sssd with a command and only supports
>> systemctl and service. For Beta, I would propose:
>> - if sssd is built with systemd, use systemctl
>> - otherwise, detect /sbin/service during configure time. If it's
>> available, use /sbin/service
>> - otherwise, don't support restarting sssd at all, but tell the
>> admin to restart it
>>
>> For the next release, I would prefer to call systemd D-Bus
>> interface to restart the service instead. This is cross-platform
>> enough. Let other distributions submit patches for restarting the
>> service via another service manager. This would be handled with a
>> 1.14.0 ticket.
>>
>> 2) We should open a ticket to merge sss_cache, sss_debuglevel and
>> maybe others under sssctl. sss_cache would just be a shell wrapper
>> that calls sssctl for some time and eventually removed (in 2.0?)
>>
>> 3) the commands that remove the cache should remove the timestamps
>> cache as well, if it exists (depends on which patches get to master
>> first)
>>
>> 4) removing logs -- is there a reason to not remove *.log but
>> special case child.log as well?
>>
>> 5) CI fails. I haven't ran Coverity yet either.
>
> 6) we also need integration tests but I'm fine with manual testing for
> now. Again, I will file a ticket for 1.14.0
7) The tool must be added to specfile. This blocks the patches.
Done.
8) The tool should get a manpage. This can be added later (although
it
probably means we have another Beta release to have at least some short
string freeze)
9) We should investigate socket-activation for the IFP interface. Again,
a ticket, but not something we should forget about.
10) sssctl user doesn't seem to work with users from subdomains. I
wonder if this would better be solved when we unify the sysdb cache
format?
Yes, I should write a comment on that. Since the way how the names will
be handled are going to change, I didn't want to add a glue code to
create a fully qualified name in case of subdomains.
[root@unidirect ~]# id administrator(a)win.trust.test
uid=300400500(ad_default_admin(a)win.trust.test)
gid=300400500(ad_default_admin(a)win.trust.test)
groups=300400500(ad_default_admin(a)win.trust.test),300400520(group policy
creator owners(a)win.trust.test),300400519(enterprise
admins(a)win.trust.test),300400512(domain
admins(a)win.trust.test),300400518(schema
admins(a)win.trust.test),300400513(domain
users(a)win.trust.test),713600009(ad_admins)
[root@unidirect ~]# sssctl user administrator(a)win.trust.test
User administrator is not present in cache.
Nonetheless, I wouldn't block the inclusion of these patches at this
point over this bug. It's not a regression, just a bug in a new feature.
11) If a backup of database exists, removing database seems to fail:
[root@client ~]# sssctl backup-local-data
SSSD backup of local data already exist, override? (yes/no) [no] asd
Invalid input, please provide either 'yes' or 'no'.
SSSD backup of local data already exist, override? (yes/no) [no] yes
/usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
[root@client ~]# sssctl remove-cache
SSSD must not be running. Stop SSSD now? (yes/no) [yes] yes
Warning: sssd.service changed on disk. Run 'systemctl daemon-reload' to
reload units.
Creating backup of local data...
Unable to create backup of local data, can not remove the cache.
SSSD backup of local data already exist, override? (yes/no) [no]
Fixed, thanks.
It is also capable of printing domain online/offline status.