Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge sss_cache and sss_debuglevel into sssctl #274

Conversation

Projects
None yet
5 participants
@justin-stephenson
Copy link
Contributor

commented May 15, 2017

Move sss_cache into a new command sssctl expire-cache and sss_debuglevel into new command sssctl debug-level for ticket https://pagure.io/SSSD/sssd/issue/3057

Shell redirect wrappers replace the sss_{debuglevel,cache} binaries and man pages updated to point admins towards the new sssctl commands.

@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2017

Can one of the admins verify this patch?

1 similar comment
@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2017

Can one of the admins verify this patch?

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

NACK to sss_cache changes.

sss_cache is part of sssd-common but sssctl is part of sssd-tools.
So after this change shell wrapper sss_cache needn't work in some cases due to missing sssctl

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

@lslebodn Yes, you make a good point. Is there a suggested way to handle this or do you prefer to drop the sss_cache commits altogether?

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 19, 2017

I still see value in having sssctl manage the cache expiration because especially for sudo rules, we not only need to expire the rules in the cache, but also tell the deamon to fetch the rules again. There I was thinking of having an interface provided by the IFP responder that the tool could call into. And having a reliable way of expiring sudo rules would be handy, it's something I see mentioned in support cases a log.

@lslebodn will probably have a better idea than I how to solve the packaging problem but what about just moving the sss_cache wrapper to sssd-tools? Of course, upgrades will then be problematic, because if someone doesn't have sss-tools installed, then sss_cache would be 'lost' for this user on upgrade. But I'm not sure I have a better idea short of forcing sss-tools on everyone who had sssd-common previously..

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2017

I am not sure what is the least intrusive way to handle this but moving the sss_cache wrapper to sssd-tools makes sense to me. Also, I can add some information to the wrapper to cover the 'sss_cache lost during upgrade' case:

+#!/bin/sh
+sbindir=@sbindir@
+echo "Due to packaging changes, please make sure the sssd-tools package is installed"
+echo "Redirecting to $sbindir/sssctl expire-cache"
+$sbindir/sssctl expire-cache $@

@lslebodn what do you think about it?

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2017

Extra message is helpful for users but it still will not fix tests or any other automated script.

In theory we could merge the package sssd-tools into sssd-common. But it pull additional dependencies:

sh# dnf install --assumeno /usr/sbin/sssctl
Last metadata expiration check: 0:01:15 ago on Wed Jul 19 16:34:23 2017.
Dependencies resolved.
================================================================================
 Package                 Arch          Version              Repository     Size
================================================================================
Installing:
 sssd-tools              x86_64        1.15.2-5.fc26        fedora        357 k
Installing dependencies:
 libsss_simpleifp        x86_64        1.15.2-5.fc26        fedora         70 k
 python3-sss             x86_64        1.15.2-5.fc26        fedora         90 k
 sssd-dbus               x86_64        1.15.2-5.fc26        fedora        164 k

Transaction Summary
================================================================================
Install  4 Packages

Total download size: 681 k
Installed size: 1.4 M
Operation aborted.

And I would like to do opposite. Reduce sssd-common due to sssd-kcm and sssd-secrets.

ATM the simplest would be to drop removing sss_cache. But we can still have "an allias" sssctl expire-cache -> sss_cache. Because I like name expire_cache more then sss_cache :-)

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

In sssctl we follow (or at least try to) the TOPIC-ACTION convention for naming the commands. So "expire-cache" is not a good name. It should be 'cache-expire'. We already have 'cache' topic in the local data section and I think this command fits there to.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@justin-stephenson justin-stephenson force-pushed the justin-stephenson:jstephen-merge-sss-tools-into-sssctl branch from e70f52b to 206852d Jul 25, 2017

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2017

PR updated:

  • Dropped the sss_cache commits
  • Rebased to master
  • Added new commit adding sssctl cache-expire command as a simple wrapper for sss_cache
@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

There is a tabulator instead of space after '=' sign
in Makefile.am:

 458 
 459 dist_sbin_SCRIPTS = contrib/tools/sss_debuglevel
 460 

But as Lukas mentioned we should not put sss_debuglevel in the tarball directly,
but only the .in version and call the replace_script on it in the Makefile.am,
so that proper paths are expanded in the script (see how replace_script is used
in the Makefile.am for other files). So the configure.ac line should be removed
too.

Also I would not put this to the contrib directory because this is directory
that we use for things that are not packaged in the tarball or nice things that
someone creted but are not core parts of the project (like some helpful sripts
etc). Maybe a new directory src/tools/wrappers would be more appropriate?
@lslebodn do you have any proposal in this regard?

I can see these warnings when I run autoreconf -if indicating you forgot to remove
some lines from Makefile.am:

