From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fabio M. Di Nitto Date: Fri, 27 Jun 2008 05:45:20 +0200 (CEST) Subject: [Cluster-devel] logsys in fenced In-Reply-To: <20080626140518.GA21081@redhat.com> References: <20080625212155.GE18958@redhat.com> <20080626140518.GA21081@redhat.com> Message-ID: List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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.