Add hostname specifying for building #1339

jingxiaolu · 2020-01-22T12:49:18Z

Signed-off-by: l00397676 lujingxiao@huawei.com

I'm trying to send this hostname arg with build-arg:HOSTNAME=<value>

Added a hostname to message Meta in solver/pb/ops.proto.

When executor generates the hosts file (in executor/oci/hosts.go), replace the default hostname buildkitsandbox with the value user specified.

Test with this Dockerfile:

FROM docker.io/centos:7
RUN echo "Env variable HOSTNAME: $HOSTNAME" && \
    echo "hostname: $(hostname)" && \
    echo "kernel value: $(cat /proc/sys/kernel/hostname)"  && \
    echo "/etc/hosts: $(cat /etc/hosts)"

Test result:

# buildctl build --frontend=dockerfile.v0 --local dockerfile=. --local context=. --output type=docker,dest=docker.tar --opt build-arg:HOSTNAME=testtest --no-cache --progress=plain
#1 [internal] load .dockerignore
#1 transferring context: 2B done
#1 DONE 0.0s

#2 [internal] load build definition from Dockerfile
#2 transferring dockerfile: 256B done
#2 DONE 0.0s

#3 [internal] load metadata for docker.io/library/centos:7
#3 DONE 4.5s

#5 [1/2] FROM docker.io/library/centos:7@sha256:4a701376d03f6b39b8c2a8f4a8e...
#5 resolve docker.io/library/centos:7@sha256:4a701376d03f6b39b8c2a8f4a8e499441b0d567f9ab9d58e4991de4472fb813c done
#5 CACHED

#4 [2/2] RUN echo "Env variable HOSTNAME: $HOSTNAME" &&     echo "hostname:...
#4 0.172 Env variable HOSTNAME: testtest
#4 0.182 hostname: testtest
#4 0.183 kernel value: testtest
#4 0.185 /etc/hosts:
#4 0.185 127.0.0.1      localhost testtest
#4 0.185 ::1    localhost ip6-localhost ip6-loopback
#4 DONE 0.2s

#6 exporting to oci image format
#6 exporting layers
#6 exporting layers 2.0s done
#6 exporting manifest sha256:88d227d6e517608fc265e77967ac10a8b0ab47d3153af424aca66a5b72671d57 0.0s done
#6 exporting config sha256:cd2b4c557e9c29670f541df579edb3f59e6541aad4e9a4c17aa2b4ef7c9f88ea
#6 exporting config sha256:cd2b4c557e9c29670f541df579edb3f59e6541aad4e9a4c17aa2b4ef7c9f88ea 0.0s done
#6 sending tarball
#6 sending tarball 0.9s done
#6 DONE 3.0s

Signed-off-by: l00397676 <lujingxiao@huawei.com>

AkihiroSuda · 2020-01-23T13:27:39Z

executor/oci/hosts.go