Makefile.am: installing 'build/depcomp'
Makefile.am:1685: warning: variable 'sss_debuglevel_SOURCES' is defined but no program or
Makefile.am:1685: library has 'sss_debuglevel' as canonical name (possible typo)
Makefile.am:1688: warning: variable 'sss_debuglevel_LDADD' is defined but no program or
Makefile.am:1688: library has 'sss_debuglevel' as canonical name (possible typo)

Bad indentation in src/tools/sssctl/sssctl.h

113 errno_t sssctl_debug_level(struct sss_cmdline *cmdline,
114                           struct sss_tool_ctx *tool_ctx,
115                           void *pvt);
                             ^ bad indentation

In src/tools/sssctl/sssctl_logs.c, the config.h should be the first header that
is included (I think we do not follow this everywhere, but still..)

Another bad indentation in src/tools/sssctl/sssctl.h

101 errno_t sssctl_cache_expire(struct sss_cmdline *cmdline,
102                              struct sss_tool_ctx *tool_ctx,
103                              void *pvt);
104                             ^^^ bad indentation

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2017

Thank you for the review @mzidek-rh - I will make the changes and update the PR.

@justin-stephenson justin-stephenson force-pushed the justin-stephenson:jstephen-merge-sss-tools-into-sssctl branch from 206852d to 278383c Jul 29, 2017

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2017

Patches updated, I believe the Makefile changes are done as Michal proposed but please let me know if anything needs correcting.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2017

I'm sorry this PR review stalled (again :-/) but I've read the patches and I like them now. I have two nitpicks, one was found by Coverity:

Error: CHECKED_RETURN (CWE-252):
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:307: check_return: Calling "sssctl_run_command" without checking return value (as is done elsewhere 7 out of 8 times).
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:108: example_assign: Example 1: Assigning: "ret" = return value from "sssctl_run_command("sss_override user-export /var/lib/sss/backup/sssd_user_overrides.bak")".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:110: example_checked: Example 1 (cont.): "ret" has its value checked in "ret != 0".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:332: example_assign: Example 2: Assigning: "ret" = return value from "sssctl_run_command(cmd)".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:333: example_checked: Example 2 (cont.): "ret" has its value checked in "ret != 0".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:161: example_assign: Example 3: Assigning: "ret" = return value from "sssctl_run_command("sss_override user-import /var/lib/sss/backup/sssd_user_overrides.bak")".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:163: example_checked: Example 3 (cont.): "ret" has its value checked in "ret != 0".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:170: example_assign: Example 4: Assigning: "ret" = return value from "sssctl_run_command("sss_override group-import /var/lib/sss/backup/sssd_group_overrides.bak")".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:172: example_checked: Example 4 (cont.): "ret" has its value checked in "ret != 0".
sssd-1.15.4/src/tools/sssctl/sssctl_logs.c:292: example_assign: Example 5: Assigning: "ret" = return value from "sssctl_run_command(cmd)".
sssd-1.15.4/src/tools/sssctl/sssctl_logs.c:293: example_checked: Example 5 (cont.): "ret" has its value checked in "ret != 0".
#  305|           fprintf(stderr, _("Missing option. \n"));
#  306|           cachecmd = "sss_cache --help";
#  307|->         sssctl_run_command(cachecmd);
#  308|           ret = EXIT_FAILURE;
#  309|           goto done;

And the other I'll point out inline.

goto done;
}

printf(_("Executing command [%s]\n"), cmd);

This comment has been minimized.

Copy link
@jhrozek

jhrozek Sep 14, 2017

Contributor

I would personally hide that we execute the sss_cache command and just remove the printf line. The user doesn't care whether another command is executed and the point is to over time hide and deprecate the other commands except for sssctl.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2017

In addition, distcheck failed:

ERROR: files left in build directory after distclean:
./src/tools/wrappers/sss_debuglevel
make[1]: *** [distcleancheck] Error 1

@justin-stephenson justin-stephenson force-pushed the justin-stephenson:jstephen-merge-sss-tools-into-sssctl branch from 278383c to 988cca7 Sep 19, 2017

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

Thank you for the review @jhrozek

Patchset updated.

@jhrozek
Copy link
Contributor

left a comment

The code looks good to me now and also works fine. I'm just waiting for the CI machines to finish before giving a final ACK.

@jhrozek jhrozek added the Accepted label Sep 19, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2017

@lslebodn lslebodn removed the Accepted label Sep 19, 2017

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2017

few nitpicks: 1st patch "SSSCTL: Replace sss_debuglevel with shell wrapper" should be after introducing command "sssctl debug*" otherwise it will break bisect an may be tests


