phew, these rebases are rough.

And realizing that hostconfig.Links was being set to nil before, so this information is only in sqlite...

−158,972

Looks cool
windows failures might be related though

Wow this is epic. Thanks so much @cpuguy83 !

Is there any undesirable side effect you can think of?

Maybe an obvious question, but does this mean that links won't survive a daemon restart, or were they still stored on-disk somewhere else besides SQLite?

Adding some context this from IRC convo:
Yes there is an issue here:
Currently hostConfig.Links is wiped out the first time it is processed.
So the only place links info is stored currently is in the sqlite db... so we can't just remove it here, it will require some migration code to insert links back into containers' hostconfig from sqlite.

Confirmed that this works with Windows containers (both existing images after updating just docker.exe and on a fresh install). Not sure why Windows Jenkins run is failing, but it works fine in manual tests.

LGTM

Ping @mavenugo @mrjana: can you please look into this in light of your plans regarding links deprecation (or not)?

ping @mavenugo @mrjana could you have a look at #16032 (comment) now that 1.9 has been release (and you hopefully have a bit more time ❤️)

we cannot remove links from one version to the other.

If we're really going to remove links we should deprecated them in 1.10 to completely remove sqlite3 and the code related in 1.12.

If we're going to keep links as they are, we can make the transition to in-memory links in 1.10, loading from sqlite to memory in that version, and remove the dependency in 1.11.

If we're going to keep links with a different implementation under the hood, we should make sure we remove sqlite from the dependencies.

@calavera unfortunately we are stuck with sqlite in any case since HostConfig.Links is cleared out once read, currently. So the only place the link info is stored is in sqlite... so basically as long as we want to support upgrading from --> it'll need to stay around.

@cpuguy83 should we close this for now or is this still an option? I see a discussion around removal of links, which is not yet clear when that'll happen

@thaJeztah The overall goal is still valid, don't use sqlite for storing things on disk which we can build at runtime easily enough.
This should ultimately be much more efficient up to a certain point.

@cpuguy83 need rebase
Did you add migration logic?

@LK4D4 Not yet.
Rebased for now, will work on migration.

I might be wrong but, the graceful downgrade is guaranteed unless we remove linkgraph.db. Obviously, if people add new containers to 1.10 and then downgrade those names will be lost. But old container names will be preserved in the database as long as the database file exists. That's guarantee enough for me.

That's fine, I updated to make it not remove the db.

All green

LGTM

LGTM
let's test this in real world :)

ping @ibuildthecloud aka, demolisher of code, hopes, and dreams.

Yay, merged! Thanks for being so patient @cpuguy83 🤘

Is this the candidate fix to #17720? If this is on experimental I'm down to see if it'll still lockup on my production boxes.

@vincentwoo This definitely speeds up some pieces of docker ps, and container creation. I'm not sure that it would fix speed reductions over time.

I can give it a run. Is it on https://experimental.docker.com? I have a hard time figuring out what revision that thing is at.

This is in stable.

Right, but there's not binaries out against it yet, right? Slated for 1.10?

https://master.dockerproject.org

Niiiice.