Skip to content

fix "host-gateway-ip" label not set for builder workers #47187

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.

Sign up for GitHub

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

Jump to bottom
Merged
thaJeztah merged 1 commit into from
Jan 23, 2024
Merged

fix "host-gateway-ip" label not set for builder workers #47187

thaJeztah merged 1 commit into from
Jan 23, 2024

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jan 23, 2024 •
edited
Loading

Commit 21e50b8 added a label on the buildkit worker to advertise the host-gateway-ip. This option can be either set by the user in the daemon config, or otherwise defaults to the gateway-ip.

If no value is set by the user, discovery of the gateway-ip happens when initializing the network-controller (NewDaemon, daemon.restore()).

However d222bf0 changed how we handle the daemon config. As a result, the cli.Config used when initializing the builder only holds configuration information form the daemon config (user-specified or defaults), but is not updated with information set by NewDaemon.

This patch adds an accessor on the daemon to get the current daemon config. An alternative could be to return the config by NewDaemon (which should likely be a copy of the config).

Before this patch:

docker buildx inspect default
Name:   default
Driver: docker

Nodes:
Name:      default
Endpoint:  default
Status:    running
Buildkit:  v0.12.4+3b6880d2a00f
Platforms: linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
Labels:
 org.mobyproject.buildkit.worker.moby.host-gateway-ip: <nil>

After this patch:

docker buildx inspect default
Name:   default
Driver: docker

Nodes:
Name:      default
Endpoint:  default
Status:    running
Buildkit:  v0.12.4+3b6880d2a00f
Platforms: linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
Labels:
 org.mobyproject.buildkit.worker.moby.host-gateway-ip: 172.18.0.1

- What I did

- How I did it

- How to verify it

- Description for the changelog

Fix `host-gateway-ip` not working during build when not set through configuration

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

@thaJeztah thaJeztah self-assigned this Jan 23, 2024
@thaJeztah thaJeztah added this to the 26.0.0 milestone Jan 23, 2024
@thaJeztah thaJeztah mentioned this pull request Jan 23, 2024
Merged
@thaJeztah
fix "host-gateway-ip" label not set for builder workers
00c9785 
Commit 21e50b8 added a label on the buildkit
worker to advertise the host-gateway-ip. This option can be either set by the
user in the daemon config, or otherwise defaults to the gateway-ip.

If no value is set by the user, discovery of the gateway-ip happens when
initializing the network-controller (`NewDaemon`, `daemon.restore()`).

However d222bf0 changed how we handle the
daemon config. As a result, the `cli.Config` used when initializing the
builder only holds configuration information form the daemon config
(user-specified or defaults), but is not updated with information set
by `NewDaemon`.

This patch adds an accessor on the daemon to get the current daemon config.
An alternative could be to return the config by `NewDaemon` (which should
likely be a _copy_ of the config).

Before this patch:

    docker buildx inspect default
    Name:   default
    Driver: docker

    Nodes:
    Name:      default
    Endpoint:  default
    Status:    running
    Buildkit:  v0.12.4+3b6880d2a00f
    Platforms: linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
    Labels:
     org.mobyproject.buildkit.worker.moby.host-gateway-ip: <nil>

After this patch:

    docker buildx inspect default
    Name:   default
    Driver: docker

    Nodes:
    Name:      default
    Endpoint:  default
    Status:    running
    Buildkit:  v0.12.4+3b6880d2a00f
    Platforms: linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
    Labels:
     org.mobyproject.buildkit.worker.moby.host-gateway-ip: 172.18.0.1

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
thaJeztah commented Jan 23, 2024
View reviewed changes
daemon/daemon.go
Comment on lines +183 to +184
// Config returns daemon's config.
func (daemon *Daemon) Config() config.Config {
Copy link
Member Author

@thaJeztah thaJeztah Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, not "stoked" by this solution, but it should get us going.

@akerouanton akerouanton mentioned this pull request Jan 23, 2024
Closed
@thaJeztah thaJeztah mentioned this pull request Jan 23, 2024
Merged
@thaJeztah
Copy link
Member Author

thaJeztah commented Jan 23, 2024

Failure is unrelated; and tracked in #47119

=== FAIL: amd64.integration.system TestDiskUsage/empty (0.00s)
    disk_usage_test.go:41: assertion failed: 
        --- du
        +++ →
          types.DiskUsage{
        - 	LayersSize: 4096,
        + 	LayersSize: 0,
          	Images:     {},
          	Containers: {},
          	... // 3 identical fields
          }
        
    --- FAIL: TestDiskUsage/empty (0.00s)

@thaJeztah thaJeztah marked this pull request as ready for review January 23, 2024 15:27
@thaJeztah
Copy link
Member Author

thaJeztah commented Jan 23, 2024

New failure is also unrelated (other flaky test); #46508

--- FAIL: TestLiveRestore (0.00s)
    --- SKIP: TestLiveRestore/autoremove (0.00s)
    --- FAIL: TestLiveRestore/volume_references (17.09s)
        --- PASS: TestLiveRestore/volume_references/restartPolicy (10.17s)
            --- PASS: TestLiveRestore/volume_references/restartPolicy/always (2.68s)
            --- PASS: TestLiveRestore/volume_references/restartPolicy/unless-stopped (2.62s)
            --- PASS: TestLiveRestore/volume_references/restartPolicy/on-failure (2.54s)
            --- PASS: TestLiveRestore/volume_references/restartPolicy/no (2.32s)
        --- FAIL: TestLiveRestore/volume_references/local_volume_with_mount_options (2.33s)
            --- SKIP: TestLiveRestore/volume_references/local_volume_with_mount_options/volume_still_mounted (0.00s)
        --- PASS: TestLiveRestore/volume_references/container_with_bind-mounts (2.34s)

@thaJeztah thaJeztah merged commit f19f233 into moby:master Jan 23, 2024
@thaJeztah thaJeztah deleted the fix_gateway_ip branch January 23, 2024 15:52
laurazard
laurazard approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM (but yeah, this is a messy area and we should track this somewhere to improve)

@thaJeztah
Copy link
Member Author

thaJeztah commented Jan 23, 2024

@laurazard yeah, agreed. I did an ever-so-slightly cleanup after I worked on this here; #47188

(nothing for the "global picture" though.

This was referenced Jan 24, 2024
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Nov 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crazy-max crazy-max crazy-max approved these changes

@vvoland vvoland vvoland approved these changes

@laurazard laurazard laurazard approved these changes

@akerouanton akerouanton Awaiting requested review from akerouanton

Assignees

@thaJeztah thaJeztah

Labels

area/builder/buildkit Build area/builder Build area/daemon Core Engine area/networking Networking kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review

Projects

None yet

Milestone

26.0.0

Development

Successfully merging this pull request may close these issues.

After Update to Docker 25.0.0 (buildx v0.12.1), host-gateway in build does not work anymore

4 participants

@thaJeztah @crazy-max @vvoland @laurazard