config: fix missing and stuck alerts #9598
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andreyaksenov
approved these changes
Jan 17, 2024
Lord-KA
reviewed
Jan 17, 2024
Serpentian
approved these changes
Jan 17, 2024
Lord-KA
reviewed
Jan 18, 2024
ochaplashkin
approved these changes
Jan 18, 2024
|
I've marked this PR as draft to work on it a bit more. I'll answer to the discussions, but some changes are only in my local copy at the moment. I'll sync everything a bit later and will notify the participants by a message here. |
Totktonada
force-pushed
the
gh-9574-gh-9586-config-alert-fixes
branch
3 times, most recently
from
c701892
to
6ae9a0a
Compare
Totktonada
requested review from
Lord-KA,
ochaplashkin,
andreyaksenov and
Serpentian
Lord-KA
reviewed
Jan 27, 2024
ochaplashkin
approved these changes
Jan 29, 2024
Lord-KA
reviewed
Jan 29, 2024
Totktonada
force-pushed
the
gh-9574-gh-9586-config-alert-fixes
branch
from
6ae9a0a
to
f806f86
Compare
Totktonada
force-pushed
the
gh-9574-gh-9586-config-alert-fixes
branch
from
f806f86
to
ac095cf
Compare
Lord-KA
reviewed
Feb 2, 2024
Lord-KA
reviewed
Feb 2, 2024
Lord-KA
approved these changes
Feb 2, 2024
Serpentian
reviewed
Feb 2, 2024
It encapsulates all the alerts manipulation in one place, splits it from the main config logic and simplifies reading of the relevant code. Several tests are updated to use the public API instead of the internal one. Part of tarantool#9574 Part of tarantool#9586 NO_DOC=refactoring, no user-visible changes NO_CHANGELOG=see NO_DOC NO_TEST=see NO_DOC
Before this patch `config:info().alerts` reports only last alert of a certain type, while others are skipped. It is a regression from commit fa97cc0 ("config: introduce droppable alerts"). The idea of the fix is to eliminate a key from all the alerts that are not to be dropped later except on configuration reloading. Fixes tarantool#9586 NO_DOC=bugfix
Totktonada
force-pushed
the
gh-9574-gh-9586-config-alert-fixes
branch
2 times, most recently
from
36727e4
to
dc61d17
Compare
ImeevMA
approved these changes
Feb 2, 2024
The declarative configuration has the `credentials` section that describes users and their privileges. It is OK to have privileges for a space/function/sequence that does not exist. Such a privilege will lead to an alert that states that the privilege will be granted, when the object is created. The problem that is fixed by this commit is that such an alert was not dropped, when the object is created and the relevant privileges are granted. There are several ways to solve the problem. Let's look on them. 1. When a privilege is granted, drop an alert if any. 2. After the config-database privilege synchronization, revisit alerts to drop all obsolete ones. 3. Drop all the alerts regarding missed privileges before the config-database privilege synchronization and issue actual alerts afterwards. The first way is the simplest, but it doesn't cover one specific scenario: an object rename. Let's assume that the object T has privileges declared in the configuration and the object doesn't exist. There is an alert regarding it. Now, object S is renamed to T. Let's assume that S had some or all the privileges needed for T according to the configuration. In the given scenario, we don't need to grant some or all of the privileges and, so, the first solution doesn't work. We don't reach the code that grants the privileges and, so, dropping alerts at this point has no effect. The second and the third solutions are similar and mainly differs in how complicated the code is. The third one is implemented here with idea of simplifying the code. The internal `aboard` module has the following changes. 1. The `aboard` module now ignores underscored fields of an alert on its serialization to allow a caller to store a machine-readable information in them. 2. The new method `:drop_if()` is added to perform a conditional alert drop. Several unit test cases are updated, because now we always need initialized `config._aboard` for testing of the credentials applier. Fixes tarantool#9574 NO_DOC=bugfix
Totktonada
force-pushed
the
gh-9574-gh-9586-config-alert-fixes
branch
from
dc61d17
to
6da09c1
Compare
andreyaksenov
approved these changes
Feb 2, 2024
|
Pushed to Backported to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are cases, when an alert is not reported, when it should (#9586). There are cases, when an alert is present, but its reason is already gone (#9574). These problems are fixed in this patchset. See details in the commit messages.
Fixes #9574
Fixes #9586