Fix pruning anon volume created from image config by cpuguy83 · Pull Request #45147 · moby/moby

cpuguy83 · 2023-03-11T22:38:19Z

Volumes created from the image config were not being pruned because the volume service did not think they were anonymous since the code to create passes along a generated name instead of letting the volume service generate it.

This changes the code path to have the volume service generate the name instead of doing it ahead of time.

Related to reports in volumes wont be deleted since update to docker 23 docker/cli#4028

thaJeztah · 2023-03-12T11:20:18Z

Linter is failing;

integration/volume/volume_test.go:6:2: `io/ioutil` is in the denylist (depguard)
	^

thaJeztah · 2023-03-12T11:32:25Z

daemon/create_unix.go

 		}

-		v, err := daemon.volumes.Create(context.TODO(), name, hostConfig.VolumeDriver, volumeopts.WithCreateReference(container.ID))
+		v, err := daemon.volumes.Create(context.TODO(), "", hostConfig.VolumeDriver, volumeopts.WithCreateReference(container.ID))


For a follow-up; given that name is optional, we should change this signature;

move name to options

and/or require "anonymous" to be set as option (if we need to disambiguate "forgot to pass a name" from "must be anonymous, and generate a name for me")

moby/volume/service/service.go

Lines 72 to 76 in 2fa66cf

