Hi Frank,
On Fri, 2021-11-05 at 22:42 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> I speculate this one will be handy for even larger debuginfod
> installations, where one must pool multiple servers together, but
> without duplicating indexing/grooming efforts.
>
> (The diff is larger than it needs to be, by virtue of nesting previous
> code into a conditional block.)
Yeah, git diff/show -u -w is much more readable.
> commit 8f2623a9ff8ac311c0ec829d982863a8dda4154a
> Author: Frank Ch. Eigler <f...@redhat.com>
> Date: Fri Nov 5 22:26:35 2021 -0400
>
> PR28430: debuginfod: support --passive mode
>
> Add support for a limited mode for debuginfod that uses a pure
> read-only sqlite index. This mode is useful for load spreading based
> on naively shared or replicated databases.
>
> Signed-off-by: Frank Ch. Eigler <f...@redhat.com>
>
> diff --git a/NEWS b/NEWS
> index b812b7438967..709c4cd9e7f5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,7 @@ debuginfod: Supply extra HTTP response headers, describing
> archive/file
> Add -r option to use -I/-X regexes for grooming stale files.
> Protect against wasted CPU from duplicate concurrent requests.
> Limit the duration of groom ops roughly to rescan (-t) times.
> + Add --passive mode for serving from read-only database.
> Several other performance improvements & prometheus metrics.
Ack.
> Version 0.185
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index 15b2ba40fa0f..f06d3ee3ecf4 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,11 @@
> +2021-11-05 Frank Ch. Eigler <f...@redhat.com>
> +
> + PR28430
> + * debuginfod.cxx (parse_opt): Add "--passive" flag. Complain
> + about inconsistent flags.
> + (main): In passive mode, suppress scan/groom/traverse threads and
> + other read-write database ops.
> +
> 2021-11-04 Frank Ch. Eigler <f...@redhat.com>
>
> PR28514
> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> index 45981d8d4279..28217e6d92d4 100644
> --- a/debuginfod/debuginfod.cxx
> +++ b/debuginfod/debuginfod.cxx
> @@ -376,6 +376,8 @@ static const struct argp_option options[] =
> prefetch cache.", 0},
> #define ARGP_KEY_FORWARDED_TTL_LIMIT 0x1007
> {"forwarded-ttl-limit", ARGP_KEY_FORWARDED_TTL_LIMIT, "NUM", 0, "Limit of
> X-Forwarded-For hops, default 8.", 0},
> +#define ARGP_KEY_PASSIVE 0x1008
^^^^ spurious whitespace
> + { "passive", ARGP_KEY_PASSIVE, NULL, 0, "Do not scan or groom, read-only
> database.", 0 },
> { NULL, 0, NULL, 0, NULL, 0 },
> };
>
> @@ -425,6 +427,8 @@ static long fdcache_prefetch_mbs;
> static long fdcache_prefetch_fds;
> static unsigned forwarded_ttl_limit = 8;
> static string tmpdir;
> +static bool passive_p = false;
> +
Spurious whitespace.
> static void set_metric(const string& key, double value);
> // static void inc_metric(const string& key);
> @@ -524,36 +528,56 @@ parse_opt (int key, char *arg,
> }
> break;
> case 'L':
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-L option inconsistent with passive
> mode");
> traverse_logical = true;
> break;
> - case 'D': extra_ddl.push_back(string(arg)); break;
> + case 'D':
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-D option inconsistent with passive
> mode");
> + extra_ddl.push_back(string(arg));
> + break;
> case 't':
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-t option inconsistent with passive
> mode");
> rescan_s = (unsigned) atoi(arg);
> break;
> case 'g':
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-g option inconsistent with passive
> mode");
> groom_s = (unsigned) atoi(arg);
> break;
> case 'G':
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-G option inconsistent with passive
> mode");
> maxigroom = true;
> break;
> case 'c':
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-c option inconsistent with passive
> mode");
> concurrency = (unsigned) atoi(arg);
> if (concurrency < 1) concurrency = 1;
> break;
> case 'I':
> // NB: no problem with unconditional free here - an earlier failed
> regcomp would exit program
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-I option inconsistent with passive
> mode");
> regfree (&file_include_regex);
> rc = regcomp (&file_include_regex, arg, REG_EXTENDED|REG_NOSUB);
> if (rc != 0)
> argp_failure(state, 1, EINVAL, "regular expression");
> break;
> case 'X':
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-X option inconsistent with passive
> mode");
> regfree (&file_exclude_regex);
> rc = regcomp (&file_exclude_regex, arg, REG_EXTENDED|REG_NOSUB);
> if (rc != 0)
> argp_failure(state, 1, EINVAL, "regular expression");
> break;
> case 'r':
> + if (passive_p)
> + argp_failure(state, 1, EINVAL, "-r option inconsistent with passive
> mode");
> regex_groom = true;
> break;
> case ARGP_KEY_FDCACHE_FDS:
Nice guarding of options to help the user.
> @@ -586,6 +610,15 @@ parse_opt (int key, char *arg,
> if ( fdcache_prefetch_mbs < 0)
> argp_failure(state, 1, EINVAL, "fdcache prefetch mbs");
> break;
> + case ARGP_KEY_PASSIVE:
> + passive_p = true;
> + if (source_paths.size() > 0 ||
> + maxigroom ||
> + extra_ddl.size() > 0 ||
> + traverse_logical)
> + // other conflicting options tricky to check
> + argp_failure(state, 1, EINVAL, "inconsistent options with passive
> mode");
> + break;
This would be a bit more readable written as:
+ if (source_paths.size() > 0
+ || maxigroom
+ || extra_ddl.size() > 0
+ || traverse_logical)
> // case 'h': argp_state_help (state, stderr,
> ARGP_HELP_LONG|ARGP_HELP_EXIT_OK);
> default: return ARGP_ERR_UNKNOWN;
> }
> @@ -3732,22 +3765,25 @@ main (int argc, char *argv[])
> (void) signal (SIGUSR2, sigusr2_handler); // end-user
>
> /* Get database ready. */
> - rc = sqlite3_open_v2 (db_path.c_str(), &db, (SQLITE_OPEN_READWRITE
> - |SQLITE_OPEN_URI
> - |SQLITE_OPEN_PRIVATECACHE
> - |SQLITE_OPEN_CREATE
> - |SQLITE_OPEN_FULLMUTEX), /*
> thread-safe */
> - NULL);
> - if (rc == SQLITE_CORRUPT)
> - {
> - (void) unlink (db_path.c_str());
> - error (EXIT_FAILURE, 0,
> - "cannot open %s, deleted database: %s", db_path.c_str(),
> sqlite3_errmsg(db));
> - }
> - else if (rc)
> - {
> - error (EXIT_FAILURE, 0,
> - "cannot open %s, consider deleting database: %s",
> db_path.c_str(), sqlite3_errmsg(db));
> + if (! passive_p)
> + {
> + rc = sqlite3_open_v2 (db_path.c_str(), &db, (SQLITE_OPEN_READWRITE
> + |SQLITE_OPEN_URI
> + |SQLITE_OPEN_PRIVATECACHE
> + |SQLITE_OPEN_CREATE
> + |SQLITE_OPEN_FULLMUTEX),
> /* thread-safe */
> + NULL);
> + if (rc == SQLITE_CORRUPT)
> + {
> + (void) unlink (db_path.c_str());
> + error (EXIT_FAILURE, 0,
> + "cannot open %s, deleted database: %s", db_path.c_str(),
> sqlite3_errmsg(db));
> + }
> + else if (rc)
> + {
> + error (EXIT_FAILURE, 0,
> + "cannot open %s, consider deleting database: %s",
> db_path.c_str(), sqlite3_errmsg(db));
> + }
> }
>
> // open the readonly query variant
> @@ -3765,8 +3801,10 @@ main (int argc, char *argv[])
> }
>
>
> - obatched(clog) << "opened database " << db_path << endl;
> + obatched(clog) << "opened database " << db_path
> + << (db?" rw":"") << (dbq?" ro":"") << endl;
> obatched(clog) << "sqlite version " << sqlite3_version << endl;
> + obatched(clog) << "service mode " << (passive_p ? "passive":"active") <<
> endl;
>
> // add special string-prefix-similarity function used in rpm sref/sdef
> resolution
> rc = sqlite3_create_function(dbq, "sharedprefix", 2, SQLITE_UTF8, NULL,
> @@ -3775,13 +3813,16 @@ main (int argc, char *argv[])
> error (EXIT_FAILURE, 0,
> "cannot create sharedprefix function: %s", sqlite3_errmsg(dbq));
>
> - if (verbose > 3)
> - obatched(clog) << "ddl: " << DEBUGINFOD_SQLITE_DDL << endl;
> - rc = sqlite3_exec (db, DEBUGINFOD_SQLITE_DDL, NULL, NULL, NULL);
> - if (rc != SQLITE_OK)
> + if (! passive_p)
> {
> - error (EXIT_FAILURE, 0,
> - "cannot run database schema ddl: %s", sqlite3_errmsg(db));
> + if (verbose > 3)
> + obatched(clog) << "ddl: " << DEBUGINFOD_SQLITE_DDL << endl;
> + rc = sqlite3_exec (db, DEBUGINFOD_SQLITE_DDL, NULL, NULL, NULL);
> + if (rc != SQLITE_OK)
> + {
> + error (EXIT_FAILURE, 0,
> + "cannot run database schema ddl: %s", sqlite3_errmsg(db));
> + }
> }
>
> // Start httpd server threads. Separate pool for IPv4 and IPv6, in
> @@ -3845,27 +3886,31 @@ main (int argc, char *argv[])
> }
>
> // run extra -D sql if given
> - for (auto&& i: extra_ddl)
> - {
> - if (verbose > 1)
> - obatched(clog) << "extra ddl:\n" << i << endl;
> - rc = sqlite3_exec (db, i.c_str(), NULL, NULL, NULL);
> - if (rc != SQLITE_OK && rc != SQLITE_DONE && rc != SQLITE_ROW)
> - error (0, 0,
> - "warning: cannot run database extra ddl %s: %s", i.c_str(),
> sqlite3_errmsg(db));
> - }
> -
> - if (maxigroom)
> - obatched(clog) << "maxigroomed database" << endl;
> + if (! passive_p)
> + for (auto&& i: extra_ddl)
> + {
> + if (verbose > 1)
> + obatched(clog) << "extra ddl:\n" << i << endl;
> + rc = sqlite3_exec (db, i.c_str(), NULL, NULL, NULL);
> + if (rc != SQLITE_OK && rc != SQLITE_DONE && rc != SQLITE_ROW)
> + error (0, 0,
> + "warning: cannot run database extra ddl %s: %s", i.c_str(),
> sqlite3_errmsg(db));
> +
> + if (maxigroom)
> + obatched(clog) << "maxigroomed database" << endl;
> + }
>
> - obatched(clog) << "search concurrency " << concurrency << endl;
> - obatched(clog) << "rescan time " << rescan_s << endl;
> + if (! passive_p)
> + obatched(clog) << "search concurrency " << concurrency << endl;
> + if (! passive_p)
> + obatched(clog) << "rescan time " << rescan_s << endl;
> obatched(clog) << "fdcache fds " << fdcache_fds << endl;
> obatched(clog) << "fdcache mbs " << fdcache_mbs << endl;
> obatched(clog) << "fdcache prefetch " << fdcache_prefetch << endl;
> obatched(clog) << "fdcache tmpdir " << tmpdir << endl;
> obatched(clog) << "fdcache tmpdir min% " << fdcache_mintmp << endl;
> - obatched(clog) << "groom time " << groom_s << endl;
> + if (! passive_p)
> + obatched(clog) << "groom time " << groom_s << endl;
> obatched(clog) << "prefetch fds " << fdcache_prefetch_fds << endl;
> obatched(clog) << "prefetch mbs " << fdcache_prefetch_mbs << endl;
> obatched(clog) << "forwarded ttl limit " << forwarded_ttl_limit << endl;
> @@ -3873,7 +3918,7 @@ main (int argc, char *argv[])
> if (scan_archives.size()>0)
> {
> obatched ob(clog);
> - auto& o = ob << "scanning archive types ";
> + auto& o = ob << "accepting archive types ";
> for (auto&& arch : scan_archives)
> o << arch.first << "(" << arch.second << ") ";
> o << endl;
> @@ -3884,37 +3929,40 @@ main (int argc, char *argv[])
>
> vector<pthread_t> all_threads;
>
> - pthread_t pt;
> - rc = pthread_create (& pt, NULL, thread_main_groom, NULL);
> - if (rc)
> - error (EXIT_FAILURE, rc, "cannot spawn thread to groom database\n");
> - else
> - {
> -#ifdef HAVE_PTHREAD_SETNAME_NP
> - (void) pthread_setname_np (pt, "groom");
> -#endif
> - all_threads.push_back(pt);
> - }
> -
> - if (scan_files || scan_archives.size() > 0)
> + if (! passive_p)
> {
> - rc = pthread_create (& pt, NULL, thread_main_fts_source_paths, NULL);
> + pthread_t pt;
> + rc = pthread_create (& pt, NULL, thread_main_groom, NULL);
> if (rc)
> - error (EXIT_FAILURE, rc, "cannot spawn thread to traverse source
> paths\n");
> + error (EXIT_FAILURE, rc, "cannot spawn thread to groom database\n");
> + else
> + {
> #ifdef HAVE_PTHREAD_SETNAME_NP
> - (void) pthread_setname_np (pt, "traverse");
> + (void) pthread_setname_np (pt, "groom");
> #endif
> - all_threads.push_back(pt);
> -
> - for (unsigned i=0; i<concurrency; i++)
> + all_threads.push_back(pt);
> + }
> +
> + if (scan_files || scan_archives.size() > 0)
> {
> - rc = pthread_create (& pt, NULL, thread_main_scanner, NULL);
> + rc = pthread_create (& pt, NULL, thread_main_fts_source_paths,
> NULL);
> if (rc)
> - error (EXIT_FAILURE, rc, "cannot spawn thread to scan source
> files / archives\n");
> + error (EXIT_FAILURE, rc, "cannot spawn thread to traverse source
> paths\n");
> #ifdef HAVE_PTHREAD_SETNAME_NP
> - (void) pthread_setname_np (pt, "scan");
> + (void) pthread_setname_np (pt, "traverse");
> #endif
> all_threads.push_back(pt);
> +
> + for (unsigned i=0; i<concurrency; i++)
> + {
> + rc = pthread_create (& pt, NULL, thread_main_scanner, NULL);
> + if (rc)
> + error (EXIT_FAILURE, rc, "cannot spawn thread to scan source
> files / archives\n");
> +#ifdef HAVE_PTHREAD_SETNAME_NP
> + (void) pthread_setname_np (pt, "scan");
> +#endif
> + all_threads.push_back(pt);
> + }
> }
> }
This looks OK when viewed with diff -u -w.
> @@ -3936,14 +3984,17 @@ main (int argc, char *argv[])
> if (d4) MHD_stop_daemon (d4);
> if (d6) MHD_stop_daemon (d6);
>
> - /* With all threads known dead, we can clean up the global resources. */
> - rc = sqlite3_exec (db, DEBUGINFOD_SQLITE_CLEANUP_DDL, NULL, NULL, NULL);
> - if (rc != SQLITE_OK)
> + if (! passive_p)
> {
> - error (0, 0,
> - "warning: cannot run database cleanup ddl: %s",
> sqlite3_errmsg(db));
> + /* With all threads known dead, we can clean up the global resources.
> */
> + rc = sqlite3_exec (db, DEBUGINFOD_SQLITE_CLEANUP_DDL, NULL, NULL,
> NULL);
> + if (rc != SQLITE_OK)
> + {
> + error (0, 0,
> + "warning: cannot run database cleanup ddl: %s",
> sqlite3_errmsg(db));
> + }
> }
> -
> +
Adding spurious whitespace...
> // NB: no problem with unconditional free here - an earlier failed regcomp
> would exit program
> (void) regfree (& file_include_regex);
> (void) regfree (& file_exclude_regex);
> @@ -3952,7 +4003,8 @@ main (int argc, char *argv[])
> sqlite3 *databaseq = dbq;
> db = dbq = 0; // for signal_handler not to freak
> (void) sqlite3_close (databaseq);
> - (void) sqlite3_close (database);
> + if (! passive_p)
> + (void) sqlite3_close (database);
>
> return 0;
> }
OK.
> diff --git a/doc/ChangeLog b/doc/ChangeLog
> index db3a35844ce9..7a73fa107bf3 100644
> --- a/doc/ChangeLog
> +++ b/doc/ChangeLog
> @@ -1,3 +1,8 @@
> +2021-11-05 Frank Ch. Eigler <f...@redhat.com>
> +
> + PR28430
> + * debuginfod.8 (--passive): Document new flag & operation mode.
Thanks for adding documentation. Looks good.
> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index b791cd7f0d95..394241c7e179 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-11-05 Frank Ch. Eigler <f...@redhat.com>
> +
> + PR28430
> + * run-debuginfod-extraction-passive.sh: New test.
> + * Makefile.am (TESTS, EXTRA_DIST): Add it.
Looks correct.
> +++ b/tests/run-debuginfod-extraction-passive.sh
>
> +# for test case debugging, uncomment:
> +set -x
> +unset VALGRIND_CMD
You aren't using testrun so this isn't really relevant.
But you might use VALGRIND_CMD itself in some the tests to make them
run under valgrind when configure to do so. Most tests should be fine,
as long as they don't mind that valgrind itself might query the
debuginfod server.
Cheers,
Mark
