clarification for namespacing of cache volumes | Dagger | Page 1

past mason Oct 7, 2024, 5:04 PM

#

I made changes similar to how "SetSecret" is handled, and now when I try to get cache volume id in two separate dagger calls across two different modules (using dag.CacheVolume("foo").ID(ctx)), I get different ID's (as expected)

#

(I tested ^^ using hack/with-dev dagger .......)

#

but when I am adding an integration test for it, whose pseudo code is:

c := connect(ctx, t)
cd /work/foo
id_foo := dagger call ....

cd /work/bar
id_bar := dagger call ....

the id_foo and id_bar are turning out to be same, unless I use a new connect call for bar.

#

I was expecting it to have different ID's as its still two different dagger calls. any pointers, what I might be doing incorrectly here

glacial owl Oct 7, 2024, 5:19 PM

#

Is the implementation just taking the cache volume key (i.e. foo in dag.CacheVolume("foo")) and mixing it in with the module source digest of the module that created the cache volume to get the "real underlying" key? Or is it more than that?

past mason Oct 7, 2024, 5:22 PM

#

https://github.com/dagger/dagger/commit/e27f85fc846e8492bb41e4e422aba2806c1cadb3#diff-da47bfee7f55e4f86b54a332c519d0d37bef79f1b869d027735afff292d7e1d4R59

#

^^ its essentially a copy of how secrets handle it. using "key" + "accessor"

glacial owl Oct 7, 2024, 5:58 PM

#

I see, I suspect the problem is that the cacheVolume2 field is not marked impure, so it gets cached between different clients in the same session (i.e. sharing the same client created by connect).

Overall though, while this sort of gets into some design questions, I think we should actually do something simpler here since the constraints are a bit different than secrets+sockets. Secrets+sockets need isolation so they aren't globally accessible and also need very explicit passing around between clients because they aren't content-defined.

The pattern you are using here results in every client (including module clients) getting their own separate cache volume when that client refers to a given key. E.g. if Module A creates a container with a mounted cache volume using key foo and then passes that container to a different Module B where it gets evaluated, the foo cache volume will actually be one private to Module B, not the one Module A tried to use.

IMO that's not what we want. I think the expected behavior in that scenario is that the foo cache volume of Module A is used after its mounted into the container, even in Module B is the one that actually evaluates the container.