func (s *VolumesService) Create(ctx context.Context, name, driverName string, options ...opts.CreateOption) (*volumetypes.Volume, error) {

if name == "" {

name = stringid.GenerateRandomID()

options = append(options, opts.WithCreateLabel(AnonymousLabel, ""))

}

thaJeztah · 2023-03-12T15:06:10Z

Did a quick fix-up for the linter error (io/ioutil -> io); https://github.com/moby/moby/compare/96aa0a9cbd41442683c8298e7ce82678ecd3becb..8a6d4d5eae8fb135a2ff7b3522121dbd111220d0

thaJeztah

LGTM (assuming CI is happy)

thaJeztah · 2023-03-12T15:47:07Z

Hm... this one looks related (failing on windows);

INFO: make.ps1 starting at 03/12/2023 15:23:19
Running D:\a\moby\moby\go\src\github.com\docker\docker\integration\build
INFO: Windows Base image is  mcr.microsoft.com/windows/servercore:ltsc2022
...
=== FAIL: github.com/docker/docker/integration/volume TestVolumePruneAnonFromImage (0.28s)
    container.go:52: assertion failed: error is not nil: Error response from daemon: No such image: busybox:withvolume

Other test is probably a flaky;

=== FAIL: github.com/docker/docker/integration/container TestLogs/driver_local/without_tty/only_stderr (1.65s)
    logs_test.go:137: assertion failed: error is not nil: Error response from daemon: failed to create task for container: failed to create shim task: hcs::CreateComputeSystem 5df8dff857afcd0d31dafc15ea17aa0d661b3a4606ba42ed3b2a61fcd96430d6: The parameter is incorrect.: unknown

thaJeztah · 2023-03-12T16:03:49Z

integration/volume/volume_test.go

+	_, err = io.Copy(io.Discard, resp.Body)
+	resp.Body.Close()
+	assert.NilError(t, err)


Does this need the ugly hack we use elsewhere to check if the image was actually built? (I have a feeling there's still other tests that don't check for this, because of the kinda horrible streaming response);

moby/integration/build/build_test.go

Lines 521 to 524 in 2fa66cf

_, err = io.Copy(out, resp.Body)

resp.Body.Close()

assert.NilError(t, err)

assert.Check(t, is.Contains(out.String(), "Successfully built"))

thaJeztah · 2023-03-13T10:17:49Z

Pushed a commit to capture the output;

=== RUN   TestVolumePruneAnonFromImage
    volume_test.go:262: assertion failed: string "{\"stream\":\"Step 1/2 : FROM busybox\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e 6071e3b1a29d\\n\"}\r\n{\"stream\":\"Step 2/2 : VOLUME /foo\"}\r\n{\"stream\":\"\\n\"}\r\n{\"errorDetail\":{\"message\":\"Unrecognised volume spec: invalid volume specification: '/foo'\"},\"error\":\"Unrecognised volume spec: invalid volume specification: '/foo'\"}\r\n" does not contain "Successfully built"
    container.go:52: assertion failed: error is not nil: Error response from daemon: No such image: busybox:withvolume
--- FAIL: TestVolumePruneAnonFromImage (0.28s)
FAIL

So, something looks broken indeed in the test (or does the Windows driver not generate a name now for the anonymous volume?)

Or is it something silly and does VOLUME in the Windows Dockerfile need a drive-letter or C:\foo to exist?

thaJeztah · 2023-03-13T11:12:55Z

This is the error;

Unrecognised volume spec: invalid volume specification: '/foo'

Which comes from;

moby/daemon/create_windows.go

Lines 23 to 26 in 2fa66cf

    
           mp, err := parser.ParseMountRaw(spec, hostConfig.VolumeDriver) 
        
           if err != nil { 
        
           	return fmt.Errorf("Unrecognised volume spec: %v", err) 
        
           }

The ParseMountRaw() there is handled by the WindowsParser;

moby/volume/mounts/parser.go

Line 39 in 2fa66cf

return NewWindowsParser()

Which uses windowsSplitRawSpecRegexp to parse

moby/volume/mounts/windows_parser.go

Line 297 in 2fa66cf

arr, err := p.splitRawSpec(raw, windowsSplitRawSpecRegexp)

This is where things get a bit confusing, as that RegExp looks to be parsing the raw spec according to the <source>:<destination>:<options> format;

moby/volume/mounts/windows_parser.go

Line 86 in 2fa66cf

    
           windowsSplitRawSpecRegexp = regexp.MustCompile(`^` + rxSource + rxDestination + rxMode + `$`)

But in the Dockerfile, this would only be destination (not source), unless the raw value here is already processed (or expected to) include the name of the anonymous volume, which the error indicates it isn't (/foo) 🤔

thaJeztah · 2023-03-13T11:26:35Z

This is where things get a bit confusing, as that RegExp looks to be parsing the raw spec according to the <source>:<destination>:<options> format

Hmm.. I guess that's still "okay" because <source> is optional, and will generate a name once we get further down;

moby/volume/mounts/windows_parser.go

Lines 304 to 310 in 2fa66cf

    
           func (p *windowsParser) parseMount(arr []string, raw, volumeDriver string, convertTargetToBackslash bool, additionalValidators ...mountValidator) (*MountPoint, error) { 
        
           	var spec mount.Mount 
        
           	var mode string 
        
           	switch len(arr) { 
        
           	case 1: 
        
           		// Just a destination path in the container 
        
           		spec.Target = arr[0]

But I guess Windows also needs the same changes as we did for non-windows (i.e., don't generate a name there?)

moby/daemon/create_windows.go

Lines 28 to 31 in 2fa66cf

    
           // If the mountpoint doesn't have a name, generate one. 
        
           if len(mp.Name) == 0 { 
        
           	mp.Name = stringid.GenerateRandomID() 
        
           }

thaJeztah · 2023-03-13T11:30:57Z

Or is it something silly and does VOLUME in the Windows Dockerfile need a drive-letter or C:\foo to exist?

So, yes, it looks like we need something like this;

moby/integration-cli/docker_cli_build_test.go

Lines 88 to 97 in 2fa66cf

    
           func (s *DockerCLIBuildSuite) TestBuildEnvironmentReplacementVolume(c *testing.T) { 
        
           	name := "testbuildenvironmentreplacement" 
        
           	var volumePath string 
        
           	if testEnv.OSType == "windows" { 
        
           		volumePath = "c:/quux" 
        
           	} else { 
        
           		volumePath = "/quux" 
        
           	}

thaJeztah · 2023-03-13T12:23:41Z

moby/daemon/create_windows.go

Lines 23 to 26 in 2fa66cf

    
           mp, err := parser.ParseMountRaw(spec, hostConfig.VolumeDriver) 
        
           if err != nil { 
        
           	return fmt.Errorf("Unrecognised volume spec: %v", err) 
        
           }

Looks like that whole ParseMountRaw() was added as part of a7e686a (Windows: [TP4] Add volume support #16433), and it's unclear why.
After that, it was updated as part of fc7b904 (Add new HostConfig field, Mounts. #22373),
and LCOW refactoring in e89b6e8 (LCOW: Prepare work for bind mounts #34258)

Looking at the original PR (#16433), that PR added ParseMountRaw() for Windows (which parses the whole <source>:<dest>:<options> syntax), whereas the Linux/Unix code only took care of the destination. Given that config.Volumes here is the (image) config (?), shouldn't it only ever have the destination, not a "raw" spec?

thaJeztah · 2023-03-13T13:27:45Z

Almost; looks like Destination is c:\foo (lowercase). ISTR there was some code to always change mounts to lowercase on Windows, so perhaps that's it here?

=== RUN   TestVolumePruneAnonFromImage
    volume_test.go:277: skipping: {Type:volume Name:430fc743010de7cd363f1ea349497520aa234decd45a0c2dba90cdd72f998857 Source:C:\Users\runneradmin\AppData\Local\Temp\moby-root\volumes\430fc743010de7cd363f1ea349497520aa234decd45a0c2dba90cdd72f998857\_data Destination:c:\foo Driver:local Mode: RW:true Propagation:}
    volume_test.go:283: assertion failed: volumeName is ""

thaJeztah · 2023-03-13T15:36:56Z

Looks like CI is at least happy with the latest changes (but it needs cleaning up)

thaJeztah · 2023-03-13T18:33:00Z

@cpuguy83 looks like something broke again 😢

=== RUN   TestVolumePruneAnonFromImage
    container.go:52: assertion failed: error is not nil: Error response from daemon: hcsshim::ExpandScratchSize failed in Win32: The system cannot find the file specified. (0x2)
--- FAIL: TestVolumePruneAnonFromImage (0.21s)

thaJeztah · 2023-03-13T20:15:33Z

https://github.com/moby/moby/compare/master...thaJeztah:fix_volume_anon_from_image_STEP2?expand=1

thaJeztah · 2023-03-13T22:38:11Z

integration/volume/volume_test.go

+
+	var volumeName string
+	for _, m := range inspect.Mounts {
+		if m.Destination != volDest {


Looks like we're back to this part not finding the volume 😞 (I suspect here it expects it to be c:\foo)

cpuguy83 · 2023-03-14T00:55:30Z

finally it is green.

thaJeztah · 2023-03-14T10:00:08Z

Oh! My PR was merged, and looks like a (probably minor) merge conflict; I'll do a quick rebase in a bit

Volumes created from the image config were not being pruned because the volume service did not think they were anonymous since the code to create passes along a generated name instead of letting the volume service generate it. This changes the code path to have the volume service generate the name instead of doing it ahead of time. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

thaJeztah

still green; LGTM

cpuguy83 marked this pull request as ready for review March 11, 2023 22:38

cpuguy83 requested a review from neersighted March 11, 2023 22:38

cpuguy83 mentioned this pull request Mar 11, 2023

volumes wont be deleted since update to docker 23 docker/cli#4028

Closed

cpuguy83 force-pushed the fix_volume_anon_from_image branch from 594ee61 to 96aa0a9 Compare March 11, 2023 22:39

neersighted approved these changes Mar 11, 2023

View reviewed changes

neersighted added status/2-code-review kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. process/cherry-pick area/volumes Volumes labels Mar 12, 2023

neersighted added this to the v-next milestone Mar 12, 2023

thaJeztah added kind/bugfix PR's that fix bugs and removed kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. labels Mar 12, 2023

thaJeztah reviewed Mar 12, 2023

View reviewed changes

thaJeztah force-pushed the fix_volume_anon_from_image branch from 96aa0a9 to 8a6d4d5 Compare March 12, 2023 15:05

thaJeztah approved these changes Mar 12, 2023

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Mar 12, 2023

thaJeztah added status/2-code-review and removed status/4-merge labels Mar 12, 2023

thaJeztah reviewed Mar 12, 2023

View reviewed changes

thaJeztah force-pushed the fix_volume_anon_from_image branch from cca29d4 to 2a6d7d0 Compare March 13, 2023 11:35

thaJeztah force-pushed the fix_volume_anon_from_image branch from a4d643f to 8b2bcb7 Compare March 13, 2023 12:41

thaJeztah force-pushed the fix_volume_anon_from_image branch from 8b2bcb7 to bb5cbe4 Compare March 13, 2023 13:59

cpuguy83 force-pushed the fix_volume_anon_from_image branch 3 times, most recently from e1f7c7e to 7810785 Compare March 13, 2023 17:38

cpuguy83 force-pushed the fix_volume_anon_from_image branch from 7810785 to 67a231f Compare March 13, 2023 19:03

cpuguy83 force-pushed the fix_volume_anon_from_image branch 2 times, most recently from bdc7126 to 977661c Compare March 13, 2023 21:04

thaJeztah reviewed Mar 13, 2023

View reviewed changes

cpuguy83 force-pushed the fix_volume_anon_from_image branch 3 times, most recently from 82df2c6 to b1a6e9b Compare March 13, 2023 23:38

thaJeztah force-pushed the fix_volume_anon_from_image branch from b1a6e9b to 146df5f Compare March 14, 2023 10:07

thaJeztah approved these changes Mar 14, 2023

View reviewed changes

thaJeztah merged commit ee17ecb into moby:master Mar 14, 2023

thaJeztah mentioned this pull request Mar 14, 2023

[23.0 backport] Fix pruning anon volume created from image config #45159

Merged

cpuguy83 deleted the fix_volume_anon_from_image branch March 14, 2023 15:10

thaJeztah mentioned this pull request Mar 14, 2023

integration: fix some duplicate imports #45161

Merged

neersighted added process/cherry-picked impact/changelog and removed process/cherry-pick labels Mar 16, 2023

neersighted mentioned this pull request Jul 20, 2023

[BUG] Compose-created anonymous volumes are not marked as anonymous docker/compose#10833

Closed

	func (s VolumesService) Create(ctx context.Context, name, driverName string, options ...opts.CreateOption) (volumetypes.Volume, error) {
	if name == "" {
	name = stringid.GenerateRandomID()
	options = append(options, opts.WithCreateLabel(AnonymousLabel, ""))
	}

	_, err = io.Copy(out, resp.Body)
	resp.Body.Close()
	assert.NilError(t, err)
	assert.Check(t, is.Contains(out.String(), "Successfully built"))

Conversation

cpuguy83 commented Mar 11, 2023 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 12, 2023

Uh oh!

thaJeztah Mar 12, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 12, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 12, 2023

Uh oh!

thaJeztah Mar 12, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 13, 2023

Uh oh!

thaJeztah commented Mar 13, 2023

Uh oh!

thaJeztah commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 13, 2023

Uh oh!

thaJeztah commented Mar 13, 2023

Uh oh!

thaJeztah commented Mar 13, 2023

Uh oh!

thaJeztah commented Mar 13, 2023

Uh oh!

thaJeztah commented Mar 13, 2023

Uh oh!

thaJeztah commented Mar 13, 2023

Uh oh!

thaJeztah Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Mar 14, 2023

Uh oh!

thaJeztah commented Mar 14, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cpuguy83 commented Mar 11, 2023 •

edited by thaJeztah

Loading

thaJeztah commented Mar 13, 2023 •

edited

Loading