Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

volume/local: Make host resolution backwards compatible #47185

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Jan 23, 2024

Commit 8ae94ca added a DNS resolution of the device part of the volume option.

The previous way to resolve the passed hostname was to use addr option, which was handled by the same code path as the nfs mount type.

The issue is that addr is also an SMB module option handled by kernel and passing a hostname as addr produces an invalid argument error.

To fix that, restore the old behavior to handle addr the same way as before, and only perform the new DNS resolution of device if there is no addr passed.

NOTE: While kernel docs don't mention the addr option (https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html#use-instructions), it's actually present in the source: https://github.com/torvalds/linux/blob/7ed2632ec7d72e926b9e8bcc9ad1bb0cd37274bf/fs/smb/client/fs_context.c#L166

- What I did

- How I did it

- How to verify it

- Description for the changelog

- Fix CIFS volume mount error when passing an `addr` mount option

- A picture of a cute animal (not mandatory but encouraged)

@vvoland vvoland added this to the 26.0.0 milestone Jan 23, 2024
@vvoland vvoland requested a review from robmry January 23, 2024 11:54
@vvoland vvoland self-assigned this Jan 23, 2024
@vvoland vvoland force-pushed the volume-cifs-resolve-optout branch 2 times, most recently from 09b24d1 to 6387429 Compare January 23, 2024 11:57
volume/local/local_unix.go Outdated Show resolved Hide resolved
@vvoland vvoland force-pushed the volume-cifs-resolve-optout branch 2 times, most recently from bddccfd to cb33e4f Compare January 23, 2024 12:10
thaJeztah
thaJeztah previously approved these changes Jan 23, 2024
Copy link
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 thaJeztah dismissed their stale review January 23, 2024 12:12

Good call from @robmry; let's look at that

@vvoland vvoland force-pushed the volume-cifs-resolve-optout branch 3 times, most recently from 799dd22 to 0157e3d Compare January 23, 2024 12:31
Copy link
Contributor

@robmry robmry left a comment

Choose a reason for hiding this comment

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

LGTM - although duplicating the code seems like a bit of a shame... could factor it out into a closure that returns a bool to say whether it did anything?

Commit 8ae94ca added a DNS resolution
of the `device` part of the volume option.

The previous way to resolve the passed hostname was to use `addr`
option, which was handled by the same code path as the `nfs` mount type.

The issue is that `addr` is also an SMB module option handled by kernel
and passing a hostname as `addr` produces an invalid argument error.

To fix that, restore the old behavior to handle `addr` the same way as
before, and only perform the new DNS resolution of `device` if there is
no `addr` passed.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland
Copy link
Contributor Author

vvoland commented Jan 23, 2024

Removed the duplication, not using a closure though it would make it less readable and more complex than plain duplication IMO.

Copy link
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

Copy link
Contributor

@robmry robmry 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
Member

Seeing failures like this quite frequently in the BuildKit integration tests. Not specifically this address, but seems to be general lookup / "not found" failures (?)

=== FAIL: client TestIntegration (2.05s)
time="2024-01-23T14:01:29Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:42291"
time="2024-01-23T14:01:29Z" level=info msg=request digest="sha256:be691b1535726014cdf3b715ff39361b19e121ca34498a9ceea61ad776b9c215" mediatype=application/vnd.docker.image.rootfs.foreign.diff.tar size=10240 url="https://gist.github.com/cpuguy83/fcf3041e5d8fb1bb5c340915aabeebe0/raw/eebc59ee14939fc38067aa8b8dfbfba053d0af94/base.tar"
    run.go:266: copied docker.io/cpuguy83/buildkit-foreign:latest to local mirror localhost:42291/cpuguy83/buildkit-foreign:latest
time="2024-01-23T14:01:29Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:42291"
    run.go:266: copied docker.io/amd64/alpine:latest@sha256:25fad2a32ad1f6f510e528448ae1ec69a28ef81916a004d3629874104f8a7f70 to local mirror localhost:42291/library/alpine:latest
time="2024-01-23T14:01:30Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:42291"
    run.go:266: copied docker.io/tonistiigi/test:nolayers to local mirror localhost:42291/tonistiigi/test:nolayers

Either way; it's not related to this PR, so kicked CI again.

Hoping one day we have stats / metrics from CI, so that we can identify flaky tests.

@thaJeztah thaJeztah merged commit ca67dbd into moby:master Jan 23, 2024
126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

container with cifs volume and "addr"-option does not start with version 25
3 participants