libnet: pass store as an arg to netdrivers#49158
Conversation
akerouanton
added
status/2-code-review
area/networking
akerouanton
force-pushed
the
netdrivers-store-param
branch
2 times, most recently
from
024499e to
82bc688
Compare
|
Hm.. would love to have some eyes from @corhere on this PR; wasn't there a discussion as well to get rid of the datastore (so some of the "pass through config" to be by design to move away from it? Also wondering; does this interface change of the Register functions break things elsewhere? (I recall some things also being present in the swarm kit repository, and plugins, but have to dig in memory on some of that).
Curious; I see all |
I think what you're referring to is what commit d21d088 did. Before that commit, we were passing the datastore config to every stateful driver, and then each would initialize their own instance of the datastore. Since the same config was passed, they were all racing for the same file lock -- with a default timeout of 1 minute. That timeout was removed in 4d7c11c. I recall we discussed removing datastore from netdrivers during a networking sync call, but that was mostly a random thought. Maybe we'll do that some day, but we still need to figure out how persistence should be done, how to manage concurrency, etc… It's not a priority, and not something that will happen some time soon IMO.
All the For Swarmkit integration, we've synthetic drivers that get registered here. These are left untouched too -- so nothing should break in Swarmkit. |
| tempDir, err := os.MkdirTemp("", "*") | ||
| assert.NilError(t, err) | ||
|
|
||
| ds, err := datastore.New(tempDir, "libnetwork") | ||
| assert.NilError(t, err) | ||
|
|
||
| t.Cleanup(func() { | ||
| assert.NilError(t, os.RemoveAll(tempDir)) | ||
| }) |
There was a problem hiding this comment.
| tempDir, err := os.MkdirTemp("", "*") | |
| assert.NilError(t, err) | |
| ds, err := datastore.New(tempDir, "libnetwork") | |
| assert.NilError(t, err) | |
| t.Cleanup(func() { | |
| assert.NilError(t, os.RemoveAll(tempDir)) | |
| }) | |
| ds, err := datastore.New(t.TempDir(), "libnetwork") | |
| assert.NilError(t, err) |
There was a problem hiding this comment.
TempDir returns the default directory to use for temporary files.
If we do that, all tests are going to use the same boltdb file. This could result in weird test failures, and this would prevent parallel execution.
I think many tests can't be parallelized currently because they share the same netns but let's not make it worse 🙂
There was a problem hiding this comment.
There was a problem hiding this comment.
Forget what I said, I thought you were suggesting os.TempDir() 🙈 I'll do that change!
|
Thanks both!! Sorry for the noise on my side; I wanted to be sure we were not undoing work that was set in progress ❤️ |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
(pending the conversation above on t.TempDir(), which I'm "+1'" on)
Before that change, we were passing the datastore to network drivers
through a `map[string]interface{}`. Then, each driver that needed the
store would cast the datastore to the correct type.
This was not a good design, as it was not clear which drivers were using
the store and which were not. Not all unit tests were passing the store,
leading to logs about uninitialized store being written.
This change makes the store a parameter of the `RegisterX` functions.
All unit tests are now passing a valid datastore to the drivers. A new
testutil func is added for that purpose.
Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
netdrivers-store-param
branch
from
82bc688 to
e5bf6d8
Compare
3 participants
- What I did
Before that change, we were passing the datastore to network drivers through a
map[string]interface{}. Then, each driver that needed the store would cast the datastore to the correct type.This was not a good design, as it was not clear which drivers were using the store and which were not. Not all unit tests were passing the store, leading to logs about uninitialized store being written.
This change makes the store a parameter of the
RegisterXfunctions. All unit tests are now passing a valid datastore to the drivers. A new testutil func is added for that purpose.- How to verify it
Existing unit tests.