-	if _, err := b.Write([]byte(hostsContent)); err != nil {
+	var hosts = hostsContent
+	if hostname != "" {
+		hosts = strings.Replace(hosts, "buildkitsandbox", hostname, 1)


strings.Replace() doesn't look robust.

const hostsContent should be changed to a function that takes hostname as a argument.

Wrapped to initHostsFile(), please help to review~

AkihiroSuda · 2020-01-23T13:28:06Z

frontend/dockerfile/dockerfile2llb/convert.go

@@ -315,6 +315,9 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State,
 			k, v := parseKeyValue(env)
 			d.state = d.state.AddEnv(k, v)
 		}
+		if val, ok := opt.BuildArgs["HOSTNAME"]; ok {


why upper chars?

Should these "special" build-args be namespaces somehow? This could easily conflict with someone having a ARG HOSTNAME in their dockerfile (is that a concern?)

This should be a frontend opt rather than build-arg?

I choose upper chars because bash always uses HOSTNAME to specifies it. And it seems it is easily causing confliction? @AkihiroSuda @thaJeztah what's your suggestion, use lower chars or other special names?

frontend opt sounds better, I'll update

@AkihiroSuda @thaJeztah I updated to --opt contexthostname=, is it better?

Why "context" prefix?

This hostname is only valid in this building context, so I added the "context" prefix. Is it OK?

I think "just" hostname would be ok from a UX perspective, as long as we document it properly (if the option is not set, the default is buildkitsandbox, and the hostname is only set during build, and not persisted in the resulting image)

@thaJeztah Thanks for your comments! In the newest patch I've updated it to hostname, now we can use it as buildctl build --opt hostname=blabla

Please use something more specific. Eg. exec-hostname run-hostname

AkihiroSuda · 2020-01-23T13:29:41Z

thanks for working on this, but please put your real name in "Signed-off-by" line

tonistiigi · 2020-01-24T01:39:49Z

executor/oci/hosts.go

 }

-func makeHostsFile(stateDir string, extraHosts []executor.HostIP, idmap *idtools.IdentityMapping) (string, func(), error) {
+func makeHostsFile(stateDir string, extraHosts []executor.HostIP, idmap *idtools.IdentityMapping, hostname string) (string, func(), error) {
 	p := filepath.Join(stateDir, "hosts")
 	if len(extraHosts) != 0 {
 		p += "." + identity.NewID()


this doesn't seem to handle the case where hostname changes for different executions

sorry @tonistiigi what do you mean by "different executions", could you share more info? Thanks~

There is some weird caching in this function with the Stat call. The changes here seem to conflict with that when different hostname values are passed.

@tonistiigi This is hard to imagine... From cmdline input's POV, there accepts only one "hostname".
Do you mean that if there're several cmdline executions with different hostnames, here makeHostsFile() may get different hostsnames at the same time? At this case, what should we do to elimite this case? Put the whole meta to GetHostsFile()?

@tonistiigi
Are you describing case when in Dockerfile hostname can overwritten by setting
ARG HOSTNAME in several layers?

@AkihiroSuda @thaJeztah maybe you can help us to find the correct solution? 5 months PR stuck.

@tonistiigi now this hostfile will be created with new one when the special hostname is specified, could you please check whether this is what you expected? Many thanks.

@tonistiigi ping

HI @tonistiigi
Could you check the new changes please?

Hi @tonistiigi
I saw that you added this issue to the 0.8.0 milestone. Many thanks!
Could you please review the fix above if it's looking good?

ShemTovYosef · 2020-01-28T17:00:31Z

Hi @jingxiaolu

Do we have a chance to include it to the preparing docker-ce 19.03.6?

With regards,
Yosef

jingxiaolu · 2020-02-04T01:40:10Z

@hinews I'm not in the docker-ce team actually I don't know their release plan.

AkihiroSuda · 2020-02-06T07:03:29Z

I'm not Docker employee either, but as a maintainer I assume this will be in 20.0X.0, not in 19.03.X.

ShemTovYosef · 2020-02-12T13:42:35Z

Hi,

any issues that stopping this pull request to be merged?

With regards,
Yosef

AkihiroSuda · 2020-02-12T14:14:02Z

@hinews The design is still under discussion: #1339 (comment)

ShemTovYosef · 2020-02-13T13:50:00Z

Off topic question.

Maybe someone know how to debug building image with buildx?
With legacy images when during build I could connect to the temporary container by bash and debug it. For buildx I don't see temporary container and don't know how to debug it.

Thanks in advance,
Yosef

thaJeztah · 2020-02-18T11:42:56Z

executor/oci/hosts.go

@@ -76,3 +70,14 @@ func makeHostsFile(stateDir string, extraHosts []executor.HostIP, idmap *idtools
 		os.RemoveAll(p)
 	}, nil
 }
+
+func initHostsFile(hostname string) string {


Wondering; is validation needed on the value? (i.e., would google.com or any other FQDN be allowed?)

@thaJeztah I checked the code in docker builder and buildah, but not found similar code logic, chould you please share me some hints on how to handle it? Or just leave it here when we meet with that scarios?

ShemTovYosef · 2020-02-29T19:15:41Z

Hi @jingxiaolu

Can you update me about this pull request? Can I help you with it somehow?

With regards,
Yosef

AlbinoGeek · 2020-04-26T21:58:13Z

Back when I used Docker... hell, this feature omission is why I don't anymore.

jingxiaolu · 2020-04-27T13:48:04Z

Just back from a hard and busy long milestone.

Now PR has rebased and updated, @tonistiigi @thaJeztah @AkihiroSuda PTAL, many thanks!

ShemTovYosef · 2020-05-01T15:58:17Z

Hi @jingxiaolu

Thank you very much for your effort.

@thaJeztah @AkihiroSuda @tonistiigi could it be merged? @jingxiaolu worked on this during several months, it’s very useful function that is missed in buildx.

With regards,
Yosef

tonistiigi

#1339 (review) has not been addressed afaics

thaJeztah · 2020-05-29T11:38:08Z

looks like this needs a rebase now as well

jingxiaolu · 2020-06-16T09:55:42Z

rebased again! 🤣

ShemTovYosef · 2020-06-18T14:17:07Z

@tiborvass @hinshun could you check this PR if it can be part of milestone v0.8.0?

ShemTovYosef · 2020-08-26T22:55:32Z

#1339 (review) has not been addressed afaics

hi @tonistiigi could you review it the latest changes?

tonistiigi

LGTM but needs rebase

jingxiaolu · 2020-08-29T12:39:31Z

Code rebased, CI all green, testing passed locally:

buildctl build --frontend=dockerfile.v0 --local dockerfile=. --local context=. --opt hostname=testtest --no-cache --progress=plain
#1 [internal] load .dockerignore
#1 sha256:6d3f11894d56b4821fbfa074552615e9053fda70b4ce7523afc74072abcddf64
#1 transferring context: 2B done
#1 DONE 0.0s

#2 [internal] load build definition from Dockerfile
#2 sha256:0515c9dc4a05825ae382c220ece96bd51f4adff0aae0f6b59846cd22a6326802
#2 transferring dockerfile: 243B done
#2 DONE 0.0s

#3 [internal] load metadata for docker.io/library/alpine:latest
#3 sha256:d4fb25f5b5c00defc20ce26f2efc4e288de8834ed5aa59dff877b495ba88fda6
#3 DONE 6.0s

#4 [1/2] FROM docker.io/library/alpine@sha256:185518070891758909c9f839cf4ca393ee977ac378609f700f60a771a2dfe321
#4 sha256:19a32bc95e3ee69be00ea449bde954c8efe76f03a39961478df70b34d56bf7d0
#4 resolve docker.io/library/alpine@sha256:185518070891758909c9f839cf4ca393ee977ac378609f700f60a771a2dfe321 0.0s done
#4 CACHED

#5 [2/2] RUN echo "Env variable HOSTNAME: $HOSTNAME" &&     echo "hostname: $(hostname)" &&     echo "kernel value: $(cat /proc/sys/kernel/hostname)"  &&     echo "/etc/hosts: $(cat /etc/hosts)"
#5 sha256:5dd50ef1bd946e5adc99fa93e7500e1f164cd2a1ca61e95d1c78729085df1326
#5 0.640 Env variable HOSTNAME: testtest
#5 0.654 hostname: testtest
#5 0.655 kernel value: testtest
#5 0.657 /etc/hosts: 127.0.0.1  localhost testtest
#5 0.657 ::1    localhost ip6-localhost ip6-loopback
#5 DONE 0.7s

Ping @hinews please help to review the test report~

ShemTovYosef · 2020-08-29T17:46:54Z

Hi @jingxiaolu thank you a lot, looks good.
@tonistiigi could you merge this PR?

tonistiigi · 2020-08-30T05:09:49Z

Sorry, I missed that this was lacking integration tests. Please add a test for LLB and also for the frontend opt. You can use https://github.com/moby/buildkit/blob/master/client/client_test.go#L279 as example for LLB test and https://github.com/moby/buildkit/blob/master/frontend/dockerfile/dockerfile_test.go#L350 for frontend test. For the LLB test also check that Hostname can be set on a single Run and it resets after the Run has completed.

client/llb/state.go

tonistiigi · 2020-10-04T07:43:14Z

ping @jingxiaolu

jingxiaolu · 2020-10-10T03:24:38Z

Sorry, I missed that this was lacking integration tests. Please add a test for LLB and also for the frontend opt. You can use https://github.com/moby/buildkit/blob/master/client/client_test.go#L279 as example for LLB test and https://github.com/moby/buildkit/blob/master/frontend/dockerfile/dockerfile_test.go#L350 for frontend test. For the LLB test also check that Hostname can be set on a single Run and it resets after the Run has completed.

I'm trying to write these two integration tests. But I'm not very familiy with buildkit's test framework, is it there any tips on debugging the cases? Or is there anyone can help on implementing the testcases? Many thanks

tonistiigi · 2020-10-10T03:26:41Z

@jingxiaolu PTAL https://github.com/moby/buildkit/blob/master/.github/CONTRIBUTING.md#run-the-unit--and-integration-tests

jingxiaolu · 2020-10-10T03:26:50Z

BTW, CI failed for authorization failed with registry, but I'm not modified such procedure...

CI log:

#26 [binaries-linux 2/2] COPY --from=buildkitd /usr/bin/buildkitd /
#26 DONE 0.1s
#28 exporting to image
#28 exporting layers done
#28 exporting manifest sha256:69f4887216d7da516d3a8f660073319bfcad80eefe58bf761f05cdb3249fde00 0.0s done
#28 exporting config sha256:af9c79173cfece5ebd157b9acc292adf9c8cc92b035f578acfb348d71a83f5f5 0.0s done
#28 pushing layers
#28 pushing layers 0.7s done
#28 ERROR: server message: insufficient_scope: authorization failed
------
 > exporting to image:
------
error: failed to solve: rpc error: code = Unknown desc = server message: insufficient_scope: authorization failed
The command "./hack/login_ci_cache && ./hack/build_ci_first_pass" exited with 1.

Fix: moby#1301 Signed-off-by: Lu Jingxiao <lujingxiao@huawei.com>

tonistiigi · 2020-10-14T06:51:42Z

client/client_test.go

+	defer c.Close()
+
+	hostname := "testtest"
+	st := llb.Image("busybox:latest").With(llb.Hostname(hostname)).Run(llb.Shlexf("sh -c 'echo $HOSTNAME | grep %s'", hostname))


check calling hostname as well

done, PTAL~

tonistiigi · 2020-10-14T06:52:17Z

frontend/dockerfile/dockerfile_test.go

+	f := getFrontend(t, sb)
+	dockerfile := []byte(`
+FROM busybox
+RUN cat /etc/hosts | grep testtest


also check hostname(1) and $HOSTNAME

done, PTAL~

Adding tests to for hostname specifying for image building Cover: moby#1301 Signed-off-by: Lu Jingxiao <lujingxiao@huawei.com>

jingxiaolu · 2020-10-14T09:26:01Z

CI job "Unit Tests & Lint & Vendor & Proto" failed for such, maybe retest is needed?

+buildctl build --progress=plain --frontend=dockerfile.v0 --local context=. --local dockerfile=. --opt filename=./hack/dockerfiles/lint.Dockerfile
error: failed to get status: rpc error: code = Unavailable desc = connection closed
The command "./hack/lint" exited with 1.

+buildctl build --progress=plain --frontend=dockerfile.v0 --opt context=git://github.com/moby/buildkit#refs/pull/1339/merge --opt build-arg:BUILDKIT_CONTEXT_KEEP_GIT_DIR=1 --opt target=integration-tests --output type=docker,name=buildkit-tests,dest=/tmp/docker-iidfile.xKDSr5x6a9 --import-cache type=registry,ref=cicache.buildk.it/moby/buildkit/master:integration-tests --import-cache=type=registry,ref=cicache.buildk.it/moby/buildkit/pr735643079:integration-tests
error: failed to dial gRPC: rpc error: code = Unavailable desc = connection closed
The command "SKIP_INTEGRATION_TESTS=1 ./hack/test integration gateway" exited with 1.

$ ./hack/validate-vendor
error: failed to get status: rpc error: code = Unavailable desc = write tcp 127.0.0.1:53518->127.0.0.1:1234: write: broken pipe
The command "./hack/validate-vendor" exited with 1.

....

tonistiigi · 2020-10-16T06:08:20Z

@jingxiaolu Thanks. Sorry it took so long to get this over the line.

jingxiaolu · 2020-10-16T06:27:25Z

Many thanks to @tonistiigi @AkihiroSuda and @hinews !

Finally we did it!

ShemTovYosef · 2020-10-16T07:32:16Z

I'm so excited to get this functional as part of build.

Many thanks to @jingxiaolu for his effort during almost year on this item and especially for reviewers @tonistiigi @AkihiroSuda @thaJeztah

So, waiting when it will be part of docker-ce to be able to use it.

tuanta · 2021-06-29T04:17:29Z

So, waiting when it will be part of docker-ce to be able to use it.

Is it ready to use now?

ShemTovYosef · 2021-06-29T05:30:20Z

Need to add support in buildx docker/buildx#474

jingxiaolu referenced this pull request in jingxiaolu/docker-ce Jan 22, 2020

debugging for issue 1301

f11dffc

Signed-off-by: l00397676 <lujingxiao@huawei.com>

AkihiroSuda reviewed Jan 23, 2020

View reviewed changes

tonistiigi reviewed Jan 24, 2020

View reviewed changes

jingxiaolu force-pushed the issue1301 branch from f594804 to 739d3c7 Compare February 4, 2020 01:28

thaJeztah reviewed Feb 18, 2020

View reviewed changes

jingxiaolu force-pushed the issue1301 branch 2 times, most recently from 7b16688 to 8e6ba02 Compare April 27, 2020 13:41

jingxiaolu force-pushed the issue1301 branch from 8e6ba02 to 51f0051 Compare April 27, 2020 13:51

AkihiroSuda approved these changes May 1, 2020

View reviewed changes

tonistiigi requested changes May 1, 2020

View reviewed changes

jingxiaolu force-pushed the issue1301 branch 2 times, most recently from 4d486db to 697255b Compare June 16, 2020 08:29

tonistiigi added this to the v0.8.0 milestone Jul 14, 2020

tonistiigi reviewed Aug 27, 2020

View reviewed changes

jingxiaolu force-pushed the issue1301 branch from 697255b to 0e69eba Compare August 29, 2020 12:16

tonistiigi reviewed Aug 30, 2020

View reviewed changes

client/llb/state.go Show resolved Hide resolved

jingxiaolu force-pushed the issue1301 branch from 0e69eba to f1efb2c Compare October 10, 2020 02:56

Add hostname specifying for building

5e7ae23

Fix: moby#1301 Signed-off-by: Lu Jingxiao <lujingxiao@huawei.com>

jingxiaolu force-pushed the issue1301 branch from 8f5df40 to ec1131b Compare October 14, 2020 06:49

tonistiigi reviewed Oct 14, 2020

View reviewed changes

tests: Adding tests for hostname specifying for building

059dac1

Adding tests to for hostname specifying for image building Cover: moby#1301 Signed-off-by: Lu Jingxiao <lujingxiao@huawei.com>

jingxiaolu force-pushed the issue1301 branch from ec1131b to 059dac1 Compare October 14, 2020 07:37

tonistiigi approved these changes Oct 16, 2020

View reviewed changes

tonistiigi merged commit ebd98bc into moby:master Oct 16, 2020

jingxiaolu deleted the issue1301 branch October 16, 2020 06:27

Add hostname specifying for building #1339

Add hostname specifying for building #1339

Conversation

jingxiaolu commented Jan 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShemTovYosef commented Jan 28, 2020 • edited Loading

jingxiaolu commented Feb 4, 2020

AkihiroSuda commented Feb 6, 2020

ShemTovYosef commented Feb 12, 2020

AkihiroSuda commented Feb 12, 2020

ShemTovYosef commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShemTovYosef commented Feb 29, 2020

AlbinoGeek commented Apr 26, 2020

jingxiaolu commented Apr 27, 2020

ShemTovYosef commented May 1, 2020

tonistiigi left a comment

Choose a reason for hiding this comment

thaJeztah commented May 29, 2020

jingxiaolu commented Jun 16, 2020

ShemTovYosef commented Jun 18, 2020

ShemTovYosef commented Aug 26, 2020

tonistiigi left a comment

Choose a reason for hiding this comment

jingxiaolu commented Aug 29, 2020

ShemTovYosef commented Aug 29, 2020

tonistiigi commented Aug 30, 2020

tonistiigi commented Oct 4, 2020

jingxiaolu commented Oct 10, 2020

tonistiigi commented Oct 10, 2020

jingxiaolu commented Oct 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxiaolu commented Oct 14, 2020

tonistiigi commented Oct 16, 2020

jingxiaolu commented Oct 16, 2020

ShemTovYosef commented Oct 16, 2020

tuanta commented Jun 29, 2021

ShemTovYosef commented Jun 29, 2021

ShemTovYosef commented Jan 28, 2020 •

edited

Loading