[Cluster-devel] logsys in fenced

[Fabio M. Di Nitto <fdinitto@redhat.com>]

On Thu, 26 Jun 2008, David Teigland wrote:

> On Thu, Jun 26, 2008 at 05:48:56AM +0200, Fabio M. Di Nitto wrote:
>> On Wed, 25 Jun 2008, David Teigland wrote:
>>
>>> Attached two patches:
>>> fenced-revert.patch reverts the current logsys changes to fenced.
>>
>> If you revert, please do it by meaning of git-revert.
>
> That was a series of git-revert -n which the man page suggested:
>
> "This is useful when reverting more than one commits' effect to
> your working tree in a row."

I missed the git header and I thought you were going to revert by meaning 
of one single manual patch.

>
>> ^^ You can't use open_ccs within get_logsys_config_data. open_ccs uses
>> log_printf to log. If it's waiting for ccs, but logging is not available
>> yet, you end up with no information of what is going on.
>>
>> In the original code, we try once to access the config data. If it works
>> good, otherwise we config logsys manually enough to log what is happening
>> and we try later. You are missing this fallback mechanism.
>
> I think I'll just avoid all the chicken/egg complication by not logging in
> in open_ccs.

So if you can't connect to ccs/objdb, we will have another daemon waiting 
for something and users will have no idea what.

>  I expect it should be nearly impossible for ccs_connect() to
> fail now with the new ccs.

Based on what assumption?

Anyway this is another pandora's box to open and should be handled in 
another thread. We have X different half baked solutions to connected to 
libccs and cman. None of them do it right. There is space for a little 
common library that handles it properly and it's not the first time that 
this issue comes up so maybe it's time to address it.

>  (Previously, ccs_connect would always fail
> leaving people wondering why nothing was happening, which is why we added
> the logging.  One of the goals of the new ccs is that reading the config
> should basically never fail.)

eh? so if cman/objdb are not available then what?

>
>
>> @@ -74,14 +75,17 @@ extern void daemon_dump_save(void);
>>  #define log_debug(fmt, args...) \
>>  do { \
>>  	snprintf(daemon_debug_buf, 255, "%ld " fmt "\n", time(NULL),
>>  	##args); \
>> -	if (daemon_debug_opt) fprintf(stderr, "%s", daemon_debug_buf); \
>>  	daemon_dump_save(); \
>> +	if (daemon_debug_opt) \
>> +		fprintf(stderr, "%s", daemon_debug_buf); \
>> +	else if (daemon_debug_logsys) \
>> +		log_printf(LOG_DEBUG, "%s", daemon_debug_buf); \
>>  } while (0)
>>
>> Can those 2 go in parallel?
>>
>> +       if (daemon_debug_opt) \
>> +               fprintf(stderr, "%s", daemon_debug_buf); \
>> +       if (daemon_debug_logsys) \
>> +               log_printf(LOG_DEBUG, "%s", daemon_debug_buf); \
>>
>> so that users can decide which one they want without worrying about
>> preferences?
>
> daemon_debug_opt and daemon_debug_logsys are mutually exclusive by
> definition; you don't want or need logsys when you're running with -D.

Who says? i don't use -D, but i set debug=on and i get the exact same 
results. Output to stderr included if i don't fork.

>
>
>> diff --git a/fence/fenced/logging.c b/fence/fenced/logging.c
>> new file mode 100644
>> index 0000000..38abc21
>> --- /dev/null
>> +++ b/fence/fenced/logging.c
>> @@ -0,0 +1,109 @@
>> +#include "fd.h"
>> +
>> +#define LEVEL_PATH
>> "/cluster/logging/logger_subsys[@subsys=\"FENCED\"]/@syslog_level"
>> +#define DEBUG_PATH
>> "/cluster/logging/logger_subsys[@subsys=\"FENCED\"]/@debug"
>> +
>> +static int get_logsys_config_data(void)
>>
>> Most of the configuration logic is wrong and you don't check for a bunch
>> of return codes that could at least inform you or the users of the status
>> of the configuration.
>
> Yeah, not understanding logsys, I couldn't make sense of the logic that
> was there, it was "non-obvious", so I just tried to copy it.  I obviously
> have a lot of logsys study to do, hopefully suggesting some
> simplifications along the way...

The configuration is dead easy. It was there. You could have just replaced 
my strcmp with your read_ccsyes/no thingy and it would have work without 
you having to spend this incredible amount of time on logsys.

>
>
>> Debug needs to be checked before log_level. In logsys debug=on is
>> equivalent of setting syslog_level=debug. The original code I proposed
>> does:
>>
>> debug from the envvar (that i missed in my original patch) > debug from
>> cmdline > subsystem config debug option > global config debug option.
>>
>> This allows you a great deal of flexibility to specify: I want all debug
>> on except for this or that subsystem, or i want debugging off, except
>> for.. etc.
>>
>> Then you set log_level according to that.
>
> that sounds like a great deal of complexity for something that shouldn't
> deserve it.

It's not complex at all. It saves you a log of ccs interactions if you set 
debug from the command line and it allows you to do fine tuned debugging 
per subsystem.

Fabio

--
I'm going to make him an offer he can't refuse.