[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Cluster-devel] logsys in fenced

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

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

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.


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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]