libnet/ds, libnet/config: various cleanups#47992
Merged
thaJeztah merged 4 commits intomoby:masterfrom Sep 20, 2024
Merged
Conversation
akerouanton
added
status/2-code-review
area/networking
thaJeztah
reviewed
Jun 15, 2024
Member
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Nice!! I left a couple of comments;
⚠️ I went through commit-by-commit, so best to look through them as such, but- 👉 some of my comments may have become obsolete in commits after that.
- 💡 it may still make sense to look at my "outdated" comments, in case they provide alternative options.
Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
libnet-datastore-cleanup
branch
2 times, most recently
from
30e4e17 to
6ff6d10
Compare
This option fn was defining a custom directory, file name and bucket name for boltdb. Users can only change data-dir through `OptionDataDir`. Better reuse that function instead, that'll make refactorings easier. It won't set a custom bucket name or file name as `OptionBoltdbWithRandomDBFile` was doing, but that's not needed since every test will use a different temp dir anyway. Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
libnet-datastore-cleanup
branch
2 times, most recently
from
7314fe6 to
8b766a8
Compare
thaJeztah
reviewed
Sep 19, 2024
That function was needlessly complex. Instead of relying on a struct and a sub-struct, it now just takes two string params: a path and a bucket name. Libnetwork config is now initialized with default values. A new struct is introduced in libnetwork/config to let tests customize the path and bucket name. Signed-off-by: Albin Kerouanton <[email protected]>
Make sure an error is returned straight away if there's contention on the underlying db file. This makes sure we don't reintroduce the issue fixed in d21d088, and it will help detect contention in parallelized tests if they're badly written. It effectively adds a new error mode to the daemon, but if anyone faces this error, they should fix their process manager. Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
libnet-datastore-cleanup
branch
from
8b766a8 to
edcefd4
Compare
thaJeztah
approved these changes
Sep 20, 2024
Comment on lines
+39
to
+52
| // The bbolt package opens the underlying db file and then issues an | ||
| // exclusive flock to ensures that it can safely write to the db. If | ||
| // it fails, it'll re-issue flocks every few ms until Timeout is | ||
| // reached. | ||
| // This nanosecond timeout bypasses that retry loop and make sure the | ||
| // bbolt package returns an ErrTimeout straight away. That way, the | ||
| // daemon, and unit tests, will fail fast and loudly instead of | ||
| // silently introducing delays. | ||
| Timeout: time.Nanosecond, | ||
| }) | ||
| if err != nil { | ||
| if errors.Is(err, bolt.ErrTimeout) { | ||
| return nil, fmt.Errorf("boltdb file %s is already open", path) | ||
| } |
Member
There was a problem hiding this comment.
Could use extra eyes on this logic, just to be sure 🫶
robmry
approved these changes
Sep 20, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
- What I did
libnet/i/kv/boltdb: remove unused field 'timeout'
libnetwork: unit tests: drop OptionBoltdbWithRandomDBFile
This option fn was defining a custom directory, file name and bucket name for boltdb. Users can only change data-dir through
OptionDataDir. Better reuse that function instead, that'll make refactorings easier.It won't set a custom bucket name or file name as
OptionBoltdbWithRandomDBFilewas doing, but that's not needed since every test will use a different temp dir anyway.libnet/ds: simplify datastore.New()
That function was needlessly complex. Instead of relying on a struct and a sub-struct, it now just takes two string params: a path and a bucket name.
Libnetwork config is now initialized with default values. A new struct is introduced in libnetwork/config to let tests customize the path and bucket name.
libnet/i/kv/boltdb: fail fast in case of contention
Make sure an error is returned straight away if there's contention on the underlying db file. This makes sure we don't reintroduce the issue fixed in d21d088, and it will help detect contention in parallelized tests if they're badly written. It effectively adds a new error mode to the daemon, but if anyone faces this error, they should fix their process manager.
- How to verify it
CI.
- A picture of a cute animal (not mandatory but encouraged)