#Thread about `WithMountedX` and

1 messages · Page 1 of 1 (latest)

normal lily
#

cc @prime bobcat

#

silently mentioning @glossy vessel and @lament badge

solar trail
#

Yes it’s working as designed. Of course that doesn’t mean the design can’t be improved 😁

deft mural
#

Yep, if you want to actually publish/export /tmp/foo you want WithDirectory, not WithMountedDirectory, so that it actually copies it into the image

normal lily
#

gotcha, makes sense. I had the idea that Export didn't work for mounted dirs as well as volumes. I know for sure it doesn't work for cache volumes, wasn't aware it worked for mounted dirs

deft mural
#

yup yup - it's used pretty often to make something available to the build process without leaving it in the published image. Though there are other ways of doing that too, e.g. just doing WithFile with the result of some separate build, and maybe that's clearer

solar trail
#

Personally I consider export not working with cache volumes to be the bug. If you can get a Directory, the full Directory API should apply to it.

prime bobcat
lament badge
prime bobcat
#

I am more confused after reading that 😅

deft mural
lament badge
lament badge
# solar trail Yes it’s working as designed. Of course that doesn’t mean the design can’t be im...

👀
ok i have a suggestion around this 🙂 could we limit WithMountedDirectory to mount a read-only copy of the directory? (or just not allow propagation between steps, so it resets to the previous option)

  1. this then lines up exactly with how --mount type=bind works today in dockerfiles
  2. the caching issue in https://github.com/dagger/dagger/issues/6421 entirely goes away (side note, i'm trying to follow up on what the upstream issue that blocks this is)
  3. there becomes a clear difference to document about why these are different - WithMountedDirectory essentially becomes the much more efficient option
#

to me, it's a bit weird that you can write to the mounted directory anyways - most of the use cases i can think of, you don't want to do this anyways
if you do need to, WithDirectory feels like the much clearer way to do it

#

maybe there's some context that i'm missing around why we want the mounting behavior, but at the moment, it really feels like there are more (really complicated) papercuts around the difference between mounting/non-mounting than there are benefits to having both at all

cursive terrace
#

@lament badge Example footgun for you:

I had multiple assets to copy into a container:

      // 'services/**/package.json',
      // '.yarn',
      // '.yarnrc.yml',
      // 'package.json',
      // 'config/**/*.json',
      // 'babel',
      // 'test-utils',
      // 'npm-packages-offline-cache',
      // 'yarn.lock',

My lazy way of doing so is something like this (code incomplete, but illustrates the point):

const dir = client.host().directory('.', {
      include: [
        'services/**/*.json',
        '.yarn',
        '.yarnrc.yml',
        'package.json',
         'npm-packages-offline-cache', 
        'config/**/*.json',
        'yarn.lock',
         // bla bla 
      ],
    });

await client.container().from('node').withEntryPoint(['/bin/bash', '-c']).withWorkDir('/root/foo').withMountedDirectory("/root/foo", dir).withExec('yarn install') // bla bla
  • The node_modules are written into /root/foo
  • Publish the image, no node modules!
  • Crash an app in production.

I can fix this by preparing a pipeline that sets certain folders/files as being a mount. To be honest it's a bit tedious to have to do that, but it's better than having it fail in production.

My gut reaction is a ➕ to match the behavior of buildkit.

mossy summit
solar trail
#

Just double checking, is the current behavior not equivalent to --type=bind,rw=1?

lament badge
#

no, data propagates through - if i write to a mounted directory, it'll continue to be there in the next WithExec step

#

this isn't the case with dockerfiles

deft mural
#

I use WithMountedDirectory in cases where I either don't intend to publish an image and just don't want to worry about the perf penalty of a full copy with WithDirectory, and/or intentionally don't want the mounted contents in the published image[^1]. I wouldn't feel great about changing it to align with less flexible tools; the copy-on-write semantics and automatic propagation are pretty nice to work with in my experience. Read-only mounts usually just annoy me.

@cursive terrace Isn't that fixed by "just" using WithDirectory? Is the issue that if you discover WithMountedDirectory without knowing about WithDirectory you might not notice the difference? What if we renamed it to WithShallowCopiedDirectory or something? (strawman)

[^1]: caveat: arguably you should use that content in a separate pipeline and copy only the output of that pipeline into the target container, but that can sometimes require a bit of a refactor; it's nice to have options

cursive terrace
#

Isn't that fixed by "just" using WithDirectory?

No because I don't want to bloat the published image with assets that I don't want to ship.

Is the issue that if you discover WithMountedDirectory without knowing about WithDirectory you might not notice the difference? What if we renamed it to WithShallowCopiedDirectory or something? (strawman)

I'm not following sorry, can you be more specific?

lament badge
#

perf penalty of a full copy
Is there a perf penalty? I think this should be pretty efficient, right? It should use buildkit's merge op, which is already using overlayfs just like a mount would (mounts are bind mounts, right)

deft mural
lament badge
deft mural
cursive terrace
#

Yes, if it's equivalent to RUN --mount=type=bind then it should be read-only and fail. That would have allowed me to fix the problem sooner before it hits production.

When you use buildkit under the hood, it sets certain expectations for me as to how Dagger works. But when I come across footguns like this it just leaves me feeling frustrated, as it puts into question if the concepts of buildkit are actually transferable to Dagger or not.

If you do still want this feature, it should be an opt-in, not a default. Currently the setup is opaque and confusing.

#

FWIW, I didn't actually understand the difference between withMountedDirectory and withDirectory until recently. I've been using Dagger for 2 years...

deft mural
#

Makes sense - what I was getting at with the "Is the issue that ..." question was that it is opt-in since it's the longer-named withMountedDirectory, so if folks are interpreting it as "the normal way to add directories to a container" that's an issue with the API naming and/or documentation. Sounds like that's the case.

I wouldn't really say we're trying to 100% accurately reflect Buildkit behavior; it's an implementation detail, our goal is to provide a more expressive API on top of it. There are pros and cons to this of course. One big pro of that is that we can implement optimizations like the one @lament badge mentioned where WithDirectory under the hood uses a MergeOp to efficiently copy instead of a llb.Copy. We were able to implement that optimization without changing the API.

cursive terrace
#

What about this: can you make it so that Dagger defaults to readonly, and you optin to readwrite? Because it doesn't come across in that name withMountedDirectory that it is an opt-in to a read write mount.

Or alternatively, no defaults allowed! You must explicity decide what mount type you want e.g.

withMountedDirectory(.... , {
type: "readwrite"
})

#

That way you communicate what it's capable of.

deft mural
#

What are the chances you would have just passed 'read-write' anyway though, knowing that you need to write to it 😛

#

Or do you think the act of passing that arg would have cued you to wonder whether it's the right thing to do?

cursive terrace
#

The latter.

solar trail
#

I don't have a strong opinion on how to address this, but to clarify priorities:

  1. It's important for the API to be intuitive and consistent. If developers are confused that is bad and we should fix it.

  2. It's not as important for familiarity with buildkit to translate perfectly to familiarity with Dagger (as @deft mural mentioned).

In your case you're caught in between the two, which can certainly be frustrating.

I just wanted to be transparent and set expectations.

normal lily
normal lily
cursive terrace
#

I changed the pipeline to explicity define certain folders/files as being mounts as opposed to the entirety of the folder.