Skip to content

Restore 27.x path for libnet's Bolt database#48793

Merged
robmry merged 1 commit intomoby:masterfrom
robmry:fix_libnet_db_path
Oct 29, 2024
Merged

Restore 27.x path for libnet's Bolt database#48793
robmry merged 1 commit intomoby:masterfrom
robmry:fix_libnet_db_path

Conversation

@robmry
Copy link
Copy Markdown
Contributor

@robmry robmry commented Oct 29, 2024

- What I did

In 27.x and earlier releases, libnetwork's database file was in a sub-directory network/files under the daemon's root data dir (/var/lib/docker/network/files/local-kv.db).

That part of the path got lost during refactoring in #47992 (ed08486).

So, libnet data ended up in the daemon's main Bolt db (/var/lib/docker/local-kv.db). Then, on upgrade from 27.x, config in the original file was no longer accessible.

- How I did it

libnet doesn't need access to any data outside its sub-dir, so change the meaning of its OptionDataDir - it now points at libnet's sub-dir, so the db will be created in the right place. Also, update other uses of that data dir to match.

Note that:

  • upgrade from a version of master before this change to one after will mean networks disappear, and
  • libnet config won't be removed from the daemon's main bolt db.

- How to verify it

Create a network with a 27.x daemon, upgrade to master, check the network is still there.

Tested in a Linux dev container, and on Windows.

Start a Linux swarm service with a port mapping (docker swarm init; docker service create -p 8080:80 nginx) and checked its hosts, resolv.conf and resolv.conf.hash files were still created in /var/lib/docker/network/files.

- Description for the changelog

n/a

@robmry
Restore 27.x path for libnet's Bolt database
9be6e90 
In 27.x and earlier releases libnetwork's database file was in a
sub-directory "network/files" under the daemon's root data dir.

That part of the path got lost in commit ed08486

So, libnet data ended up in the daemon's main Bolt db. Then, on
upgrade, config in the original file was no longer accessible.

libnet doesn't need access to any data outside its sub-dir, so
change the meaning of its OptionDataDir - it now points at libnet's
sub-dir, so the db will be created in the right place. Also, update
other uses of that data dir to match.

Signed-off-by: Rob Murray <[email protected]>
@robmry robmry added area/networking Networking kind/bugfix PR's that fix bugs version/28.0 labels Oct 29, 2024
@robmry robmry added this to the 28.0.0 milestone Oct 29, 2024
@robmry robmry self-assigned this Oct 29, 2024
@robmry robmry requested a review from akerouanton October 29, 2024 18:08
akerouanton
akerouanton approved these changes Oct 29, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

thaJeztah
thaJeztah approved these changes Oct 29, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 29, 2024

Should we update this one as well to use the new const?

moby/libnetwork/libnetwork_linux_test.go

Lines 46 to 47 in 53013c1

// Cleanup local datastore file
_ = os.Remove("/var/lib/docker/network/files/local-kv.db")

@robmry
Copy link
Copy Markdown
Contributor Author

robmry commented Oct 29, 2024

Should we update this one as well to use the new const?

moby/libnetwork/libnetwork_linux_test.go

Lines 46 to 47 in 53013c1

// Cleanup local datastore file
_ = os.Remove("/var/lib/docker/network/files/local-kv.db")

Before the refactoring, libnetwork had a const defaultPrefix = "/var/lib/docker/network/files", and this Remove used datastore.DefaultScope("").Client.Address to get to it. Now, the code's a lot simpler and libnet has no default location. So, the new bit of relative path (network/files) isn't enough to find the test-host's db file, and this hard-coded path seems like the best option.

But, removing the host's libnet db seems like quite a bad idea, particularly if these tests are run outside a dev container. Digging back to where it came from, I got to 71e14dd - but that's no help!

Some tests use config.OptionDataDir(t.TempDir()) to guarantee themselves a fresh start, that seems like the best approach. I think it'd probably be best to get rid of that TestMain ... but it's probably best to look into that more closely as a separate exercise?

@robmry
Copy link
Copy Markdown
Contributor Author

robmry commented Oct 29, 2024

I'll go ahead and merge this one, we can follow up on that TestMain separately.

@robmry robmry merged commit e775c68 into moby:master Oct 29, 2024
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 30, 2024

Some tests use config.OptionDataDir(t.TempDir()) to guarantee themselves a fresh start, that seems like the best approach. I think it'd probably be best to get rid of that TestMain ... but it's probably best to look into that more closely as a separate exercise?

Yes, using t.TempDir() if possible sounds like a good thing to do

@robmry robmry deleted the fix_libnet_db_path branch October 30, 2024 09:15
@robmry robmry mentioned this pull request Oct 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akerouanton akerouanton akerouanton approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

@robmry robmry

Labels

area/networking Networking kind/bugfix PR's that fix bugs version/28.0

Projects

None yet

Milestone

28.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@robmry @thaJeztah @akerouanton