Check the validity of the platforms #854

chendave · 2019-03-05T08:22:47Z

Signed-off-by: Dave Chen dave.chen@arm.com

AkihiroSuda · 2019-03-07T05:06:02Z

chendave · 2019-03-07T10:52:11Z

Cause we cannot make it without the "-F" options is set, we'd better to show the warning somewhere to give end-user an hint, thanks for the review.

tonistiigi · 2019-03-07T18:02:41Z

In this place, I don't think there is any indication that the user is using qemu or if it is configured at all or configured incorrectly.

We could instead modify the main qemu detection in #840 to look for the enabled configuration and warn or error if F is missing in the options. If binfmt_misc is not mounted but the binary check in #840 still works then it should be safe to assume that F was configured properly.

tonistiigi · 2019-03-07T19:34:59Z

Another way would be to run (a second pass of) #840 with chroot that should also detect if F was checked or not.

chendave · 2019-03-08T08:55:20Z

okay, I will take a look, thanks!

chendave · 2019-03-09T11:41:14Z

@tonistiigi, I did a test locally, the check will pass regardless the binfmt_misc is mounted or not, but as long as the "-F" flag has been set.

tonistiigi · 2019-03-11T06:53:04Z

With #840 the config of secondary platforms is not needed anymore. If it is used, it means manual override and probably doesn't need hints. Also, in the place where you have added it, it only works for flags and not for configuration.

We could do either #854 (comment) or #854 (comment) though if you want to hint the need for -F.

chendave · 2019-03-11T08:34:56Z

okay, if the "-F" is not configured well, secondary platforms will not pass the check, and then #840 will not configure the platform as a available platform, I think this is good so far, the secondary platforms are not added so that the warn per my understanding is needn't.

But what I want to say in this patch is when the platform is added by flags manually, it can be show by the CLI, and those platforms that are added seems okay, but actually, if the "-F" is not set configured well, those secondary platforms won't work, this is why I want to show the warning message here, those secondary platforms are added but they might not work though.

Based on those warning message, he/she might consider to set the option "-F" and then those manually added platform should work.

what do you think? thanks!

chendave · 2019-03-11T08:45:28Z

and someone like me not always depend on what's the default configuration provides but only concern the specific platform and this is case why the flag override is need here.

tonistiigi

At least make it so it works with manual config as well, not as an exception for flags.

Note that #840 does not actually guarantee -F afaik . -F is needed for chrooted programs but the detection currently does not check that.

cmd/buildkitd/main_containerd_worker.go

chendave · 2019-03-12T02:00:55Z

At least make it so it works with manual config as well, not as an exception for flags.

May I know what do you mean by "make it works with manual config"? Could you pls elaborate a little? My understanding is #840 will provide the default available platforms, while the flag here is to override the default and is the manual configuration, am I wrong?

Note that #840 does not actually guarantee -F afaik . -F is needed for chrooted programs but the detection currently does not check that.

I tried, if the "-F" option is not set, the #840 check while fail, while "-F" option is set the #840 will success, so I think #840 check it enough.

BTW, I am still new to this project, pls help to guide me if there is any misunderstanding from me, thanks!

tonistiigi · 2019-03-12T17:32:03Z

May I know what do you mean by "make it works with manual config"

GlobalStringSlice only returns values from the flags, there is also a config file for buildkitd that has the same options.

I tried, if the "-F" option is not set, the #840 check while fail, while "-F" option is set the #840 will success, so I think #840 check it enough.

I haven't tested the flag myself but it also depends on if you are running the buildkitd from host or from a different mount namespace.

chendave · 2019-03-14T10:27:53Z

@tonistiigi , you are right! this patch doesn't check the option of "-F", I still don't have a good way to check that, cause this need a secondary filesystem so that chroot can have a try there.

I am thinking of providing a similar tool in buildkit to do something like below, but this will be a long term effort.

docker run --rm --privileged multiarch/qemu-user-static:register --reset (set, clean etc.)

tonistiigi · 2019-03-15T03:46:31Z

If you want to go with chroot path then as the test binaries are static you could just specify the chroot to their parent directory on invoking. That should be enough. You could also run with and without chroot and provide a better error if you can detect that missing F is the issue.

For the validation of currently set ones, instead of repeating SupportedPlatforms call it and compare the 2 sets. You may also need to do normalization of the values (or just validate after they have been parsed). The error/warn should contain the name of the wrongly configured platform.

chendave · 2019-03-15T10:07:38Z

