Project

General

Profile

Actions

Bug #18487

closed

controller fails to start when vocab properties file is invalid (e.g. duplicate synonyms)

Added by Ward Vandewege 9 months ago. Updated 5 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
01/20/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Need to check vocabulary file as part of config-check.

Since config-check is also meant to be usable on nodes that aren't running controller and don't need access to the vocabulary file, either
  • detect whether this host matches a controller InternalURL, and if not, don't treat ENOENT as an error, or
  • don't ever treat ENOENT as an error

Should print out enough information so that the admin can edit the file to work around the problem.

Need to document the monitoring features that were added.

Document controller behavior when the vocab changes at runtime.


Subtasks 1 (0 open1 closed)

Task #18568: Review 18487-vocab-config-checkResolvedWard Vandewege01/20/2022

Actions

Related issues

Related to Arvados - Bug #18488: [controller] does not release pg_advisory_lock($1) when it fails to startResolved12/02/2021

Actions
Related to Arvados Epics - Story #17454: Vocabulary checking of properties by API server/controllerResolved10/01/202103/31/2022

Actions
Actions #1

Updated by Ward Vandewege 9 months ago

  • Related to Bug #18488: [controller] does not release pg_advisory_lock($1) when it fails to start added
Actions #2

Updated by Peter Amstutz 8 months ago

  • Category set to API
  • Description updated (diff)
  • Subject changed from [controller] fails to start when vocab properties file is invalid (e.g. duplicate synonyms) to controller fails to start when vocab properties file is invalid (e.g. duplicate synonyms)
Actions #3

Updated by Peter Amstutz 8 months ago

  • Target version changed from 2021-12-08 sprint to 2022-01-05 sprint
Actions #4

Updated by Peter Amstutz 8 months ago

  • Assigned To set to Lucas Di Pentima
Actions #5

Updated by Tom Clegg 8 months ago

  • Description updated (diff)
Actions #6

Updated by Lucas Di Pentima 8 months ago

  • Target version changed from 2022-01-05 sprint to 2022-01-19 sprint
Actions #7

Updated by Lucas Di Pentima 7 months ago

  • Related to Story #17454: Vocabulary checking of properties by API server/controller added
Actions #8

Updated by Lucas Di Pentima 7 months ago

  • Status changed from New to In Progress
Actions #9

Updated by Lucas Di Pentima 7 months ago

  • Target version changed from 2022-01-19 sprint to 2022-02-02 sprint
Actions #10

Updated by Lucas Di Pentima 7 months ago

Updates at b701b0f38 - branch 18487-vocab-config-check
Test run: developer-run-tests: #2882

  • Adds vocabulary checking to arvados-server config-check, ignoring the special case when there's an API.VocabularyPath value set, but no file is found, to avoid false positives on non-controller nodes.
  • Improves error detection by also checking for duplicated JSON keys that were previously ignored.
  • Improves error reporting by collecting all the issues before returning, so that the config-check command and also the /_health/vocabulary endpoint do a full issue report and not annoy the site admin by providing errors one at a time.
  • Improves the Metadata vocabulary documentation page on the Admin section.
  • Adds tests.
Actions #11

Updated by Ward Vandewege 7 months ago

Lucas Di Pentima wrote:

Updates at b701b0f38 - branch 18487-vocab-config-check
Test run: developer-run-tests: #2882

  • Adds vocabulary checking to arvados-server config-check, ignoring the special case when there's an API.VocabularyPath value set, but no file is found, to avoid false positives on non-controller nodes.
  • Improves error detection by also checking for duplicated JSON keys that were previously ignored.
  • Improves error reporting by collecting all the issues before returning, so that the config-check command and also the /_health/vocabulary endpoint do a full issue report and not annoy the site admin by providing errors one at a time.
  • Improves the Metadata vocabulary documentation page on the Admin section.
  • Adds tests.

In lib/config/cmd.go:

The addition to config-check isn't ideal; if there are any warnings from the checks above it (e.g. if ManagementToken is not set), config-check aborts without even doing the new test, and nothing is printed.

Would it be possible to move the new tests above the diff command, and send their output to logbuf rather than returning immediately so they are handled the same way as other warnings? That would also make the output more uniform, for better or worse we output structured logs from config-check, which is hard to read, but at least it will be consistent.

Otherwise, LGTM, thanks.

Actions #12

Updated by Lucas Di Pentima 7 months ago

Updates at 4f1341cf0
Test run: developer-run-tests: #2888

Thanks for the suggestions, I totally missed the logging part.
I've moved the vocabulary checking up a little and use the logger to output the reporting (sadly, it's kind of unreadable now) so that it can be handled the same way as the rest.
Had to adjust a test regexp, too.

Actions #13

Updated by Ward Vandewege 7 months ago

Lucas Di Pentima wrote:

Updates at 4f1341cf0
Test run: developer-run-tests: #2888

Thanks for the suggestions, I totally missed the logging part.
I've moved the vocabulary checking up a little and use the logger to output the reporting (sadly, it's kind of unreadable now) so that it can be handled the same way as the rest.
Had to adjust a test regexp, too.

LGTM, thanks! I think we need to change the default output for config-check to human readable, perhaps with a machine-friendly output format as an optional flag. But not as part of this ticket.

Actions #14

Updated by Lucas Di Pentima 7 months ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados-private:commit:arvados|107af77a83788ebdd0cbcfdcae91fed44deec11d.

Actions #15

Updated by Peter Amstutz 5 months ago

  • Release set to 46
Actions

Also available in: Atom PDF