if (cmdline->argc == 0) {
fprintf(stderr, _("Missing option. \n"));
cachecmd = "sss_cache --help";

This comment has been minimized.

Copy link
@lslebodn

lslebodn Sep 19, 2017

Contributor

Please do not append help here.
Help is already displayed in case of missing arguments

[root@graviton ~]# sss_cache
Usage: sss_cache [-?EUGNSAHR] [-?|--help] [--usage] [-E|--everything]
        [-u|--user=STRING] [-U|--users] [-g|--group=STRING] [-G|--groups]
        [-n|--netgroup=STRING] [-N|--netgroups] [-s|--service=STRING]
        [-S|--services] [-a|--autofs-map=STRING] [-A|--autofs-maps]
        [-h|--ssh-host=STRING] [-H|--ssh-hosts] [-r|--sudo-rule=STRING]
        [-R|--sudo-rules] [-d|--domain=STRING]
Please select at least one object to invalidate

And it will make patch simpler :-)

* so the configuration can be restored on next start.
*/
errno = 0;
if (utime(config_file, NULL) == -1 ) {

This comment has been minimized.

Copy link
@lslebodn

lslebodn Sep 19, 2017

Contributor

It might be caused by git mv but there is space after "-1"

@justin-stephenson justin-stephenson force-pushed the justin-stephenson:jstephen-merge-sss-tools-into-sssctl branch from 988cca7 to 5fa06b9 Sep 19, 2017

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

Thanks for the comments @lslebodn, changes made.


ret = EXIT_FAILURE;
goto done;
}

This comment has been minimized.

Copy link
@lslebodn

lslebodn Sep 21, 2017

Contributor

I'm sorry that I noticed it earlier. Ant it take me quite long to review patches.
But is there a reason why we need to have "reports" to stderr here.

IMHO we should keep it simple and only call sss_cache. All logic is already there.
e.g sss_cache without arguments always return 1. And comment "Unable to expire cache objects" is confusing.
because sss_cache already reports info about usage; it didn't try to invalidate anything

[root@host ~]# sss_cache 
Usage: sss_cache [-?EUGNSAHR] [-?|--help] [--usage] [-E|--everything]
        [-u|--user=STRING] [-U|--users] [-g|--group=STRING] [-G|--groups]
        [-n|--netgroup=STRING] [-N|--netgroups] [-s|--service=STRING]
        [-S|--services] [-a|--autofs-map=STRING] [-A|--autofs-maps]
        [-h|--ssh-host=STRING] [-H|--ssh-hosts] [-r|--sudo-rule=STRING]
        [-R|--sudo-rules] [-d|--domain=STRING]
Please select at least one object to invalidate
[root@host ~]# echo $?
1

ret = sssctl_run_command(cmd);
if (ret != EOK) {
fprintf(stderr, _("Unable to expire cache objects\n"));

This comment has been minimized.

Copy link
@lslebodn

lslebodn Sep 21, 2017

Contributor

I would avoid this message as well due to same reason as in previous comment.
Messages from sss_cache are enough and this message would be confusing in following context.

[root@host ~]# sss_cache --user=dummy
No cache object matched the specified search
[root@host ~]# echo $?
2

Do you agree with me?

justin-stephenson added some commits May 13, 2017

SSSCTL: Replace sss_debuglevel with shell wrapper
The sss_debuglevel binary is replaced by a shell wrapper calling
sssctl debug-level as part of merging sss_debuglevel into sssctl.
The wrapper will redirect sss_debuglevel to the sssctl debug-level
command performing the same task. The sss_debuglevel(8) man page is
updated to indicate that sss_debuglevel is deprecated and functionality
exists now in sssctl.
SSSCTL: Move sss_debuglevel to sssctl debug-level
Move code from sss_debuglevel to sssctl_logs.c and add new debug-logs
sssctl command to perform the same task of changing debug level
dynamically.

POPT_CONTEXT_KEEP_FIRST Flag added to poptGetContext call in
sssctl_debug_level() to fix argument parsing.

Resolves:
https://pagure.io/SSSD/sssd/issue/3057
SSSCTL: Add cache-expire command
Add sssctl cache-expire as a wrapper for the sss_cache utility to
invalidate cached objects.

@justin-stephenson justin-stephenson force-pushed the justin-stephenson:jstephen-merge-sss-tools-into-sssctl branch from 5fa06b9 to 78cdaf4 Sep 21, 2017

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@lslebodn comments addressed, I was primarily following similar sssctl functions as an example.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2017

@justin-stephenson, I did few changes to build each commit separately (make git bisect happy :-)
Are you fine with them?
https://pagure.io/fork/lslebodn/SSSD/sssd/commits/pr274_changes

@justin-stephenson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2017

@lslebodn yes looks good to me, thank you for that.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2017

@lslebodn lslebodn closed this Sep 25, 2017

@lslebodn lslebodn added the Pushed label Sep 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.