Fix datarace when reading handlers in SharedHandler #91
Conversation
This file contains hidden or 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
MbolotSuse
suggested changes
Jul 31, 2024
Contributor
MbolotSuse
left a comment
MbolotSuse
left a comment
There was a problem hiding this comment.
This seems to be close to what the code was previously doing: a3ff816. I think we need more consideration on this one before making changes.
Contributor
|
Before the code was holding the lock while it ran each handler (and there are various metrics in the loop that suggest people have been concerned with how long it takes to update an object by applying an array of (mutating webhook?) handlers to it. Now the use of the lock suggests that it's waiting for the array of handlers to be built, and then it uses it. But the fact of the race condition in the helm-lock tests suggest that something else is modifying the array while the lasso sharedhandler loops through it, so it's safer to make a shallow copy of it and loop through that. If the underlying |
ericpromislow
previously approved these changes
Jul 31, 2024
Contributor
ericpromislow
left a comment
ericpromislow
left a comment
There was a problem hiding this comment.
Well, I wrote this code, but I think it's correct, and not onerously expensive.
Contributor
|
We could avoid making a copy for every |
* Wrap the main and count queries in ListByOptions with a transaction (rancher#112) * Ensure that all db-related failures roll back the transaction. There's only one remaining instance in lasso of a function that takes an instance of `db.TXClient`, finds an error situation that doesn't happen due to a database call (`Exec` or `ExecStmt`), and doesn't rollback the transaction before returning a non-nil error. --------- Co-authored-by: Max Sokolovsky <max.sokolovsky@suse.com>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
* sql: silence informer errors on unwatchable resources Listable-but-unwatchable resources such as nodes metrics.k8s.io/v1beta1 allow an Informer to be started, but it will by default log all watch failures. Add a boolean flag to silence those errors. Signed-off-by: Silvio Moioli <silvio@moioli.net> * adapt tests Signed-off-by: Silvio Moioli <silvio@moioli.net> --------- Signed-off-by: Silvio Moioli <silvio@moioli.net>
* vai: log errors on add client-go unfortunately "swallows" them Signed-off-by: Silvio Moioli <silvio@moioli.net> * vai: log errors on delete Signed-off-by: Silvio Moioli <silvio@moioli.net> * Apply suggestions from code review Co-authored-by: Tom Lebreux <me@tomlebreux.com> * Update pkg/cache/sql/store/store.go Co-authored-by: Tom Lebreux <me@tomlebreux.com> --------- Signed-off-by: Silvio Moioli <silvio@moioli.net> Co-authored-by: Tom Lebreux <me@tomlebreux.com>
* Support searching by metadata.label fields: * Add labels when adding/replacing objects. * Add labels to the query language * Add a unit test for label tests. * Stick with terse in-place table alias names rather than interpolate descriptive named constants. Also: Move all the label operations from store to listoption_indexer. * Run go generate after refactoring label handling. * Move label handling out of store.go and into the listoption indexer. * Fix the number of args in the mocked calls to creating the indices tables. - tx.Exec takes only one argument. * Split 'ListOptionIndexer.afterUpsert' into two funcs with more descriptive names. * Split the 'afterX' methods into two parts, with better names. * Refactor the query-generator into its own method. * Split 'ListByOptions' into two functions to simplify testing. * Add tests for converting other ops to sql stmts, fixing breakage. * Add more tests, fix NOT-EXISTS for labels. * Code quality changes: - Fixed rendering NOT-EXISTS queries. - Wrap query error in a 'db.QueryError' object. - Use consistent error message when failing to get a unstructured object. - Don't include ORDER-BY clauses in COUNT queries. - Don't bother pulling the various fields out of the `queryInfo` struct - Pull the count queryInfo parts out only when needed. * Simplify the way the final queryInfo struct is built. In particular, don't clear the count query values if no count query needs to be made -- just leave the default struct values, and the query executor won't run a count-query. * No need to map nil to an empty array when there's no count query. * Process comparisons in the query strings. * Allow exist-tests on filters only on labels. * Fix the labels-not-exists (and not-equals) sql expressions. Conceptually it's simpler to do a '<key> NOT IN (SELECT o1.key...)' to find the rows that don't have the target label. * Use 'LEFT OUTER JOIN' on label tables only where necessary. For example, if the test is to find all rows where `metadata.labels.animal = "cows"`, we can ignore any rows that don't have associated labels, because they will never match. * LABEL <label> NOT IN (...) tests also succeed where the <label> isn't present. * Add unit tests on multiple filters with only positive-tests on labels. There are negative tests on non-label fields -- the goal here is to verify that we never do an OUTER JOIN when we don't need one. * Always LEFT-OUTER-JOIN when testing labels. * Two simplifications when querying on labels: - Always LEFT-OUTER-JOIN to retain fields that have no associated labels (for negative operators) - Always SELECT-DISTINCT in case some results return duplicate entries. And always SELECT DISTINCT when * Fix multiple filters involving label tests. When we have multiple filter sub-queries, these get ANDed together. But we need to do a self-join for each instance of a label-type filter. Added unit tests to verify that this is what we're generating.
…s. (rancher#122) * Replace primary/secondary sort fields with an array of sort directives. * Return an error if # sort-fields != # sort-orders * Rebase against main to pull in separated sql generator and matcher. * Add unit tests checking that sort-fields match up with sort-orders.
* Include release/0.1 branch in renovate Signed-off-by: Vatsal Parekh <vatsalparekh@outlook.com> * Add release GHA Signed-off-by: Vatsal Parekh <vatsalparekh@outlook.com> * Add Versioning Doc Signed-off-by: Vatsal Parekh <vatsalparekh@outlook.com> * Correct branch in renovate Signed-off-by: Vatsal Parekh <vatsalparekh@outlook.com> --------- Signed-off-by: Vatsal Parekh <vatsalparekh@outlook.com>
Co-authored-by: renovate-rancher[bot] <119870437+renovate-rancher[bot]@users.noreply.github.com>
…ncher#134) Co-authored-by: renovate-rancher[bot] <119870437+renovate-rancher[bot]@users.noreply.github.com>
Co-authored-by: renovate-rancher[bot] <119870437+renovate-rancher[bot]@users.noreply.github.com>
Co-authored-by: renovate-rancher[bot] <119870437+renovate-rancher[bot]@users.noreply.github.com>
Co-authored-by: renovate-rancher[bot] <119870437+renovate-rancher[bot]@users.noreply.github.com>
Co-authored-by: renovate-rancher[bot] <119870437+renovate-rancher[bot]@users.noreply.github.com>
…20.5 (rancher#133) Co-authored-by: renovate-rancher[bot] <119870437+renovate-rancher[bot]@users.noreply.github.com>
alexandreLamarre
force-pushed
the
master
branch
from
622f79c to
87cab81
Compare
Contributor
|
This needs to be redone against |
Signed-off-by: Alexandre Lamarre <alexandre.lamarre@suse.com>
alexandreLamarre
force-pushed
the
master
branch
from
87cab81 to
60991a3
Compare
Contributor
|
Does this really need 19 commits changing 49 files? And you also want to merge this against |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.
No description provided.