I think all we need to do is take the key arg and mix it in with the module source digest (i.e. the one used here https://github.com/sipsma/dagger/blob/7527e4bf607f1206afc64565fefdf78737fcef9a/core/client_resource.go#L20-L20) into one digest and then use that as the "real" underlying cache volume key.

That will result in cache volumes being namespaced on "creation" in a module but retain their original identity when passed to other modules (if that makes sense, let me know if not)

GitHub

dagger/core/client_resource.go at 7527e4bf607f1206afc64565fefdf7873...

A portable devkit for CI/CD pipelines. Contribute to sipsma/dagger development by creating an account on GitHub.

#

Also sorry if there was an issue or comment or something somewhere where I suggested using the secrets/sockets pattern for this; I feel like that was an idea in my head at some point but thinking through it more now it doesn't seem called for in this case

past mason Oct 7, 2024, 6:03 PM

#

It does make sense and that is one of the testcase i have on my list to verify.

#

I will try this out further tomorrow and get back to you if i have additional questions

past mason Oct 8, 2024, 9:25 AM

#

I tried adding the module source digest to the key, but still same ID's are returned for different modules with same cache-volume-name.

return core.NewCache(scopeDigest.String() + args.Key), nil

The requirements from the original ticket are:

When a cache volume is loaded by key (cacheVolume(key: String!)), the key is always namespaced by module address. There is no way to bypass this.

I believe adding the module digest takes care of this requirement.

A cache volume ID can be used by any module. But a module cannot guess the ID of another module's cache volume: it needs to be passed explicitly.

The cache volume ID is turning out to be same for multiple modules calls (using same engine). I am not quite sure if this is expected.

Ideally, cache volume IDs do NOT become sensitive values to be scrubbed from screenshots and logs (because ie. they are not reusable across sessions)

For this, we need to ensure the volume ids are not predictable (or derivable).

glacial owl Oct 8, 2024, 5:09 PM

#

I tried adding the module source digest to the key, but still same ID's are returned for different modules with same cache-volume-name.
Is this fixed by making the cache volume API impure? e.g.

diff --git a/core/schema/cache.go b/core/schema/cache.go
index 2d634ee8a..193052b27 100644
--- a/core/schema/cache.go
+++ b/core/schema/cache.go
@@ -21,6 +21,7 @@ func (s *cacheSchema) Install() {
        dagql.Fields[*core.Query]{
                dagql.Func("cacheVolume", s.cacheVolume).
                        Doc("Constructs a cache volume for a given cache key.").
+                       Impure("cache volumes are namespaced based on the caller").
                        ArgDoc("key", `A string identifier to target this cache volume (e.g., "modules-cache").`),
        }.Install(s.srv)

(except applied to your tmp cacheVolume2 api in your case)

#

Just want to make sure that fixes the problem before getting to the rest of the points

past mason Oct 8, 2024, 5:21 PM

#

yeah, I think Impure did not help here. Although the fieldSpec says its tainted, it still end up generating same ID. (thought I have tried quite a few combo today, so may be misremembering some thing here).

#

What I am trying now is that when user calls CacheVolume, in schema/cache.go, I make a select call with modified key. what I am thinking should happen is that now the generated id should consider the digest + key and therefore should be unique per module

#

i am still making changes for ^^, so not sure if this would work or not. what do you think?

past mason Oct 8, 2024, 5:58 PM

#

with an additional select:

foo -> https://dagger.cloud/rajatjindal/traces/3b3512c0f0db9aec8573b4dfed412f26
bar -> https://dagger.cloud/rajatjindal/traces/f49e73c419e89aec04aa5b6a0db75d98

both return same CacheVolumeID. (I do have Impure on both apis)

#

without an additional select:

foo -> https://dagger.cloud/rajatjindal/traces/10551b3b41758a6cb3ed2f69491de610
bar -> https://dagger.cloud/rajatjindal/traces/4ff8f9a58ffc427bb6dadfeaa2650096

both return same CacheVolumeID. (still have Impure on both apis)

#

I did verify in cachedSelect, that tainted is true.

glacial owl Oct 8, 2024, 6:03 PM

#

Can you push the code you have so far somewhere again? I think I just need to look at it in detail and try it locally to figure out what's going wrong, too many little details everywhere to debug-via-conversation 😄

past mason Oct 8, 2024, 6:03 PM

#

yeah, I understand :). I will push the code with some docs

#

I have pushed changes here: https://github.com/rajatjindal/dagger/tree/cache-volume-id-issue
and the sample module i am using for test is here: https://github.com/rajatjindal/dagger-same-cache-volume-id

#

I have added some doc comments to explain the issue here as well: https://github.com/dagger/dagger/commit/f02f5bf35ac96532384f436de9896de12c8a6647#diff-da47bfee7f55e4f86b54a332c519d0d37bef79f1b869d027735afff292d7e1d4R64

glacial owl Oct 9, 2024, 2:52 AM

#

Didn't get to this today, but will do a deep dive first thing my tomorrow morning!

past mason Oct 9, 2024, 4:28 AM

#

🙏 thanks Erik.

past mason Oct 9, 2024, 1:02 PM

#

just dumping out a msg here (which I am still investigating more) to ensure you are aware that I am seeing this now (which you may have immediate idea about, no worries if you don't):

when calling id, it seems innerCacheVolume is NIL somehow. I don't know what that mean yet, and investigating it further:

dagql.ID[*github.com/dagger/dagger/core.CacheVolume]{id:(*call.ID)(0x4002470680), inner:(*core.CacheVolume)(nil)}

"ChV4eGgzOjAzOWZlMzM0Y2YzN2ZiM2EScQoVeHhoMzowMzlmZTMzNGNmMzdmYjNhElgSDwoLQ2FjaGVWb2x1bWUYARoLY2FjaGVWb2x1bWUiHwoDa2V5Ehg6FnZvbHVtZS1uYW1lLWNoZWNrLWVsc2UoAUoVeHhoMzowMzlmZTMzNGNmMzdmYjNh"

selector -> id

past mason Oct 9, 2024, 3:43 PM

#

nvrmind, I am still stuck on this, and will need help from you to unblock myself here. @glacial owl 🙏.

past mason Oct 9, 2024, 5:30 PM

#

Thank you

#

🙏

glacial owl Oct 9, 2024, 5:56 PM

#

No problem! This is getting into the tricky subtle stuff so happy to help.

This diff on your branch seems to fix it:

diff --git a/core/schema/cache.go b/core/schema/cache.go
index 278908769..dd87092eb 100644
--- a/core/schema/cache.go
+++ b/core/schema/cache.go
@@ -20,14 +20,13 @@ func (s *cacheSchema) Name() string {
 
 func (s *cacheSchema) Install() {
        dagql.Fields[*core.Query]{
-               dagql.Func("cacheVolume", s.cacheVolume).
+               dagql.NodeFunc("cacheVolume", s.cacheVolume).
                        Doc("Constructs a cache volume for a given cache key.").
                        ArgDoc("key", `A string identifier to target this cache volume (e.g., "modules-cache").`).
                        Impure("evaluate each time"),
                dagql.Func("cache", s.cache).
                        Doc("Constructs a cache volume for a given cache key.").
-                       ArgDoc("key", `A string identifier to target this cache volume (e.g., "modules-cache").`).
-                       Impure("evaluate each time"),
+                       ArgDoc("key", `A string identifier to target this cache volume (e.g., "modules-cache").`),
        }.Install(s.srv)
 
        dagql.Fields[*core.CacheVolume]{}.Install(s.srv)
@@ -45,14 +44,14 @@ func (s *cacheSchema) cache(ctx context.Context, parent *core.Query, args cacheA
        return core.NewCache(args.Key), nil
 }
 
-func (s *cacheSchema) cacheVolume(ctx context.Context, parent *core.Query, args cacheArgs) (*core.CacheVolume, error) {
+func (s *cacheSchema) cacheVolume(ctx context.Context, parent dagql.Instance[*core.Query], args cacheArgs) (inst dagql.Instance[*core.CacheVolume], _ error) {
        // TODO(vito): inject some sort of scope/session/project/user derived value
        // here instead of a static value
        //
        // we have to inject something so we can tell it's a valid ID
-       m, err := parent.Server.CurrentModule(ctx)
+       m, err := parent.Self.Server.CurrentModule(ctx)
        if err != nil && !errors.Is(err, core.ErrNoCurrentModule) {
-               return nil, err
+               return inst, err
        }
 
        key := args.Key
@@ -93,8 +92,8 @@ func (s *cacheSchema) cacheVolume(ctx context.Context, parent *core.Query, args
                },
        )
        if err != nil {
-               return nil, err
+               return inst, err
        }
 
-       return svc.Self, nil
+       return svc, nil
 }

Basically, there's a subtle difference between a Func (what you were using before) and a NodeFunc. All objects in dagql are ID-able and when you pass them around you are passing that ID around. The ID is actually just a "recipe" for how the object was created, which is essentially just the graphql query that was used to construct it (it's a slightly different format that has extra metadata, but can be converted back and forth to graphql query).

When you use a Func and return an object the ID for that object is just based on the graphql query so far. So in your case you were returning an object with an ID that amounted to cacheVolume even though the select statement was calling out to cache. The end result is that when you passed the object around to different clients, they see the recipe as cacheVolume and that API gets re-run for them and they get the cache volume namespaced to them.

By switching to a NodeFunc the code now returns an object with ID set to what you do in that Select statement; so the ID is actually just cache. The end result is that when you pass around the object to different clients, they see the recipe as just cache and only execute that API, using the cache volume key that was calculated in the context of the original client.

GitHub

dagger/dagql/call/id.go at f02f5bf35ac96532384f436de9896de12c8a6647...

An engine to run your pipelines in containers. Contribute to rajatjindal/dagger development by creating an account on GitHub.

#

Hopefully that's making some degree of sense, it's super subtle, to a degree I didn't really realize until I just had to explain it now 😅

past mason Oct 9, 2024, 6:01 PM

#

it does makes sense. thank you so much for helping out with this.

#

I'll have to read through the msg with fresh eyes tomorrow, but i do want to verify that I see expected result after the fix you suggested to ensure I have something to make progress on tomorrow 🙂

#

also, is there any documentation that you can suggest I should go through, which may help with these kind of subtle differences.

glacial owl Oct 9, 2024, 6:04 PM

#

past mason also, is there any documentation that you can suggest I should go through, which...

I'm not sure if there's anything that addresses this specifically, there's some high level design points here: https://github.com/dagger/dagger/blob/main/dagql/README.md

But not sure if there's anything else (cc @bleak trail)

past mason Oct 9, 2024, 6:11 PM

#

yep, i can see they have different ids now. thank you so much @glacial owl

bleak trail Oct 9, 2024, 6:14 PM

#

yeah the README is all there is right now, in the fullness of time it'd be fun to write more about these patterns

past mason Oct 9, 2024, 6:14 PM

#

👍

#

a walk through video of codebase would be great for new contributors.

#

(when time permits ofcourse)

#clarification for namespacing of cache volumes