Unless I'm missing something this seems like what we'd want.

Forgot about this one; yes, looks like all the extra checks are not needed

hc.StorageOpt can't be nil here?

Had to double-check, but yes, it can be; https://play.golang.org/p/bD7H35U2wdZ

package main

import (
	"fmt"
)

type HostConfig struct {
	StorageOpt map[string]string `json:",omitempty"` // Storage driver options per container.
}

func main() {
	hc := &HostConfig{}

	if hc.StorageOpt["size"] == "" {
		if hc.StorageOpt == nil {
			fmt.Println("StorageOpt is nil")
		}

		hc.StorageOpt["size"] = "127GB"
	}
}

Running that will panic;

StorageOpt is nil
panic: assignment to entry in nil map

goroutine 1 [running]:
main.main()
	/tmp/sandbox898184674/prog.go:19 +0xa8

While you're at it, you might want to look at the definition of isWCOW on line 451. Currently, it's true if you run docker build . --platform windows but false if you run docker build .. I was just now looking into whether it should be inverted to be

isWCOW := runtime.GOOS == "windows" && (b.platform == nil || b.platform.OS == "windows")

Edit: Confirmed that with the above change, the "127GB overrides storage-opts" now replicates both with and without --platform windows.

Oh! Comments crossed with the last comment I left on #37352 (comment)

Ah, interesting; sounds like more fun because of the LCOW paths (I guess it doesn't set WCOW if it doesn't know if the image is gonna be Linux or Windows?) haven't checked all paths

Should we remove the isWCOW there? It would be a somewhat closer match to "linux" (where by default size is only restricted by physical disk space)and replace with if runtime.GOOS == "windows"

WDYT?

LCOW has its own option, lcow.sandboxsize, so size would be ignored by LCOW. We can't validate that because the config is shared between WCOW and LCOW. -_-

I would expect that if runtime.GOOS == "windows", then LCOW would only happen if b.platform.OS == "linux", which I assume is what this check was intended to do.

Or perhaps at the time, b.platform would not reasonably be nil, but now we have things that check platform == nil and fallback to runtime.GOOS, e.g., createCopyInstruction in builder/dockerfile/copy.go.

Anyway, given that, a simple check for runtime.GOOS would be okay, since we know the only graphdrivers that run on Windows are lcow and windowsfilter.

LCOW is also a 20GB default, it might make sense that we set both size= and lcow.sandboxsize= to 127GB here.

How can hc.StorageOpt ever be set here? hc is defined 14 lines earlier, and doesn't set the StorageOpt member, so won't it always be nil here?

I think maybe this fix is in the wrong place. The values set here are merged with the daemon's config values later, in create in daemon/create.go, which is called with the output of this method. See line 190 of daemon/create.go. There, the logic is for the StorageOpts created here to override the daemon config, but it should be the other way 'round since the StorageOpts here are defaults.

🤦 You are absolutely right. Thinking how I came to this change at the time, as it's a while, but looks like the daemon config is merged in

🤔

Alternatively, move this entire chunk of logic to set a default sandbox size, into daemon/create.go after the merge. It makes now different now, but if we ever find a way to get docker build to pass storage opts, then we'd want those to win over the daemon.json and the default 127GB sandbox.

Perhaps we should hold this change until after we cut the 20.10 branch, and when we start removing the LCOW paths (I'd also personally be ok with increasing the size to a higher value if 127GB is still too low for some scenarios); IIRC, setting a higher value shouldn't affect performance when creating the container (and would only make a difference if someone actually used the given amount of space)

The advantage of moving this default-size logic into moby/daemon/create.go is that I think we know if it's LCOW or WCOW by then, because we have os set with reference to the image.

On the other hand, taking it out of builder/dockerfile means that we can't as-easily reuse the number for the copy mount, as part of fixing #37352.

Easiest approach might be to leave builder/dockerfile/internals.go as-is, or with only the isWCOW test fixed, and just change daemon/create.go from

 		if _, ok := ctr.HostConfig.StorageOpt[opt[0]]; !ok { 
 			ctr.HostConfig.StorageOpt[opt[0]] = opt[1] 
 		} 

to just

 		ctr.HostConfig.StorageOpt[opt[0]] = opt[1] 

i.e.. daemon.configStore.GraphOptions always overrides ctr.HostConfig.StorageOpt, and today the only thing that this affects is size=127GB from builder/dockerfile/internals.go.

Ignore that, I'm dumb. Running containers will hit this, not just builders, and that would blat docker run --storage-opts.