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.
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?
[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]