executor/oci: use fork of libnetwork/resolvconf #6086
Conversation
github-actions
bot
added
area/dependencies
|
Just a quick draft, because I was curious what it would look like /cc @robmry Also curious if this could help with this issue that was reported; edit: wrong link on my clipboard 😂 |
This comment was marked as resolved.
This comment was marked as resolved.
thaJeztah
force-pushed
the
fork_resolvconf
branch
from
cc5f635 to
7d74216
Compare
| // systemError implements [github.com/docker/docker/errdefs.ErrSystem]. | ||
| // | ||
| // We don't use the errdefs helpers here, because the resolvconf package | ||
| // is imported in BuildKit, and this is the only location that used the | ||
| // errdefs package outside of the client. | ||
| type errSystem struct{ error } | ||
| type systemError struct{ error } |
There was a problem hiding this comment.
This could probably use containerd/errdefs now
There was a problem hiding this comment.
We have our own Internal() classifier for such wrapping (integrated with the same System interface).
There was a problem hiding this comment.
Ah, that's the errdefs one? I can replace it for that.
This PR was a quick write up; still needs some cleaning up.
There was a problem hiding this comment.
Updated to use buildkit's errdefs
| "path/filepath" | ||
|
|
||
| "github.com/docker/docker/libnetwork/resolvconf" | ||
| "github.com/moby/buildkit/executor/oci/internal/resolvconf" |
There was a problem hiding this comment.
Not sure why we need this resolvconf package? Can we not just put content of this package directly here and make functions private?
There was a problem hiding this comment.
Not sure if that would help much; generally in go, the package name is part of the name; separate package makes it nice an isolated (resolvconf.Parse) the alternative would be resolveconfParse etc.
I also tried to keep it as it's in moby for now; also discussing with @robmry if we possibly should consider moving the internal/ package in moby to a separate module, because it's quite generic, with some small parts that were written for legacy bits.
There was a problem hiding this comment.
We usually try to avoid internal as enforced visibility in BuildKit but we can look at this as follow-up.
There was a problem hiding this comment.
@crazy-max If we are going to go ahead with this (when outside of draft) then carry to remove internal, and in later follow-up should try to remove dependency of text/template as that pkg should be considered harmful, and don't want new dependencies to it.
|
Ah, yes there's some template in this one; I haven't looked yet if it's easy to replace, but I don't think anyone would object against already doing so in moby/moby (I think it's a fixed template, so nothing customizable, so I'd be 👍 to replace it) |
|
@tonistiigi gave it a go, for fun; Before/After: |
thaJeztah
force-pushed
the
fork_resolvconf
branch
2 times, most recently
from
0ef1546 to
2a0a71d
Compare
thaJeztah
force-pushed
the
fork_resolvconf
branch
4 times, most recently
from
9f27461 to
3f7d11f
Compare
|
@tonistiigi @crazy-max I think this is ready for review PTAL |
There was a problem hiding this comment.
Don't use github.com/containerd/log but github.com/moby/buildkit/util/bklog:
| "path/filepath" | ||
|
|
||
| "github.com/docker/docker/libnetwork/resolvconf" | ||
| "github.com/moby/buildkit/executor/oci/internal/resolvconf" |
There was a problem hiding this comment.
We usually try to avoid internal as enforced visibility in BuildKit but we can look at this as follow-up.
Add a fork of github.com/docker/docker/daemon/libnetwork/internal/resolvconf, taken at commit [254f64ded64027db0d2d1531a8ef9015de68e2f2]. I did not preserve git history for this one (just a copy), but history can be found in the Moby repository if needed. [254f64ded64027db0d2d1531a8ef9015de68e2f2]: moby/moby@254f64d Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Rewrite the resolvconf code to use libnetwork's internal packege, which allows us to skip some of the moby-specific handling (writing to a file, creating a hash of the file to detect changes made by the user (not supported by BuildKit, which always mounts read-only). This rewrite also allows us to skip GetNameservers, GetSearchDomains, GetOptions, and FilterResolvDNS, which repeatedly would parse the resolvconf file for each of them. The new code parses the original resolvconf once, after which mutations (overrides) are done in memory, after which we generate the resolv.conf to write to disk. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
fork_resolvconf
branch
from
3f7d11f to
4e1e0fe
Compare
Mostly wanted to keep our options open here, in case things get moved. |
executor/oci: add fork of moby resolvconf (does not compile)
Add a fork of github.com/docker/docker/daemon/libnetwork/internal/resolvconf,
taken at commit 254f64ded64027db0d2d1531a8ef9015de68e2f2. I did not
preserve git history for this one (just a copy), but history can be found
in the Moby repository if needed.
executor/oci: use fork of libnetwork/resolvconf
Rewrite the resolvconf code to use libnetwork's internal packege, which
allows us to skip some of the moby-specific handling (writing to a file,
creating a hash of the file to detect changes made by the user (not
supported by BuildKit, which always mounts read-only).
This rewrite also allows us to skip GetNameservers, GetSearchDomains, GetOptions,
and FilterResolvDNS, which repeatedly would parse the resolvconf file for
each of them.
The new code parses the original resolvconf once, after which mutations
(overrides) are done in memory, after which we generate the resolv.conf to
write to disk.