Thank you, I will address both of them later.

chendave · 2019-03-19T01:30:34Z

let's retest this.

chendave · 2019-03-20T01:38:10Z

@tonistiigi , hi, what do you think about this? thanks!

tonistiigi · 2019-03-21T05:08:21Z

util/binfmt_misc/check.go

-	return exec.Command(pp).Run()
+	output, err := exec.Command(pp).CombinedOutput()
+	if err == nil {
+		cmd := exec.Command("chroot", ".", "/check")


set chroot in SysProcAttr, don't call the binary

tonistiigi · 2019-03-21T05:10:13Z

cmd/buildkitd/main.go

 	}
 	if cfg.Workers.Containerd.Platforms == nil {
 		cfg.Workers.Containerd.Platforms = binfmt_misc.SupportedPlatforms()
+	} else {


just run it once per each worker instead of looking for else/flags-set case. if detection was automatic validation is just a no-op.

tonistiigi · 2019-03-21T05:17:08Z

util/binfmt_misc/check.go

@@ -10,30 +10,36 @@ import (
 	"path/filepath"
 )

-func check(bin string) error {
+func check(bin string) (string, error) {


just return error. You can access stderr from ExitError or you can wrap the error with extra message if you like.

tonistiigi · 2019-03-21T05:17:32Z

util/binfmt_misc/detect.go

-			arr = append(arr, p)
+		if p := "linux/amd64"; def != p {
+			_, err := amd64Supported()
+			if err == nil {


nit: put these check on a single line

tonistiigi · 2019-03-21T05:19:18Z

util/binfmt_misc/detect.go

+//Only show the warning when the platforms are maually specified, show the message for the platforms
+//finding provided by `SupportedPlatforms` is a little annoying, no platform specifed but the warning
+//message is shown might confuse end users.
+func printErr(p string, output string, err error) {


rename to printPlatfromWarning

tonistiigi · 2019-03-21T05:19:59Z

util/binfmt_misc/detect.go

+//Here, the func just validate the platforms and show warning message if there is,
+//the end user could fix the issue based on those warning, and thus no need to drop
+//the platfrom from the candidates.
+func ValidatePlatforms(pfs []string) {


rename WarnIfUnsupported

tonistiigi · 2019-03-21T05:20:34Z

util/binfmt_misc/detect.go

+//message is shown might confuse end users.
+func printErr(p string, output string, err error) {
+	if strings.Contains(err.Error(), "exec format error") {
+		logrus.Warnf("platform %s cannot pass the validation, kernel support for miscellaneous bianry may have not enabled.", p)


tonistiigi · 2019-03-21T05:20:47Z

util/binfmt_misc/detect.go

+func printErr(p string, output string, err error) {
+	if strings.Contains(err.Error(), "exec format error") {
+		logrus.Warnf("platform %s cannot pass the validation, kernel support for miscellaneous bianry may have not enabled.", p)
+	} else if strings.Contains(output, "No such file or director") {


directory

chendave · 2019-03-21T10:34:16Z

@tonistiigi , thanks for your review! hope it is better than before.

tonistiigi · 2019-03-22T03:02:14Z

util/binfmt_misc/detect.go

@@ -14,19 +15,56 @@ func SupportedPlatforms() []string {
 	once.Do(func() {
 		def := platforms.DefaultString()
 		arr = append(arr, def)
-
-		if p := "linux/amd64"; def != p && amd64Supported() {
+		if p := "linux/amd64"; def != p && (amd64Supported() == nil) {


parenthesis are not needed in these checks

agreed, but parenthesis here make it easier to read, anyway, I will remove it.

tonistiigi · 2019-03-22T03:03:06Z

util/binfmt_misc/detect.go

+	for _, p := range pfs {
+		if p != def {
+			if p == "linux/amd64" {
+				err := amd64Supported()


nit: single-line these:

if err := amd64Supported(); err != nil { printPlatfromWarning(p, err) }

tonistiigi · 2019-03-22T03:05:16Z

util/binfmt_misc/check.go

-	return exec.Command(pp).Run()
+	err = exec.Command(pp).Run()
+	if err == nil {
+		cmd := exec.Command("/check")


what's the benefit of running it twice and not only the chroot version?

Nice catch! no benefit at all, new RP is one the way.

tonistiigi · 2019-03-22T17:30:53Z

util/binfmt_misc/check.go

@@ -35,5 +36,12 @@ func check(bin string) error {
 	}
 	f.Close()

-	return exec.Command(pp).Run()
+	if err == nil {


this err == nil check is meaningless

tonistiigi

LGTM after comment fix

tonistiigi · 2019-03-24T03:08:21Z

util/binfmt_misc/detect.go

+//Here, the func just validate the platforms and show warning message if there is,
+//the end user could fix the issue based on those warning, and thus no need to drop
+//the platfrom from the candidates.
+func WarnIfUnsupported(pfs []string) {


(Sorry, didn't notice this before.) The godoc convention is that a function comment needs to start with the function name. Eg. // WarnIfUnsupported validates the platform and shows warning ....

np, you left many good comments! really learnt a lot from those comments, thanks!

platforms can still be added but some warning message will be emitted if the platform cannot pass the validity check. Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave · 2019-03-24T14:25:20Z

retest

chendave · 2019-03-24T14:52:51Z

weird, the CI is not much stable?

chendave force-pushed the hint branch from 0ed3099 to 715747d Compare March 9, 2019 11:36

chendave closed this Mar 10, 2019

chendave reopened this Mar 10, 2019

chendave force-pushed the hint branch from 715747d to 027ed68 Compare March 10, 2019 12:39

tonistiigi reviewed Mar 11, 2019

View reviewed changes

cmd/buildkitd/main_containerd_worker.go Outdated Show resolved Hide resolved

chendave force-pushed the hint branch 2 times, most recently from ca781b0 to d7a155d Compare March 12, 2019 05:23

chendave force-pushed the hint branch from d7a155d to a53fa2d Compare March 14, 2019 10:23

chendave changed the title ~~Show the hints when buildkit is on multi-arch mode~~ Check the validity of the platforms Mar 14, 2019

chendave closed this Mar 14, 2019

chendave reopened this Mar 14, 2019

chendave closed this Mar 14, 2019

chendave reopened this Mar 14, 2019

chendave force-pushed the hint branch from a53fa2d to a8ef2cc Compare March 18, 2019 10:43

chendave closed this Mar 19, 2019

chendave reopened this Mar 19, 2019

chendave closed this Mar 19, 2019

chendave reopened this Mar 19, 2019

chendave force-pushed the hint branch from a8ef2cc to 6347a70 Compare March 19, 2019 02:49

tonistiigi reviewed Mar 21, 2019

View reviewed changes

chendave force-pushed the hint branch from 6347a70 to ee6f12c Compare March 21, 2019 10:32

tonistiigi reviewed Mar 22, 2019

View reviewed changes

chendave force-pushed the hint branch from ee6f12c to 2ab70fe Compare March 22, 2019 05:45

tonistiigi reviewed Mar 22, 2019

View reviewed changes

chendave force-pushed the hint branch from 2ab70fe to 96ec4d8 Compare March 23, 2019 03:47

tonistiigi reviewed Mar 24, 2019

View reviewed changes

Check the validity of the platforms

aa31ff0

platforms can still be added but some warning message will be emitted if the platform cannot pass the validity check. Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave force-pushed the hint branch from 96ec4d8 to aa31ff0 Compare March 24, 2019 04:11

tonistiigi approved these changes Mar 24, 2019

View reviewed changes

chendave closed this Mar 24, 2019

chendave reopened this Mar 24, 2019

chendave closed this Mar 24, 2019

chendave reopened this Mar 24, 2019

tonistiigi merged commit 2453e6f into moby:master Mar 24, 2019

chendave deleted the hint branch March 25, 2019 01:42

Check the validity of the platforms #854

Check the validity of the platforms #854

Conversation

chendave commented Mar 5, 2019

AkihiroSuda commented Mar 7, 2019

chendave commented Mar 7, 2019

tonistiigi commented Mar 7, 2019

tonistiigi commented Mar 7, 2019

chendave commented Mar 8, 2019

chendave commented Mar 9, 2019

tonistiigi commented Mar 11, 2019

chendave commented Mar 11, 2019

chendave commented Mar 11, 2019

tonistiigi left a comment

Choose a reason for hiding this comment

chendave commented Mar 12, 2019

tonistiigi commented Mar 12, 2019

chendave commented Mar 14, 2019

tonistiigi commented Mar 15, 2019

chendave commented Mar 15, 2019

chendave commented Mar 19, 2019

chendave commented Mar 20, 2019

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

chendave commented Mar 21, 2019

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

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chendave commented Mar 24, 2019

chendave commented Mar 24, 2019