-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
All of these patches require Nikolai's "DEBUG Macro Refactoring v3" patches to be applied first.
Patch 0001: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
Patch 0002: Enable sending structured debug logs to journald
We are now able to send structured debug logs to journald, tagged with the code file, line number and domain that the log pertains to. To enable this functionality, SSSD must be configured at build-time with - --with-syslog=journald and must be launched without -f/--debug-to-files
This behavior is nearly identical to how SSSD will function today on a systemd-based system if --debug-to-files is disabled, since it will redirect stdout and stderr into journald. This patch merely enhances the situation to send structured logs instead of simple string messages.
Patch 0003: BUILD: Build with journald support by default on Fedora
The journal provided by systemd gives us structured logging capabilities that we should be taking advantage of.
Note: this patch explicitly does not change the systemd unit file for SSSD. Right now, an administrator will need to manually remove the '-f' from ExecStart in the unit file to send debug logs to journald. I suspect we'll want to discuss this before we make it the default. This patch DOES change the default for sss_log messages to use sd_journal_send() instead of straight log() for those messages that we traditionally sent to the syslog (such as login events). This is code that has been in place for some time now, but has not been the default because we hadn't build with --with-syslog=journald.
On (20/12/13 17:05), Stephen Gallagher wrote:
All of these patches require Nikolai's "DEBUG Macro Refactoring v3" patches to be applied first.
Patch 0001: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
Patch 0002: Enable sending structured debug logs to journald
We are now able to send structured debug logs to journald, tagged with the code file, line number and domain that the log pertains to. To enable this functionality, SSSD must be configured at build-time with --with-syslog=journald and must be launched without -f/--debug-to-files
This behavior is nearly identical to how SSSD will function today on a systemd-based system if --debug-to-files is disabled, since it will redirect stdout and stderr into journald. This patch merely enhances the situation to send structured logs instead of simple string messages.
Patch 0003: BUILD: Build with journald support by default on Fedora
The journal provided by systemd gives us structured logging capabilities that we should be taking advantage of.
Note: this patch explicitly does not change the systemd unit file for SSSD. Right now, an administrator will need to manually remove the '-f' from ExecStart in the unit file to send debug logs to journald. I suspect we'll want to discuss this before we make it the default. This patch DOES change the default for sss_log messages to use sd_journal_send() instead of straight log() for those messages that we traditionally sent to the syslog (such as login events). This is code that has been in place for some time now, but has not been the default because we hadn't build with --with-syslog=journald.
From e52fdecc33b0e8b2df8e983ee145ab421a6fffc2 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher sgallagh@redhat.com Date: Fri, 20 Dec 2013 15:17:19 -0500 Subject: [PATCH 1/3] DEBUG: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
src/tools/selinux.c | 2 +- src/util/debug.c | 8 ++++++-- src/util/util.h | 14 ++++++++++---- 3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/tools/selinux.c b/src/tools/selinux.c index b3565de3de1f35218afbb7e3bf3e3362ae8122ec..5905461ca1d09673409cc05d519b6c00a83ca3d9 100644 --- a/src/tools/selinux.c +++ b/src/tools/selinux.c @@ -121,7 +121,7 @@ static void sss_semanage_error_callback(void *varg, return; }
- debug_fn("libsemanage", level, "%s\n", message);
- debug_fn(__FILE__, __LINE__, "libsemanage", level, "%s\n", message); free(message);
}
diff --git a/src/util/debug.c b/src/util/debug.c index 0aff2da9e5eaeb468c9502d2686f78da9cc31056..b3304a859e91b2509746c5ffbd101cf73b711fce 100644 --- a/src/util/debug.c +++ b/src/util/debug.c @@ -128,7 +128,11 @@ static void debug_printf(const char *format, ...) va_end(ap); }
-void debug_fn(const char *function, int newlevel, const char *format, ...) +void debug_fn(const char *file,
long line,
const char *function,
int newlevel,
const char *format, ...)
{ va_list ap; if (debug_timestamps) { @@ -190,7 +194,7 @@ void ldb_debug_messages(void *context, enum ldb_debug_level level, return; }
- debug_fn("ldb", loglevel, "%s\n", message);
debug_fn(__FILE__, __LINE__, "ldb", loglevel, "%s\n", message);
free(message);
} diff --git a/src/util/util.h b/src/util/util.h index 7d9a5264d461e3bcf511187320050936f44da70f..b3bb740f7780ce71f6726099d51ccac0065d3c9d 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -68,8 +68,11 @@ extern int debug_timestamps; extern int debug_microseconds; extern int debug_to_file; extern const char *debug_log_file; -void debug_fn(const char *function, int newlevel, const char *format, ...)
SSS_ATTRIBUTE_PRINTF(3, 4);
+void debug_fn(const char *file,
long line,
const char *function,
int newlevel,
const char *format, ...);
int debug_convert_old_level(int old_level); errno_t set_debug_file_from_fd(const int fd);
NACK
Why do you removed SSS_ATTRIBUTE_PRINTF ?
We had some crashes in sssd because of wrong debug message. For example: e9ea740625881291893b006ffad906d4861c668d "Fix segfault in DYNDNS"
LS
On Dec 21, 2013, at 1:25 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (20/12/13 17:05), Stephen Gallagher wrote: All of these patches require Nikolai's "DEBUG Macro Refactoring v3" patches to be applied first.
Patch 0001: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
Patch 0002: Enable sending structured debug logs to journald
We are now able to send structured debug logs to journald, tagged with the code file, line number and domain that the log pertains to. To enable this functionality, SSSD must be configured at build-time with --with-syslog=journald and must be launched without -f/--debug-to-files
This behavior is nearly identical to how SSSD will function today on a systemd-based system if --debug-to-files is disabled, since it will redirect stdout and stderr into journald. This patch merely enhances the situation to send structured logs instead of simple string messages.
Patch 0003: BUILD: Build with journald support by default on Fedora
The journal provided by systemd gives us structured logging capabilities that we should be taking advantage of.
Note: this patch explicitly does not change the systemd unit file for SSSD. Right now, an administrator will need to manually remove the '-f' from ExecStart in the unit file to send debug logs to journald. I suspect we'll want to discuss this before we make it the default. This patch DOES change the default for sss_log messages to use sd_journal_send() instead of straight log() for those messages that we traditionally sent to the syslog (such as login events). This is code that has been in place for some time now, but has not been the default because we hadn't build with --with-syslog=journald.
From e52fdecc33b0e8b2df8e983ee145ab421a6fffc2 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher sgallagh@redhat.com Date: Fri, 20 Dec 2013 15:17:19 -0500 Subject: [PATCH 1/3] DEBUG: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
src/tools/selinux.c | 2 +- src/util/debug.c | 8 ++++++-- src/util/util.h | 14 ++++++++++---- 3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/tools/selinux.c b/src/tools/selinux.c index b3565de3de1f35218afbb7e3bf3e3362ae8122ec..5905461ca1d09673409cc05d519b6c00a83ca3d9 100644 --- a/src/tools/selinux.c +++ b/src/tools/selinux.c @@ -121,7 +121,7 @@ static void sss_semanage_error_callback(void *varg, return; }
- debug_fn("libsemanage", level, "%s\n", message);
- debug_fn(__FILE__, __LINE__, "libsemanage", level, "%s\n", message); free(message);
}
diff --git a/src/util/debug.c b/src/util/debug.c index 0aff2da9e5eaeb468c9502d2686f78da9cc31056..b3304a859e91b2509746c5ffbd101cf73b711fce 100644 --- a/src/util/debug.c +++ b/src/util/debug.c @@ -128,7 +128,11 @@ static void debug_printf(const char *format, ...) va_end(ap); }
-void debug_fn(const char *function, int newlevel, const char *format, ...) +void debug_fn(const char *file,
long line,
const char *function,
int newlevel,
const char *format, ...)
{ va_list ap; if (debug_timestamps) { @@ -190,7 +194,7 @@ void ldb_debug_messages(void *context, enum ldb_debug_level level, return; }
- debug_fn("ldb", loglevel, "%s\n", message);
debug_fn(__FILE__, __LINE__, "ldb", loglevel, "%s\n", message);
free(message);
} diff --git a/src/util/util.h b/src/util/util.h index 7d9a5264d461e3bcf511187320050936f44da70f..b3bb740f7780ce71f6726099d51ccac0065d3c9d 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -68,8 +68,11 @@ extern int debug_timestamps; extern int debug_microseconds; extern int debug_to_file; extern const char *debug_log_file; -void debug_fn(const char *function, int newlevel, const char *format, ...)
SSS_ATTRIBUTE_PRINTF(3, 4);
+void debug_fn(const char *file,
long line,
const char *function,
int newlevel,
const char *format, ...);
int debug_convert_old_level(int old_level); errno_t set_debug_file_from_fd(const int fd);
NACK
Why do you removed SSS_ATTRIBUTE_PRINTF ?
We had some crashes in sssd because of wrong debug message. For example: e9ea740625881291893b006ffad906d4861c668d "Fix segfault in DYNDNS"
I think that was probably me hitting ctrl-d one too many times and not noticing that I removed it. It wasn't intentional. I'll send a new version of the patch with that added back in when I am back from vacation in the new year (or it can be fixed at push, if someone else could test and verify it doesn't break anything.)
On Mon, Dec 23, 2013 at 08:01:15AM -0500, Stephen Gallagher wrote:
On Dec 21, 2013, at 1:25 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (20/12/13 17:05), Stephen Gallagher wrote: All of these patches require Nikolai's "DEBUG Macro Refactoring v3" patches to be applied first.
Patch 0001: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
ACK
Patch 0002: Enable sending structured debug logs to journald
We are now able to send structured debug logs to journald, tagged with the code file, line number and domain that the log pertains to. To enable this functionality, SSSD must be configured at build-time with --with-syslog=journald and must be launched without -f/--debug-to-files
This behavior is nearly identical to how SSSD will function today on a systemd-based system if --debug-to-files is disabled, since it will redirect stdout and stderr into journald. This patch merely enhances the situation to send structured logs instead of simple string messages.
ACK to the code but I wonder if it would be nicer to put the whole block that actually prints to journal into its own function?
Patch 0003: BUILD: Build with journald support by default on Fedora
The journal provided by systemd gives us structured logging capabilities that we should be taking advantage of.
Note: this patch explicitly does not change the systemd unit file for SSSD. Right now, an administrator will need to manually remove the '-f' from ExecStart in the unit file to send debug logs to journald.
I agree that not removing -f is the right thing to do. At the very least, I think we should amend our wiki and document how the administrator should obtain the debug logs.
suspect we'll want to discuss this before we make it the default. This patch DOES change the default for sss_log messages to use sd_journal_send() instead of straight log() for those messages that we traditionally sent to the syslog (such as login events). This is code that has been in place for some time now, but has not been the default because we hadn't build with --with-syslog=journald.
ACK to this patch. Let me know your preference about splitting debug_fn, otherwise I think this patch is good to go along with re-adding the PRINTF_ATTRIBUTE.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/07/2014 10:22 AM, Jakub Hrozek wrote:
On Mon, Dec 23, 2013 at 08:01:15AM -0500, Stephen Gallagher wrote:
On Dec 21, 2013, at 1:25 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (20/12/13 17:05), Stephen Gallagher wrote: All of these patches require Nikolai's "DEBUG Macro Refactoring v3" patches to be applied first.
Patch 0001: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
ACK
Patch 0002: Enable sending structured debug logs to journald
We are now able to send structured debug logs to journald, tagged with the code file, line number and domain that the log pertains to. To enable this functionality, SSSD must be configured at build-time with --with-syslog=journald and must be launched without -f/--debug-to-files
This behavior is nearly identical to how SSSD will function today on a systemd-based system if --debug-to-files is disabled, since it will redirect stdout and stderr into journald. This patch merely enhances the situation to send structured logs instead of simple string messages.
ACK to the code but I wonder if it would be nicer to put the whole block that actually prints to journal into its own function?
Done. I created a new static function 'journal_send()' in debug.c
Patch 0003: BUILD: Build with journald support by default on Fedora
The journal provided by systemd gives us structured logging capabilities that we should be taking advantage of.
Note: this patch explicitly does not change the systemd unit file for SSSD. Right now, an administrator will need to manually remove the '-f' from ExecStart in the unit file to send debug logs to journald.
I agree that not removing -f is the right thing to do. At the very least, I think we should amend our wiki and document how the administrator should obtain the debug logs.
I've added a new patch 0004 that will create a systemd service file override in /etc/systemd/system/sssd.service.d/journal.conf
This file will have its contents commented out by default, but if it is uncommented, the effect will be to redirect output to the journal. This should vastly simplify the effort required.
Simo recommended that we may want to reverse the behavior (have the journal be the standard destination if --with-syslog=journald was passed to configure) and make the override capable of reverting to the old behavior. This might have some real value (particularly for RHEL 7.0) so that we can migrate people easier while giving them an easy way to get the old behavior back.
If we prefer to do it that way, I can quickly amend patch 0004 to do this.
suspect we'll want to discuss this before we make it the default. This patch DOES change the default for sss_log messages to use sd_journal_send() instead of straight log() for those messages that we traditionally sent to the syslog (such as login events). This is code that has been in place for some time now, but has not been the default because we hadn't build with --with-syslog=journald.
ACK to this patch. Let me know your preference about splitting debug_fn, otherwise I think this patch is good to go along with re-adding the PRINTF_ATTRIBUTE.
I've fixed the PRINTF_ATTRIBUTE and split the debug_fn. New patches attached.
On Tue, Jan 07, 2014 at 07:55:59PM -0500, Stephen Gallagher wrote:
I've added a new patch 0004 that will create a systemd service file override in /etc/systemd/system/sssd.service.d/journal.conf
This file will have its contents commented out by default, but if it is uncommented, the effect will be to redirect output to the journal. This should vastly simplify the effort required.
Simo recommended that we may want to reverse the behavior (have the journal be the standard destination if --with-syslog=journald was passed to configure) and make the override capable of reverting to the old behavior. This might have some real value (particularly for RHEL 7.0) so that we can migrate people easier while giving them an easy way to get the old behavior back.
If we prefer to do it that way, I can quickly amend patch 0004 to do this.
I'm fine with defaulting to journald in the next major upstream release, but RHEL-7.0 Beta was already out. I think *only* printing debug messages to journald by default is too much of a change at this point..
suspect we'll want to discuss this before we make it the default. This patch DOES change the default for sss_log messages to use sd_journal_send() instead of straight log() for those messages that we traditionally sent to the syslog (such as login events). This is code that has been in place for some time now, but has not been the default because we hadn't build with --with-syslog=journald.
ACK to this patch. Let me know your preference about splitting debug_fn, otherwise I think this patch is good to go along with re-adding the PRINTF_ATTRIBUTE.
I've fixed the PRINTF_ATTRIBUTE and split the debug_fn. New patches attached.
Thank you, unfortunately these patches don't apply cleanly on top of the latest version of Nikolai's patches, can you rebase them? I acked Nikolai's patches, so hopefully this would be the last rebase..
So far I only read the diffs and they look good to me.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/13/2014 10:09 AM, Jakub Hrozek wrote:
On Tue, Jan 07, 2014 at 07:55:59PM -0500, Stephen Gallagher wrote:
I've added a new patch 0004 that will create a systemd service file override in /etc/systemd/system/sssd.service.d/journal.conf
This file will have its contents commented out by default, but if it is uncommented, the effect will be to redirect output to the journal. This should vastly simplify the effort required.
Simo recommended that we may want to reverse the behavior (have the journal be the standard destination if --with-syslog=journald was passed to configure) and make the override capable of reverting to the old behavior. This might have some real value (particularly for RHEL 7.0) so that we can migrate people easier while giving them an easy way to get the old behavior back.
If we prefer to do it that way, I can quickly amend patch 0004 to do this.
I'm fine with defaulting to journald in the next major upstream release, but RHEL-7.0 Beta was already out. I think *only* printing debug messages to journald by default is too much of a change at this point..
I'm leaving patch 0004 as-is for now. It should be pretty obvious what needs to happen to reverse it if-and-when you're ready to do so.
As for RHEL 7.0, the beta is not the final release. I leave it up to you to decide whether that's sufficient argument for moving now rather than trying to move post-final.
suspect we'll want to discuss this before we make it the default. This patch DOES change the default for sss_log messages to use sd_journal_send() instead of straight log() for those messages that we traditionally sent to the syslog (such as login events). This is code that has been in place for some time now, but has not been the default because we hadn't build with --with-syslog=journald.
ACK to this patch. Let me know your preference about splitting debug_fn, otherwise I think this patch is good to go along with re-adding the PRINTF_ATTRIBUTE.
I've fixed the PRINTF_ATTRIBUTE and split the debug_fn. New patches attached.
Thank you, unfortunately these patches don't apply cleanly on top of the latest version of Nikolai's patches, can you rebase them? I acked Nikolai's patches, so hopefully this would be the last rebase..
So far I only read the diffs and they look good to me.
Ask and ye shall receive. Rebased patches attached.
On Mon, Jan 13, 2014 at 02:51:59PM -0500, Stephen Gallagher wrote:
As for RHEL 7.0, the beta is not the final release. I leave it up to you to decide whether that's sufficient argument for moving now rather than trying to move post-final.
I do agree it's easier to move at x.0 release rather than post-release :-) but there is a process to follow in RHEL. For now I filed https://fedorahosted.org/sssd/ticket/2195 so all parties involved in RHEL process can discuss the enhancement.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/14/2014 06:12 AM, Jakub Hrozek wrote:
On Mon, Jan 13, 2014 at 02:51:59PM -0500, Stephen Gallagher wrote:
As for RHEL 7.0, the beta is not the final release. I leave it up to you to decide whether that's sufficient argument for moving now rather than trying to move post-final.
I do agree it's easier to move at x.0 release rather than post-release :-) but there is a process to follow in RHEL. For now I filed https://fedorahosted.org/sssd/ticket/2195 so all parties involved in RHEL process can discuss the enhancement.
Attached patches are rebased atop the debug_macro_refactoring_v5 patches I just sent to the "[SSSD] DEBUG macro refactoring v4" thread (which in retrospect, I should have updated the subject line on as well).
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/12/2014 10:01 AM, Stephen Gallagher wrote:
On 01/14/2014 06:12 AM, Jakub Hrozek wrote:
On Mon, Jan 13, 2014 at 02:51:59PM -0500, Stephen Gallagher wrote:
As for RHEL 7.0, the beta is not the final release. I leave it up to you to decide whether that's sufficient argument for moving now rather than trying to move post-final.
I do agree it's easier to move at x.0 release rather than post-release :-) but there is a process to follow in RHEL. For now I filed https://fedorahosted.org/sssd/ticket/2195 so all parties involved in RHEL process can discuss the enhancement.
Attached patches are rebased atop the debug_macro_refactoring_v5 patches I just sent to the "[SSSD] DEBUG macro refactoring v4" thread (which in retrospect, I should have updated the subject line on as well).
I realized I made a mistake in the rebasing above. I fixed that and then needed to do another manual rebase of these patches atop it. These patches apply atop the "[SSSD] DEBUG macro refactoring v6" patches.
On Wed, Feb 12, 2014 at 10:38:52AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/12/2014 10:01 AM, Stephen Gallagher wrote:
On 01/14/2014 06:12 AM, Jakub Hrozek wrote:
On Mon, Jan 13, 2014 at 02:51:59PM -0500, Stephen Gallagher wrote:
As for RHEL 7.0, the beta is not the final release. I leave it up to you to decide whether that's sufficient argument for moving now rather than trying to move post-final.
I do agree it's easier to move at x.0 release rather than post-release :-) but there is a process to follow in RHEL. For now I filed https://fedorahosted.org/sssd/ticket/2195 so all parties involved in RHEL process can discuss the enhancement.
Attached patches are rebased atop the debug_macro_refactoring_v5 patches I just sent to the "[SSSD] DEBUG macro refactoring v4" thread (which in retrospect, I should have updated the subject line on as well).
I realized I made a mistake in the rebasing above. I fixed that and then needed to do another manual rebase of these patches atop it. These patches apply atop the "[SSSD] DEBUG macro refactoring v6" patches.
These patches work for me, so ACK.
I have one question mostly about journalctl usability -- is it possible to include select fields in the journalctl output? For instance, I'd like to see the log message, the function and the code line. By default, journalctl only gives me the message, but I can see (in JSON output) that all journalctl knows all the data.
On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek wrote:
I realized I made a mistake in the rebasing above. I fixed that and then needed to do another manual rebase of these patches atop it. These patches apply atop the "[SSSD] DEBUG macro refactoring v6" patches.
These patches work for me, so ACK.
Pushed to master.
On (19/02/14 12:37), Jakub Hrozek wrote:
On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek wrote:
I realized I made a mistake in the rebasing above. I fixed that and then needed to do another manual rebase of these patches atop it. These patches apply atop the "[SSSD] DEBUG macro refactoring v6" patches.
These patches work for me, so ACK.
Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
It is really strange, because build for i386 on fedora 20 passed without any problem. http://koji.fedoraproject.org/koji/taskinfo?taskID=6548365
I tried some mock builds and result is: fedora 19 x86_64 -- no problem fedora 20 x86_64 -- the same problem like in koji fedora rawhide x86_64 -- no problem
Tests passed on fedora 20 without these patches. (sprm generated from commit a37bd3e4ad3e65c7011f29384f54aa15ac3311aa)
Attaching find_uid-tests.log
LS
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote:
On (19/02/14 12:37), Jakub Hrozek wrote:
On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek wrote:
I realized I made a mistake in the rebasing above. I fixed that and then needed to do another manual rebase of these patches atop it. These patches apply atop the "[SSSD] DEBUG macro refactoring v6" patches.
These patches work for me, so ACK.
Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
bye, Sumit
It is really strange, because build for i386 on fedora 20 passed without any problem. http://koji.fedoraproject.org/koji/taskinfo?taskID=6548365
I tried some mock builds and result is: fedora 19 x86_64 -- no problem fedora 20 x86_64 -- the same problem like in koji fedora rawhide x86_64 -- no problem
Tests passed on fedora 20 without these patches. (sprm generated from commit a37bd3e4ad3e65c7011f29384f54aa15ac3311aa)
Attaching find_uid-tests.log
LS
Running suite(s): find_uid Proc file [ 1272 kB VmData: 448 kB VmStk: 136 kB VmExe: 16 kB VmLib: 5168 kB VmPTE: 112 kB VmSwap: 0 kB Threads: 1 SigQ: 3/61566 SigPnd: 0000000000000000 ShdPnd: 0000000000000000 SigBlk: 0000000000000000 SigIgn: 0000000001000000 SigCgt: 0000000180002000 CapInh: 0000000000000000 CapPrm: 0000000000000000 CapEff: 0000000000000000 CapBnd: 0000001fffffffff Seccomp: 0 Cpus_allowed: ff Cpus_allowed_list: 0-7 Mems_allowed: 00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000001 Mems_allowed_list: 0 voluntary_ctxt_switches: 21 nonvoluntary_ctxt_switches: 2
tches: 2 50274 @?ٷ] is not available anymore, continuing. 66%: Checks: 3, Failures: 0, Errors: 1 src/tests/find_uid-tests.c:50:E:find_uid:test_check_if_uid_is_active_fail:0: (after this point) Received signal 11 (Segmentation fault)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote:
On (19/02/14 12:37), Jakub Hrozek wrote:
On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek wrote:
I realized I made a mistake in the rebasing above. I fixed that and then needed to do another manual rebase of these patches atop it. These patches apply atop the "[SSSD] DEBUG macro refactoring v6" patches.
These patches work for me, so ACK.
Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
LS
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote:
On (19/02/14 12:37), Jakub Hrozek wrote:
On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek wrote:
I realized I made a mistake in the rebasing above. I fixed that and then needed to do another manual rebase of these patches atop it. These patches apply atop the "[SSSD] DEBUG macro refactoring v6" patches.
These patches work for me, so ACK.
Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote:
On (19/02/14 12:37), Jakub Hrozek wrote:
On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek wrote:
> I realized I made a mistake in the rebasing above. I > fixed that and then needed to do another manual rebase of > these patches atop it. These patches apply atop the > "[SSSD] DEBUG macro refactoring v6" patches.
These patches work for me, so ACK.
Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote:
On (19/02/14 12:37), Jakub Hrozek wrote:
On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek wrote: >> I realized I made a mistake in the rebasing above. I >> fixed that and then needed to do another manual rebase of >> these patches atop it. These patches apply atop the >> "[SSSD] DEBUG macro refactoring v6" patches. > > These patches work for me, so ACK.
Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Wouldn't it be simpler to just set debug_file = stderr ?
(although I guess we may need to protect from trying to manipulate the fd in unsafe ways then)
Simo.
On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote:
On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote:
On (19/02/14 12:37), Jakub Hrozek wrote: > On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek > wrote: >>> I realized I made a mistake in the rebasing above. I >>> fixed that and then needed to do another manual rebase of >>> these patches atop it. These patches apply atop the >>> "[SSSD] DEBUG macro refactoring v6" patches. >> >> These patches work for me, so ACK. > > Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Wouldn't it be simpler to just set debug_file = stderr ?
I tried to go this way, but it seems actually harder to me..
To summarize: with "sssd -f" debug_file should be a log file with "sssd" debug_file should be NULL, indicating journald with "sssd -i", debug_file should be stderr, indicating stderr.
The problem is, that we don't pass the "-i" flag down to the responders and back ends when forking them. So the alternative would be to add "-i" to sssd_be and all responders, pass the flag to the child processes and only set debug_file to stderr if '-i' is present. If you prefer this way, I can prepare a patch along these lines.
But then we also terminate the process which is running with "-i" if its stdin goes away, I guess it would be safe to keep this logic since stdin in the child processes is /dev/null already, I just wanted to mention there is a number of cases to test..
On Tue, Jun 03, 2014 at 09:53:13AM +0200, Jakub Hrozek wrote:
On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote:
On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote: > On (19/02/14 12:37), Jakub Hrozek wrote: >> On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek >> wrote: >>>> I realized I made a mistake in the rebasing above. I >>>> fixed that and then needed to do another manual rebase of >>>> these patches atop it. These patches apply atop the >>>> "[SSSD] DEBUG macro refactoring v6" patches. >>> >>> These patches work for me, so ACK. >> >> Pushed to master. > > find_uid-tests failed with these patches on fedora20. > http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
>
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Wouldn't it be simpler to just set debug_file = stderr ?
I tried to go this way, but it seems actually harder to me..
To summarize: with "sssd -f" debug_file should be a log file with "sssd" debug_file should be NULL, indicating journald with "sssd -i", debug_file should be stderr, indicating stderr.
The problem is, that we don't pass the "-i" flag down to the responders and back ends when forking them. So the alternative would be to add "-i" to sssd_be and all responders, pass the flag to the child processes and only set debug_file to stderr if '-i' is present. If you prefer this way, I can prepare a patch along these lines.
But then we also terminate the process which is running with "-i" if its stdin goes away, I guess it would be safe to keep this logic since stdin in the child processes is /dev/null already, I just wanted to mention there is a number of cases to test..
Any opinions? I'd like to fix this bug before tagging 1.12.0..
On Mon, Jul 07, 2014 at 03:21:54PM +0200, Jakub Hrozek wrote:
On Tue, Jun 03, 2014 at 09:53:13AM +0200, Jakub Hrozek wrote:
On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote:
On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote: > On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote: >> On (19/02/14 12:37), Jakub Hrozek wrote: >>> On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek >>> wrote: >>>>> I realized I made a mistake in the rebasing above. I >>>>> fixed that and then needed to do another manual rebase of >>>>> these patches atop it. These patches apply atop the >>>>> "[SSSD] DEBUG macro refactoring v6" patches. >>>> >>>> These patches work for me, so ACK. >>> >>> Pushed to master. >> >> find_uid-tests failed with these patches on fedora20. >> http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log > > >>
Do you have additional patches in this build?
> > /builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm > > >
The git hash does not look like anything from master.
> Yes, I was testing some patches.
> I started a koji build > http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with > current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which > is working fine. > I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Wouldn't it be simpler to just set debug_file = stderr ?
I tried to go this way, but it seems actually harder to me..
To summarize: with "sssd -f" debug_file should be a log file with "sssd" debug_file should be NULL, indicating journald with "sssd -i", debug_file should be stderr, indicating stderr.
The problem is, that we don't pass the "-i" flag down to the responders and back ends when forking them. So the alternative would be to add "-i" to sssd_be and all responders, pass the flag to the child processes and only set debug_file to stderr if '-i' is present. If you prefer this way, I can prepare a patch along these lines.
But then we also terminate the process which is running with "-i" if its stdin goes away, I guess it would be safe to keep this logic since stdin in the child processes is /dev/null already, I just wanted to mention there is a number of cases to test..
Any opinions? I'd like to fix this bug before tagging 1.12.0..
Maybe having an option --debug-to-stdout would be nicer for the child processes because it fits better to the existing SSSD_DEBUG_OPTS and will have no additional meaning as -i? For the monitor this option should be rejected and hidden from the help.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Jul 07, 2014 at 03:57:28PM +0200, Sumit Bose wrote:
On Mon, Jul 07, 2014 at 03:21:54PM +0200, Jakub Hrozek wrote:
On Tue, Jun 03, 2014 at 09:53:13AM +0200, Jakub Hrozek wrote:
On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote:
On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote: > On (19/02/14 22:03), Sumit Bose wrote: >> On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote: >>> On (19/02/14 12:37), Jakub Hrozek wrote: >>>> On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek >>>> wrote: >>>>>> I realized I made a mistake in the rebasing above. I >>>>>> fixed that and then needed to do another manual rebase of >>>>>> these patches atop it. These patches apply atop the >>>>>> "[SSSD] DEBUG macro refactoring v6" patches. >>>>> >>>>> These patches work for me, so ACK. >>>> >>>> Pushed to master. >>> >>> find_uid-tests failed with these patches on fedora20. >>> http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log >> >> >>> Do you have additional patches in this build? >> >> /builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm >> >> >> The git hash does not look like anything from master. >> > Yes, I was testing some patches. > >> I started a koji build >> http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with >> current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which >> is working fine. >> > I tried one more time with master and koji works fine. > Unfortunately, I am able to reproduce it locally in mock :-( >
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Wouldn't it be simpler to just set debug_file = stderr ?
I tried to go this way, but it seems actually harder to me..
To summarize: with "sssd -f" debug_file should be a log file with "sssd" debug_file should be NULL, indicating journald with "sssd -i", debug_file should be stderr, indicating stderr.
The problem is, that we don't pass the "-i" flag down to the responders and back ends when forking them. So the alternative would be to add "-i" to sssd_be and all responders, pass the flag to the child processes and only set debug_file to stderr if '-i' is present. If you prefer this way, I can prepare a patch along these lines.
But then we also terminate the process which is running with "-i" if its stdin goes away, I guess it would be safe to keep this logic since stdin in the child processes is /dev/null already, I just wanted to mention there is a number of cases to test..
Any opinions? I'd like to fix this bug before tagging 1.12.0..
Maybe having an option --debug-to-stdout would be nicer for the child processes because it fits better to the existing SSSD_DEBUG_OPTS and will have no additional meaning as -i? For the monitor this option should be rejected and hidden from the help.
bye, Sumit
Perhaps..
to clarify, you're suggesting that --debug-to-stdout would be added to all processes (monitor and child), but only child processes in libexec would expose it and this option would be used when spawning the child processes?
I wouldn't like to change the meaning of "-i" for the monitor process, though..but there we can detect sssd is running on the foreground.
On Mon, Jul 07, 2014 at 10:20:21PM +0200, Jakub Hrozek wrote:
On Mon, Jul 07, 2014 at 03:57:28PM +0200, Sumit Bose wrote:
On Mon, Jul 07, 2014 at 03:21:54PM +0200, Jakub Hrozek wrote:
On Tue, Jun 03, 2014 at 09:53:13AM +0200, Jakub Hrozek wrote:
On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote:
On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 02/19/2014 05:18 PM, Lukas Slebodnik wrote: > > On (19/02/14 22:03), Sumit Bose wrote: > >> On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote: > >>> On (19/02/14 12:37), Jakub Hrozek wrote: > >>>> On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek > >>>> wrote: > >>>>>> I realized I made a mistake in the rebasing above. I > >>>>>> fixed that and then needed to do another manual rebase of > >>>>>> these patches atop it. These patches apply atop the > >>>>>> "[SSSD] DEBUG macro refactoring v6" patches. > >>>>> > >>>>> These patches work for me, so ACK. > >>>> > >>>> Pushed to master. > >>> > >>> find_uid-tests failed with these patches on fedora20. > >>> http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log > >> > >> > >>> > Do you have additional patches in this build? > >> > >> /builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm > >> > >> > >> > The git hash does not look like anything from master. > >> > > Yes, I was testing some patches. > > > >> I started a koji build > >> http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with > >> current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which > >> is working fine. > >> > > I tried one more time with master and koji works fine. > > Unfortunately, I am able to reproduce it locally in mock :-( > > > > > I've seen that happen intermittently as well in mock and koji. I > suspect there may be a bug/race condition in sd_uid_get_sessions(), > but I can't reproduce it consistently (and the DEBUG error doesn't > seem to fire...) > > A quick look at the code suggests that we're probably missing a > 'return EOK' after '*result = false', though. > > I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Wouldn't it be simpler to just set debug_file = stderr ?
I tried to go this way, but it seems actually harder to me..
To summarize: with "sssd -f" debug_file should be a log file with "sssd" debug_file should be NULL, indicating journald with "sssd -i", debug_file should be stderr, indicating stderr.
The problem is, that we don't pass the "-i" flag down to the responders and back ends when forking them. So the alternative would be to add "-i" to sssd_be and all responders, pass the flag to the child processes and only set debug_file to stderr if '-i' is present. If you prefer this way, I can prepare a patch along these lines.
But then we also terminate the process which is running with "-i" if its stdin goes away, I guess it would be safe to keep this logic since stdin in the child processes is /dev/null already, I just wanted to mention there is a number of cases to test..
Any opinions? I'd like to fix this bug before tagging 1.12.0..
Maybe having an option --debug-to-stdout would be nicer for the child processes because it fits better to the existing SSSD_DEBUG_OPTS and will have no additional meaning as -i? For the monitor this option should be rejected and hidden from the help.
bye, Sumit
Perhaps..
to clarify, you're suggesting that --debug-to-stdout would be added to all processes (monitor and child), but only child processes in libexec would expose it and this option would be used when spawning the child processes?
yes, my idea was to add it to SSSD_DEBUG_OPTS which would make it immediately available to all processes. But having it for the monitor make no sense because '-D --debug-to-stdout' and '-f --debug-to-stdout' are config errors and '-i --debug-to-stdout' is a no-op.
bye, Sumit
I wouldn't like to change the meaning of "-i" for the monitor process, though..but there we can detect sssd is running on the foreground. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Jul 08, 2014 at 09:53:50AM +0200, Sumit Bose wrote:
On Mon, Jul 07, 2014 at 10:20:21PM +0200, Jakub Hrozek wrote:
On Mon, Jul 07, 2014 at 03:57:28PM +0200, Sumit Bose wrote:
On Mon, Jul 07, 2014 at 03:21:54PM +0200, Jakub Hrozek wrote:
On Tue, Jun 03, 2014 at 09:53:13AM +0200, Jakub Hrozek wrote:
On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote:
On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote: > On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote: > > -----BEGIN PGP SIGNED MESSAGE----- > > Hash: SHA1 > > > > On 02/19/2014 05:18 PM, Lukas Slebodnik wrote: > > > On (19/02/14 22:03), Sumit Bose wrote: > > >> On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote: > > >>> On (19/02/14 12:37), Jakub Hrozek wrote: > > >>>> On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek > > >>>> wrote: > > >>>>>> I realized I made a mistake in the rebasing above. I > > >>>>>> fixed that and then needed to do another manual rebase of > > >>>>>> these patches atop it. These patches apply atop the > > >>>>>> "[SSSD] DEBUG macro refactoring v6" patches. > > >>>>> > > >>>>> These patches work for me, so ACK. > > >>>> > > >>>> Pushed to master. > > >>> > > >>> find_uid-tests failed with these patches on fedora20. > > >>> http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log > > >> > > >> > > >>> > > Do you have additional patches in this build? > > >> > > >> /builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm > > >> > > >> > > >> > > The git hash does not look like anything from master. > > >> > > > Yes, I was testing some patches. > > > > > >> I started a koji build > > >> http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with > > >> current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which > > >> is working fine. > > >> > > > I tried one more time with master and koji works fine. > > > Unfortunately, I am able to reproduce it locally in mock :-( > > > > > > > > > I've seen that happen intermittently as well in mock and koji. I > > suspect there may be a bug/race condition in sd_uid_get_sessions(), > > but I can't reproduce it consistently (and the DEBUG error doesn't > > seem to fire...) > > > > A quick look at the code suggests that we're probably missing a > > 'return EOK' after '*result = false', though. > > > > I'll run a couple tests and see if that fixes the issue... > > I found another issue related to journald support..if you compile sssd > with journald support and run it on the foreground (-i) then the debug > messages are still redirected to journal. That seems strange to me, but > also in line with what a comment in the code says: > > /* If we are not outputting logs to files, we should be sending them > * to journald. > * NOTE: on modern systems, this is where > * stdout/stderr will end up > > So I'm not entirely sure about the expected behaviour, but I would > prefer if there was still a way to see the debug messages directly on > the console. > > I hacked up a patch that also adds a third state where if sssd is running > on the foreground even with journald support, then just fprintf is used. > See the attachment.
Wouldn't it be simpler to just set debug_file = stderr ?
I tried to go this way, but it seems actually harder to me..
To summarize: with "sssd -f" debug_file should be a log file with "sssd" debug_file should be NULL, indicating journald with "sssd -i", debug_file should be stderr, indicating stderr.
The problem is, that we don't pass the "-i" flag down to the responders and back ends when forking them. So the alternative would be to add "-i" to sssd_be and all responders, pass the flag to the child processes and only set debug_file to stderr if '-i' is present. If you prefer this way, I can prepare a patch along these lines.
But then we also terminate the process which is running with "-i" if its stdin goes away, I guess it would be safe to keep this logic since stdin in the child processes is /dev/null already, I just wanted to mention there is a number of cases to test..
Any opinions? I'd like to fix this bug before tagging 1.12.0..
Maybe having an option --debug-to-stdout would be nicer for the child processes because it fits better to the existing SSSD_DEBUG_OPTS and will have no additional meaning as -i? For the monitor this option should be rejected and hidden from the help.
bye, Sumit
Perhaps..
to clarify, you're suggesting that --debug-to-stdout would be added to all processes (monitor and child), but only child processes in libexec would expose it and this option would be used when spawning the child processes?
yes, my idea was to add it to SSSD_DEBUG_OPTS which would make it immediately available to all processes. But having it for the monitor make no sense because '-D --debug-to-stdout' and '-f --debug-to-stdout' are config errors and '-i --debug-to-stdout' is a no-op.
bye, Sumit
Patches are attached. I found out the same issue also affected the CLI tools and tests, which is fixed with the second patch.
In order to test, you need to run configure --with-initscript=systemd --with-systemdunitdir=/usr/lib/systemd/system --with-syslog=journald
sssd -i -d <N> logs to stderr sssd -i -f -d <N> logs to files sssd -D -d <N> logs to journald sssd -D -f -d <N> logs to files
It's not possible to log to journald when running on the foreground or to stderr when running as deamon.
On 07/09/2014 11:43 AM, Jakub Hrozek wrote:
On Tue, Jul 08, 2014 at 09:53:50AM +0200, Sumit Bose wrote:
On Mon, Jul 07, 2014 at 10:20:21PM +0200, Jakub Hrozek wrote:
On Mon, Jul 07, 2014 at 03:57:28PM +0200, Sumit Bose wrote:
On Mon, Jul 07, 2014 at 03:21:54PM +0200, Jakub Hrozek wrote:
On Tue, Jun 03, 2014 at 09:53:13AM +0200, Jakub Hrozek wrote:
On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote: > On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote: >> On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote: >>> -----BEGIN PGP SIGNED MESSAGE----- >>> Hash: SHA1 >>> >>> On 02/19/2014 05:18 PM, Lukas Slebodnik wrote: >>>> On (19/02/14 22:03), Sumit Bose wrote: >>>>> On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote: >>>>>> On (19/02/14 12:37), Jakub Hrozek wrote: >>>>>>> On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek >>>>>>> wrote: >>>>>>>>> I realized I made a mistake in the rebasing above. I >>>>>>>>> fixed that and then needed to do another manual rebase of >>>>>>>>> these patches atop it. These patches apply atop the >>>>>>>>> "[SSSD] DEBUG macro refactoring v6" patches. >>>>>>>> >>>>>>>> These patches work for me, so ACK. >>>>>>> >>>>>>> Pushed to master. >>>>>> >>>>>> find_uid-tests failed with these patches on fedora20. >>>>>> http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log >>>>> >>>>> >>>>>> >>> Do you have additional patches in this build? >>>>> >>>>> /builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm >>>>> >>>>> >>>>> >>> The git hash does not look like anything from master. >>>>> >>>> Yes, I was testing some patches. >>>> >>>>> I started a koji build >>>>> http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with >>>>> current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which >>>>> is working fine. >>>>> >>>> I tried one more time with master and koji works fine. >>>> Unfortunately, I am able to reproduce it locally in mock :-( >>>> >>> >>> >>> I've seen that happen intermittently as well in mock and koji. I >>> suspect there may be a bug/race condition in sd_uid_get_sessions(), >>> but I can't reproduce it consistently (and the DEBUG error doesn't >>> seem to fire...) >>> >>> A quick look at the code suggests that we're probably missing a >>> 'return EOK' after '*result = false', though. >>> >>> I'll run a couple tests and see if that fixes the issue... >> >> I found another issue related to journald support..if you compile sssd >> with journald support and run it on the foreground (-i) then the debug >> messages are still redirected to journal. That seems strange to me, but >> also in line with what a comment in the code says: >> >> /* If we are not outputting logs to files, we should be sending them >> * to journald. >> * NOTE: on modern systems, this is where >> * stdout/stderr will end up >> >> So I'm not entirely sure about the expected behaviour, but I would >> prefer if there was still a way to see the debug messages directly on >> the console. >> >> I hacked up a patch that also adds a third state where if sssd is running >> on the foreground even with journald support, then just fprintf is used. >> See the attachment. > > Wouldn't it be simpler to just set debug_file = stderr ?
I tried to go this way, but it seems actually harder to me..
To summarize: with "sssd -f" debug_file should be a log file with "sssd" debug_file should be NULL, indicating journald with "sssd -i", debug_file should be stderr, indicating stderr.
The problem is, that we don't pass the "-i" flag down to the responders and back ends when forking them. So the alternative would be to add "-i" to sssd_be and all responders, pass the flag to the child processes and only set debug_file to stderr if '-i' is present. If you prefer this way, I can prepare a patch along these lines.
But then we also terminate the process which is running with "-i" if its stdin goes away, I guess it would be safe to keep this logic since stdin in the child processes is /dev/null already, I just wanted to mention there is a number of cases to test..
Any opinions? I'd like to fix this bug before tagging 1.12.0..
Maybe having an option --debug-to-stdout would be nicer for the child processes because it fits better to the existing SSSD_DEBUG_OPTS and will have no additional meaning as -i? For the monitor this option should be rejected and hidden from the help.
bye, Sumit
Perhaps..
to clarify, you're suggesting that --debug-to-stdout would be added to all processes (monitor and child), but only child processes in libexec would expose it and this option would be used when spawning the child processes?
yes, my idea was to add it to SSSD_DEBUG_OPTS which would make it immediately available to all processes. But having it for the monitor make no sense because '-D --debug-to-stdout' and '-f --debug-to-stdout' are config errors and '-i --debug-to-stdout' is a no-op.
bye, Sumit
Patches are attached. I found out the same issue also affected the CLI tools and tests, which is fixed with the second patch.
In order to test, you need to run configure --with-initscript=systemd --with-systemdunitdir=/usr/lib/systemd/system --with-syslog=journald
sssd -i -d <N> logs to stderr sssd -i -f -d <N> logs to files sssd -D -d <N> logs to journald sssd -D -f -d <N> logs to files
It's not possible to log to journald when running on the foreground or to stderr when running as deamon.
I think the behaviour with these patches makes sense and works as expected. Ack.
Michal
On Wed, Jul 09, 2014 at 03:14:31PM +0200, Michal Židek wrote:
On 07/09/2014 11:43 AM, Jakub Hrozek wrote:
On Tue, Jul 08, 2014 at 09:53:50AM +0200, Sumit Bose wrote:
On Mon, Jul 07, 2014 at 10:20:21PM +0200, Jakub Hrozek wrote:
On Mon, Jul 07, 2014 at 03:57:28PM +0200, Sumit Bose wrote:
On Mon, Jul 07, 2014 at 03:21:54PM +0200, Jakub Hrozek wrote:
On Tue, Jun 03, 2014 at 09:53:13AM +0200, Jakub Hrozek wrote: >On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote: >>On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote: >>>On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote: >>>>-----BEGIN PGP SIGNED MESSAGE----- >>>>Hash: SHA1 >>>> >>>>On 02/19/2014 05:18 PM, Lukas Slebodnik wrote: >>>>>On (19/02/14 22:03), Sumit Bose wrote: >>>>>>On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote: >>>>>>>On (19/02/14 12:37), Jakub Hrozek wrote: >>>>>>>>On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek >>>>>>>>wrote: >>>>>>>>>>I realized I made a mistake in the rebasing above. I >>>>>>>>>>fixed that and then needed to do another manual rebase of >>>>>>>>>>these patches atop it. These patches apply atop the >>>>>>>>>>"[SSSD] DEBUG macro refactoring v6" patches. >>>>>>>>> >>>>>>>>>These patches work for me, so ACK. >>>>>>>> >>>>>>>>Pushed to master. >>>>>>> >>>>>>>find_uid-tests failed with these patches on fedora20. >>>>>>>http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log >>>>>> >>>>>> >>>>>>> >>>>Do you have additional patches in this build? >>>>>> >>>>>>/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm >>>>>> >>>>>> >>>>>> >>>>The git hash does not look like anything from master. >>>>>> >>>>>Yes, I was testing some patches. >>>>> >>>>>>I started a koji build >>>>>>http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with >>>>>>current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which >>>>>>is working fine. >>>>>> >>>>>I tried one more time with master and koji works fine. >>>>>Unfortunately, I am able to reproduce it locally in mock :-( >>>>> >>>> >>>> >>>>I've seen that happen intermittently as well in mock and koji. I >>>>suspect there may be a bug/race condition in sd_uid_get_sessions(), >>>>but I can't reproduce it consistently (and the DEBUG error doesn't >>>>seem to fire...) >>>> >>>>A quick look at the code suggests that we're probably missing a >>>>'return EOK' after '*result = false', though. >>>> >>>>I'll run a couple tests and see if that fixes the issue... >>> >>>I found another issue related to journald support..if you compile sssd >>>with journald support and run it on the foreground (-i) then the debug >>>messages are still redirected to journal. That seems strange to me, but >>>also in line with what a comment in the code says: >>> >>> /* If we are not outputting logs to files, we should be sending them >>> * to journald. >>> * NOTE: on modern systems, this is where >>> * stdout/stderr will end up >>> >>>So I'm not entirely sure about the expected behaviour, but I would >>>prefer if there was still a way to see the debug messages directly on >>>the console. >>> >>>I hacked up a patch that also adds a third state where if sssd is running >>>on the foreground even with journald support, then just fprintf is used. >>>See the attachment. >> >>Wouldn't it be simpler to just set debug_file = stderr ? > >I tried to go this way, but it seems actually harder to me.. > >To summarize: >with "sssd -f" debug_file should be a log file >with "sssd" debug_file should be NULL, indicating journald >with "sssd -i", debug_file should be stderr, indicating stderr. > >The problem is, that we don't pass the "-i" flag down to the responders >and back ends when forking them. So the alternative would be to add "-i" >to sssd_be and all responders, pass the flag to the child processes and >only set debug_file to stderr if '-i' is present. If you prefer this way, >I can prepare a patch along these lines. > >But then we also terminate the process which is running with "-i" if its >stdin goes away, I guess it would be safe to keep this logic since stdin >in the child processes is /dev/null already, I just wanted to mention >there is a number of cases to test..
Any opinions? I'd like to fix this bug before tagging 1.12.0..
Maybe having an option --debug-to-stdout would be nicer for the child processes because it fits better to the existing SSSD_DEBUG_OPTS and will have no additional meaning as -i? For the monitor this option should be rejected and hidden from the help.
bye, Sumit
Perhaps..
to clarify, you're suggesting that --debug-to-stdout would be added to all processes (monitor and child), but only child processes in libexec would expose it and this option would be used when spawning the child processes?
yes, my idea was to add it to SSSD_DEBUG_OPTS which would make it immediately available to all processes. But having it for the monitor make no sense because '-D --debug-to-stdout' and '-f --debug-to-stdout' are config errors and '-i --debug-to-stdout' is a no-op.
bye, Sumit
Patches are attached. I found out the same issue also affected the CLI tools and tests, which is fixed with the second patch.
In order to test, you need to run configure --with-initscript=systemd --with-systemdunitdir=/usr/lib/systemd/system --with-syslog=journald
sssd -i -d <N> logs to stderr sssd -i -f -d <N> logs to files sssd -D -d <N> logs to journald sssd -D -f -d <N> logs to files
It's not possible to log to journald when running on the foreground or to stderr when running as deamon.
I think the behaviour with these patches makes sense and works as expected. Ack.
Thanks for the review, pushed to master: 6b57784f0f175275fd900eca21c77415e3a5ea52 9a990aa9f7e8c105e0cfeea8d8cbdc776c2d5d7a
Michal
btw feel free to open the ticket you mentioned in office today about better (more easier) configuration of the debugging..
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/24/2014 05:54 AM, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote:
On (19/02/14 12:37), Jakub Hrozek wrote:
On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek wrote: >> I realized I made a mistake in the rebasing above. I >> fixed that and then needed to do another manual >> rebase of these patches atop it. These patches apply >> atop the "[SSSD] DEBUG macro refactoring v6" >> patches. > > These patches work for me, so ACK.
Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Sorry, I completely forgot about the *real* foreground case there. I was thinking purely of how SSSD works when launched by systemd. In that case, if it runs in the foreground, the output is trapped.
I don't love this particular implementation (it's clearly a hack), but I also can't think of another approach (aside from adding a new command-line flag to enable sending to journal instead of files and leaving the default as stdout/stderr).
On (24/02/14 08:40), Stephen Gallagher wrote:
On 02/24/2014 05:54 AM, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote:
On (19/02/14 12:37), Jakub Hrozek wrote: > On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub Hrozek > wrote: >>> I realized I made a mistake in the rebasing above. I >>> fixed that and then needed to do another manual >>> rebase of these patches atop it. These patches apply >>> atop the "[SSSD] DEBUG macro refactoring v6" >>> patches. >> >> These patches work for me, so ACK. > > Pushed to master.
find_uid-tests failed with these patches on fedora20. http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with current master (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Sorry, I completely forgot about the *real* foreground case there. I was thinking purely of how SSSD works when launched by systemd. In that case, if it runs in the foreground, the output is trapped.
I don't love this particular implementation (it's clearly a hack), but I also can't think of another approach (aside from adding a new command-line flag to enable sending to journal instead of files and leaving the default as stdout/stderr).
Could we try to fix this issue?
I had a related problem with logging in container. The daemon journald isn't running in container and sd_journal_sendv ignores this situation.
k = sendmsg(fd, &mh, MSG_NOSIGNAL); if (k >= 0) return 0;
/* Fail silently if the journal is not available */ if (errno == ENOENT) return 0; ^^^^^^^^^^^^^^^^^^^ The debug message was not logged in systemd and sd_journal_sendv did not fail and therefore we could not fall back to the standard output.
I don't want to argue with systemd developers about this behaviour, but I would like to see debug messages in containers. My current workaround is to log into files, but it has a lot of disadvantages for me.
LS
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/06/2014 09:41 AM, Lukas Slebodnik wrote:
On (24/02/14 08:40), Stephen Gallagher wrote:
On 02/24/2014 05:54 AM, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote:
On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas Slebodnik wrote: > On (19/02/14 12:37), Jakub Hrozek wrote: >> On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub >> Hrozek wrote: >>>> I realized I made a mistake in the rebasing >>>> above. I fixed that and then needed to do another >>>> manual rebase of these patches atop it. These >>>> patches apply atop the "[SSSD] DEBUG macro >>>> refactoring v6" patches. >>> >>> These patches work for me, so ACK. >> >> Pushed to master. > > find_uid-tests failed with these patches on fedora20. > http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
>
>
>
Do you have additional patches in this build?
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
The git hash does not look like anything from master.
Yes, I was testing some patches.
I started a koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081
with current master
(4cde267bec52ae1723a125d19439a5c75b47ebb7) which is working fine.
I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Sorry, I completely forgot about the *real* foreground case there. I was thinking purely of how SSSD works when launched by systemd. In that case, if it runs in the foreground, the output is trapped.
I don't love this particular implementation (it's clearly a hack), but I also can't think of another approach (aside from adding a new command-line flag to enable sending to journal instead of files and leaving the default as stdout/stderr).
Could we try to fix this issue?
I had a related problem with logging in container. The daemon journald isn't running in container and sd_journal_sendv ignores this situation.
k = sendmsg(fd, &mh, MSG_NOSIGNAL); if (k >= 0) return 0;
/* Fail silently if the journal is not available */ if (errno == ENOENT) return 0; ^^^^^^^^^^^^^^^^^^^ The debug message was not logged in systemd and sd_journal_sendv did not fail and therefore we could not fall back to the standard output.
I don't want to argue with systemd developers about this behaviour, but I would like to see debug messages in containers. My current workaround is to log into files, but it has a lot of disadvantages for me.
Failing silently REALLY doesn't seem like correct behavior here. I did some digging and the reasoning behind it is that otherwise, it breaks a lot of software when running in mock/chroot.
- From talking to journald devs, the recommended approach for us should be to link against libsystemd-daemon and call sd_booted() during process start. This will return 0 or 1, depending on whether systemd was used to boot the system. If it returns 0, we should defer to the traditional logging path rather than using the journal.
On (07/05/14 16:27), Stephen Gallagher wrote:
On 05/06/2014 09:41 AM, Lukas Slebodnik wrote:
On (24/02/14 08:40), Stephen Gallagher wrote:
On 02/24/2014 05:54 AM, Jakub Hrozek wrote:
On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
On (19/02/14 22:03), Sumit Bose wrote: > On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas > Slebodnik wrote: >> On (19/02/14 12:37), Jakub Hrozek wrote: >>> On Wed, Feb 19, 2014 at 12:02:23PM +0100, Jakub >>> Hrozek wrote: >>>>> I realized I made a mistake in the rebasing >>>>> above. I fixed that and then needed to do another >>>>> manual rebase of these patches atop it. These >>>>> patches apply atop the "[SSSD] DEBUG macro >>>>> refactoring v6" patches. >>>> >>>> These patches work for me, so ACK. >>> >>> Pushed to master. >> >> find_uid-tests failed with these patches on fedora20. >> http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log > > >>
>>
>>
Do you have additional patches in this build?
> > /builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm > > >
>
>
The git hash does not look like anything from master.
> Yes, I was testing some patches.
> I started a koji build > http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 > >
with current master
> (4cde267bec52ae1723a125d19439a5c75b47ebb7) which is > working fine. > I tried one more time with master and koji works fine. Unfortunately, I am able to reproduce it locally in mock :-(
I've seen that happen intermittently as well in mock and koji. I suspect there may be a bug/race condition in sd_uid_get_sessions(), but I can't reproduce it consistently (and the DEBUG error doesn't seem to fire...)
A quick look at the code suggests that we're probably missing a 'return EOK' after '*result = false', though.
I'll run a couple tests and see if that fixes the issue...
I found another issue related to journald support..if you compile sssd with journald support and run it on the foreground (-i) then the debug messages are still redirected to journal. That seems strange to me, but also in line with what a comment in the code says:
/* If we are not outputting logs to files, we should be sending them * to journald. * NOTE: on modern systems, this is where * stdout/stderr will end up
So I'm not entirely sure about the expected behaviour, but I would prefer if there was still a way to see the debug messages directly on the console.
I hacked up a patch that also adds a third state where if sssd is running on the foreground even with journald support, then just fprintf is used. See the attachment.
Sorry, I completely forgot about the *real* foreground case there. I was thinking purely of how SSSD works when launched by systemd. In that case, if it runs in the foreground, the output is trapped.
I don't love this particular implementation (it's clearly a hack), but I also can't think of another approach (aside from adding a new command-line flag to enable sending to journal instead of files and leaving the default as stdout/stderr).
Could we try to fix this issue?
I had a related problem with logging in container. The daemon journald isn't running in container and sd_journal_sendv ignores this situation.
k = sendmsg(fd, &mh, MSG_NOSIGNAL); if (k >= 0) return 0;
/* Fail silently if the journal is not available */ if (errno == ENOENT) return 0; ^^^^^^^^^^^^^^^^^^^ The debug message was not logged in systemd and sd_journal_sendv did not fail and therefore we could not fall back to the standard output.
I don't want to argue with systemd developers about this behaviour, but I would like to see debug messages in containers. My current workaround is to log into files, but it has a lot of disadvantages for me.
Failing silently REALLY doesn't seem like correct behavior here. I did some digging and the reasoning behind it is that otherwise, it breaks a lot of software when running in mock/chroot.
containers are almost like a chroot and it breaks logging in containers. The problem is solved in mock, new problem is introduced in containers It is double edged sword. The biggest problem is that it is not documented (man sd_journal_send) https://bugzilla.redhat.com/show_bug.cgi?id=1096067
From talking to journald devs, the recommended approach for us should be to link against libsystemd-daemon and call sd_booted() during process start. This will return 0 or 1, depending on whether systemd was used to boot the system. If it returns 0, we should defer to the traditional logging path rather than using the journal.
calling function sd_booted is not solution. It can fix one problem, but it will introduce another problem.
With containers, you can mount bind directory from a host machine to a container. This would solve problem mith missing log messages, but your solution will break this workaround. The functions from library libsystemd-journal.so would be able to send messages to the journald, but systemd is not booted (sd_booted return negative value) and we will send debug messages to the stderr
We could test if socket "/var/run/dbus/system_bus_socket" exists but it is relying on undocumented location of socket.
LS
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Fri 09 May 2014 04:12:42 AM EDT, Lukas Slebodnik wrote:
On (07/05/14 16:27), Stephen Gallagher wrote:
On 05/06/2014 09:41 AM, Lukas Slebodnik wrote:
Could we try to fix this issue?
I had a related problem with logging in container. The daemon journald isn't running in container and sd_journal_sendv ignores this situation.
k = sendmsg(fd, &mh, MSG_NOSIGNAL); if (k >= 0) return 0;
/* Fail silently if the journal is not available */ if (errno == ENOENT) return 0; ^^^^^^^^^^^^^^^^^^^ The debug message was not logged in systemd and sd_journal_sendv did not fail and therefore we could not fall back to the standard output.
I don't want to argue with systemd developers about this behaviour, but I would like to see debug messages in containers. My current workaround is to log into files, but it has a lot of disadvantages for me.
Failing silently REALLY doesn't seem like correct behavior here. I did some digging and the reasoning behind it is that otherwise, it breaks a lot of software when running in mock/chroot.
containers are almost like a chroot and it breaks logging in containers. The problem is solved in mock, new problem is introduced in containers It is double edged sword. The biggest problem is that it is not documented (man sd_journal_send) https://bugzilla.redhat.com/show_bug.cgi?id=1096067
From talking to journald devs, the recommended approach for us should be to link against libsystemd-daemon and call sd_booted() during process start. This will return 0 or 1, depending on whether systemd was used to boot the system. If it returns 0, we should defer to the traditional logging path rather than using the journal.
calling function sd_booted is not solution. It can fix one problem, but it will introduce another problem.
With containers, you can mount bind directory from a host machine to a container. This would solve problem mith missing log messages, but your solution will break this workaround. The functions from library libsystemd-journal.so would be able to send messages to the journald, but systemd is not booted (sd_booted return negative value) and we will send debug messages to the stderr
We could test if socket "/var/run/dbus/system_bus_socket" exists but it is relying on undocumented location of socket.
Wouldn't it make more sense to bind in /run/systemd/journal/socket? That's supposedly a well-known location and that's really all that should be needed to dump the logs out to the parent journal, I think.
On (09/05/14 08:48), Stephen Gallagher wrote:
On Fri 09 May 2014 04:12:42 AM EDT, Lukas Slebodnik wrote:
On (07/05/14 16:27), Stephen Gallagher wrote:
On 05/06/2014 09:41 AM, Lukas Slebodnik wrote:
Could we try to fix this issue?
I had a related problem with logging in container. The daemon journald isn't running in container and sd_journal_sendv ignores this situation.
k = sendmsg(fd, &mh, MSG_NOSIGNAL); if (k >= 0) return 0;
/* Fail silently if the journal is not available */ if (errno == ENOENT) return 0; ^^^^^^^^^^^^^^^^^^^ The debug message was not logged in systemd and sd_journal_sendv did not fail and therefore we could not fall back to the standard output.
I don't want to argue with systemd developers about this behaviour, but I would like to see debug messages in containers. My current workaround is to log into files, but it has a lot of disadvantages for me.
Failing silently REALLY doesn't seem like correct behavior here. I did some digging and the reasoning behind it is that otherwise, it breaks a lot of software when running in mock/chroot.
containers are almost like a chroot and it breaks logging in containers. The problem is solved in mock, new problem is introduced in containers It is double edged sword. The biggest problem is that it is not documented (man sd_journal_send) https://bugzilla.redhat.com/show_bug.cgi?id=1096067
From talking to journald devs, the recommended approach for us should be to link against libsystemd-daemon and call sd_booted() during process start. This will return 0 or 1, depending on whether systemd was used to boot the system. If it returns 0, we should defer to the traditional logging path rather than using the journal.
calling function sd_booted is not solution. It can fix one problem, but it will introduce another problem.
With containers, you can mount bind directory from a host machine to a container. This would solve problem mith missing log messages, but your solution will break this workaround. The functions from library libsystemd-journal.so would be able to send messages to the journald, but systemd is not booted (sd_booted return negative value) and we will send debug messages to the stderr
We could test if socket "/var/run/dbus/system_bus_socket" exists but it is relying on undocumented location of socket.
Wouldn't it make more sense to bind in /run/systemd/journal/socket?
copy&paste problem :-) (yet another problem in IT)
That's supposedly a well-known location and that's really all that should be needed to dump the logs out to the parent journal, I think.
It is not guaranted that this socket will not be moved to another place. systemd has rapid development.
BTW. manual page was updated http://cgit.freedesktop.org/systemd/systemd/commit/?id=bdf9fc1a but they did not mention full path to the socket. "(the socket is not present)"
LS
On Fri, 2014-05-09 at 15:17 +0200, Lukas Slebodnik wrote:
That's supposedly a well-known location and that's really all that should be needed to dump the logs out to the parent journal, I think.
It is not guaranted that this socket will not be moved to another place. systemd has rapid development.
I think this would be a good question to ask the systemd developers about.
On Fri, Dec 20, 2013 at 05:05:56PM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
All of these patches require Nikolai's "DEBUG Macro Refactoring v3" patches to be applied first.
Patch 0001: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
Patch 0002: Enable sending structured debug logs to journald
We are now able to send structured debug logs to journald, tagged with the code file, line number and domain that the log pertains to. To enable this functionality, SSSD must be configured at build-time with
- --with-syslog=journald and must be launched without -f/--debug-to-files
This behavior is nearly identical to how SSSD will function today on a systemd-based system if --debug-to-files is disabled, since it will redirect stdout and stderr into journald. This patch merely enhances the situation to send structured logs instead of simple string messages.
I have to admit that I haven't look into much detail about what journald is capable of. So there is a fair chance that the following can already easily solved within journald.
On typical scenario when a user hits an issue is that we ask to increase the debug level and collect the logs which are generated when reproducing the issue. With structured logs it is easy to get the specific logs out of journald but I wonder if we want to write everything into journald in the first place and fill its storage with one-time debug data?
I was thinking if we might want to introduce a new option which gives some upper value for log levels which should go to journald. Then every message up to this level will always go to journald. If the debug_level is higher then this value debug files with all messages are created as well. This way we still have all error message in the journal but keep it free of debug messages.
An alternative, if journald supports it, would be to provide special config options to instruct journald to store messages with a high debug_level to a short term storage where they can easily be deleted without removing other messages as well.
bye, Sumit
Patch 0003: BUILD: Build with journald support by default on Fedora
The journal provided by systemd gives us structured logging capabilities that we should be taking advantage of.
Note: this patch explicitly does not change the systemd unit file for SSSD. Right now, an administrator will need to manually remove the '-f' from ExecStart in the unit file to send debug logs to journald. I suspect we'll want to discuss this before we make it the default. This patch DOES change the default for sss_log messages to use sd_journal_send() instead of straight log() for those messages that we traditionally sent to the syslog (such as login events). This is code that has been in place for some time now, but has not been the default because we hadn't build with --with-syslog=journald. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlK0v0QACgkQeiVVYja6o6NwxgCfWnU4IbYyJPXxPe83zWjjsCHt cG8AnitRgM9CAUjHM2ofYg3TgcUxCyCC =Fara -----END PGP SIGNATURE-----
On Tue, Feb 04, 2014 at 11:33:19AM +0100, Sumit Bose wrote:
On Fri, Dec 20, 2013 at 05:05:56PM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
All of these patches require Nikolai's "DEBUG Macro Refactoring v3" patches to be applied first.
Patch 0001: Allow debug_fn to process __FILE__ and __LINE__
In preparation for enabling journald support for the DEBUG logs, we will need to be able to pass in certain additional arguments that will be required, specifically the code file and line number.
We will be able to optionally enable this in the file-based logs as well if we so choose, but for right now we will avoid breaking the log format on disk.
Patch 0002: Enable sending structured debug logs to journald
We are now able to send structured debug logs to journald, tagged with the code file, line number and domain that the log pertains to. To enable this functionality, SSSD must be configured at build-time with
- --with-syslog=journald and must be launched without -f/--debug-to-files
This behavior is nearly identical to how SSSD will function today on a systemd-based system if --debug-to-files is disabled, since it will redirect stdout and stderr into journald. This patch merely enhances the situation to send structured logs instead of simple string messages.
I have to admit that I haven't look into much detail about what journald is capable of. So there is a fair chance that the following can already easily solved within journald.
On typical scenario when a user hits an issue is that we ask to increase the debug level and collect the logs which are generated when reproducing the issue. With structured logs it is easy to get the specific logs out of journald but I wonder if we want to write everything into journald in the first place and fill its storage with one-time debug data?
I was thinking if we might want to introduce a new option which gives some upper value for log levels which should go to journald. Then every message up to this level will always go to journald. If the debug_level is higher then this value debug files with all messages are created as well. This way we still have all error message in the journal but keep it free of debug messages.
An alternative, if journald supports it, would be to provide special config options to instruct journald to store messages with a high debug_level to a short term storage where they can easily be deleted without removing other messages as well.
bye, Sumit
I was playing with journald some more this morning and I haven't found a way to solve your concerns easily. As far as I could see it is only possible to redirect messages to persistent or volatile storage globally, not per-unit. But maybe I missed something.
Luckily, the journald has an upper cap on the size the journal takes (SystemMaxUse/SystemKeepFree).
So far the easiest way I found was disabling the logging with sss_debuglevel after the debugging is done. For now, it's not a big deal as the journal is not enabled by default, but we should think about this issue again if we wanted to make journald the default.
sssd-devel@lists.fedorahosted.org