Bug #18487
closed
controller fails to start when vocab properties file is invalid (e.g. duplicate synonyms)
Added by Ward Vandewege about 3 years ago.
Updated over 2 years ago.
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.
- Related to Bug #18488: [controller] does not release pg_advisory_lock($1) when it fails to start added
- 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)
- Target version changed from 2021-12-08 sprint to 2022-01-05 sprint
- Assigned To set to Lucas Di Pentima
- Description updated (diff)
- Target version changed from 2022-01-05 sprint to 2022-01-19 sprint
- Related to Idea #17454: Vocabulary checking of properties by API server/controller added
- Status changed from New to In Progress
- Target version changed from 2022-01-19 sprint to 2022-02-02 sprint
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.
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.
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.
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.
- Status changed from In Progress to Resolved
Applied in changeset arvados-private:commit:arvados|107af77a83788ebdd0cbcfdcae91fed44deec11d.
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by 
Updated by