Parse remote cache attributes #3869

hiddenmarten · 2024-03-05T14:14:06Z

Hi!

Addresses: #3868
A straightforward solution that works on my side.

I'm not sure that it's suitable to put these options next to the max mode, if not please lead me to the new flags in earthly that should be created.

CLAassistant · 2024-03-05T14:14:28Z

All committers have signed the CLA.

alexcb · 2024-03-05T17:10:09Z

That's great to hear it unblocks your issue; however to accept this PR I think we would need to support parsing a config string that's formatted the same way that docker.

I had a look under buildkit and couldn't find any utility function for parsing these attributes -- maybe you could have better luck grepping for something?

Otherwise it feels easy enough for us to parse them via something like:

	func parseImageNameAndAttrs(s) (string, map[string]string, error) {
		entries := strings.Split(s, ",")
		imageName := entries[0]
		attrs := map[string][string]
		for _, entry := range entries[1:] {
			pair := strings.Split(strings.TrimSpace(entry), "=")
			//TODO check that len(pair) == 2
			attrs[k] = v
		}
	}

alexcb · 2024-03-05T17:11:04Z

builder/solver.go

@@ -166,6 +166,8 @@ func newCacheExportOpt(ref string, max bool) client.CacheOptionsEntry {
 	registryCacheOptAttrs["ref"] = ref
 	if max {
 		registryCacheOptAttrs["mode"] = "max"
+		registryCacheOptAttrs["image-manifest"] = "true"


once we handle parsing the image string, I think it's better to assign the attr values outside of this if statement.

hiddenmarten · 2024-03-07T10:39:50Z

I'm not quite sure about error handling, it should be better I suppose.
In general, the solution works for me.

@alexcb Thanks for the suggestion.

alexcb · 2024-03-07T19:30:17Z

I'm not quite sure about error handling, it should be better I suppose. In general, the solution works for me.

@alexcb Thanks for the suggestion.

good question!

how about parse them before calling the newCacheExportOpt function? something like:

$ git diff
diff --git a/builder/solver.go b/builder/solver.go
index be22709a..da7e74a6 100644
--- a/builder/solver.go
+++ b/builder/solver.go
@@ -98,10 +98,18 @@ func (s *solver) newSolveOptMulti(ctx context.Context, eg *errgroup.Group, onIma
        }
        var cacheExports []client.CacheOptionsEntry
        if s.cacheExport != "" {
-               cacheExports = append(cacheExports, newCacheExportOpt(s.cacheExport, false))
+               cacheExportName, attrs, err := parseImageNameAndAttrs(s.cacheExport)
+               if err != nil {
+                       return nil, errors.Wrapf(err, "parse cache export %s", s.cacheExport)
+               }
+               cacheExports = append(cacheExports, newCacheExportOpt(cacheExportName, attrs, false))
        }
        if s.maxCacheExport != "" {
-               cacheExports = append(cacheExports, newCacheExportOpt(s.maxCacheExport, true))
+               maxCacheExportName, attrs, err := parseImageNameAndAttrs(s.cacheExport)
+               if err != nil {
+                       return nil, errors.Wrapf(err, "parse max cache export %s", s.maxCacheExport)
+               }
+               cacheExports = append(cacheExports, newCacheExportOpt(maxCacheExportName, attrs, true))
        }
        if s.saveInlineCache {
                cacheExports = append(cacheExports, newInlineCacheOpt())

builder/solver.go

Co-authored-by: Alex Couture-Beil <alex@mofo.ca>

hiddenmarten · 2024-03-09T17:49:09Z

Okay, it was a bit challenging for me :D

I wasn't able to implement all the modifications in builder/solver.go because it caused issues with the imports.

Consequently, I opted to relocate the parsing to build_cmd.go. I hope this won't cause any inconvenience.

This reverts commit 30f327766c975db25aaf8bf57f5562a41e8ce77f.

alexcb · 2024-03-11T17:34:27Z

cmd/earthly/subcmd/build_cmd.go

 	if a.cli.Flags().RemoteCache != "" {
-		cacheImports = append(cacheImports, a.cli.Flags().RemoteCache)
+		cacheImportImageName, _, err = a.parseImageNameAndAttrs(a.cli.Flags().RemoteCache)


What's the reason for discarding the attributes here? It would be better to raise an error saying attributes are not supported by

What about error handling?

From what I understand, in buildkit, there are only output keys and no input keys, which in Earthly terminology translates to export attributes without any corresponding import attributes. Consequently, these cannot be utilized in the import process without causing issues.

The critical aspect here is the --remote-cache flag, which facilitates the provision of a ref that applies to both exporting and importing. To address my concern, I should be able to specify attributes exclusively for the export process.

Additionally, I'm unclear about the import mechanism in Earthly. Specifically, I attempted to eliminate input attributes in the solver.go file, but this effort was unsuccessful. Earthly defaulted to using export attributes for the import process, which resulted in the malfunction of the import function.

Sorry I misunderstood this change earlier, I didn't appreciate the fact that the import is also set to the same RemoteCache value that's used for the export.

can you move the error handling to into this if statement -- currently the error handling is being run even when this code block is skipped.

In the case of imports, it might have been easier to just move parseImageNameAndAttrs into a utility package (and rename to ParseImageNameAndAttrs), which could be called both here and in the solver.go code.

Sure, done.
@alexcb

alexcb · 2024-03-11T17:42:36Z

cmd/earthly/subcmd/build_cmd.go

 	if a.cli.Flags().RemoteCache != "" && a.cli.Flags().Push {
 		if a.cli.Flags().MaxRemoteCache {
-			maxCacheExport = a.cli.Flags().RemoteCache
+			maxCacheExport, cacheExportAttrs, err = a.parseImageNameAndAttrs(a.cli.Flags().RemoteCache)


since the max cache setting ends up in a different variable name, I would create a similar one for attributes

Suggested change

maxCacheExport, cacheExportAttrs, err = a.parseImageNameAndAttrs(a.cli.Flags().RemoteCache)

maxCacheExport, maxCacheExportAttrs, err = a.parseImageNameAndAttrs(a.cli.Flags().RemoteCache)

(alternatively I would remove the MaxCacheExport field entirely, and just set the max attribute to true, but please don't do this in this PR, since it might be a larger task)

For me, the alternative option looks better.

I'm just unsure if there's a chance the code was designed to support both a min and max export mode; that's why I'm wary of making an even bigger change in this PR.

I agree with you on this point.

P.S. Honestly, I think would be good to consider deprecating the --max-remote-cache flag and just adding a flag like: --export-cache-attributes, it looks a bit easier to understand from my point of view.

I can create an Issue on this topic if it is necessary.

About your suggestion, it will lead to creating additional fields

maxCacheExportAttrs map[string]string

in builder and solver structs, is that okay for you?

yes that is what I'm asking for, thanks.

Okay, I'll do it now :)

@alexcb done

alexcb · 2024-03-11T17:44:20Z

cmd/earthly/subcmd/build_cmd.go

 		}
 	}
+	if err != nil {


can you do this immediately after each call

and please use a clearer error message, like the one I suggested earlier:

return nil, errors.Wrapf(err, "parse max cache export %s", s.maxCacheExport)

this hasn't been fully addressed -- can you do this immediately after each call

I've added it in one more place

hiddenmarten · 2024-03-11T19:28:47Z

I'll update the error handling soon.

(cherry picked from commit e344ae8)

alexcb · 2024-03-12T19:50:31Z

Thanks for the help with this issue, and getting it across the finish line :)

hiddenmarten · 2024-03-12T21:43:10Z

@alexcb
Thank you for the perfect lead :)

Add image-manifest and oci-mediatypes opts next to max mode

ae824bc

hiddenmarten requested a review from a team as a code owner March 5, 2024 14:14

alexcb reviewed Mar 5, 2024

View reviewed changes

Add parseImageNameAndAttrs function

111b2ba

Update imports

966703d

hiddenmarten requested a review from alexcb March 7, 2024 18:04

alexcb reviewed Mar 7, 2024

View reviewed changes

builder/solver.go Outdated Show resolved Hide resolved

alexcb reviewed Mar 7, 2024

View reviewed changes

builder/solver.go Outdated Show resolved Hide resolved

hiddenmarten and others added 8 commits March 8, 2024 10:21

alexcb error handling update

194eee0

Co-authored-by: Alex Couture-Beil <alex@mofo.ca>

Update return

b628414

Implement review suggestions

0994b23

Fix cache export varaible

6efe06e

Update cache import

14666a5

Update two more imports

6188db8

Revert naming

ef5c486

Revert accidenta builder changes

a3c2133

hiddenmarten requested a review from alexcb March 9, 2024 17:49

Rework, parse in build_cmd.go

4c4d52f

This reverts commit 30f327766c975db25aaf8bf57f5562a41e8ce77f.

hiddenmarten force-pushed the main branch from f94efb8 to 4c4d52f Compare March 9, 2024 18:05

hiddenmarten changed the title ~~Add image-manifest and oci-mediatypes opts next to max mode~~ Parse explicit cache attributes Mar 9, 2024

hiddenmarten changed the title ~~Parse explicit cache attributes~~ Parse remote cache attributes Mar 9, 2024

alexcb reviewed Mar 11, 2024

View reviewed changes

Add error handling in build_cmd.go

d9cba63

hiddenmarten requested a review from alexcb March 11, 2024 19:52

hiddenmarten and others added 4 commits March 11, 2024 21:47

Add maxCacheExportAttrs

f8503ab

Add errors.Wrapf after parsing cacheImportImageName

2b517d4

Remove unnecessary comment

3139afc

Reworked to a public func in utils

ff664b0

(cherry picked from commit e344ae8)

hiddenmarten force-pushed the main branch from e344ae8 to ff664b0 Compare March 12, 2024 18:16

alexcb added the approved-for-tests Commnity PRs that the tests can run on label Mar 12, 2024

littleredcorvette mentioned this pull request Mar 12, 2024

Run tests for PR 3869 #3894

Closed

alexcb approved these changes Mar 12, 2024

View reviewed changes

alexcb merged commit 30c9d54 into earthly:main Mar 12, 2024
2 checks passed

alexcb mentioned this pull request Mar 15, 2024

Explicit cache does not work with AWS ECR due to wrong manifest mediatype #3714

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse remote cache attributes #3869

Parse remote cache attributes #3869

hiddenmarten commented Mar 5, 2024

CLAassistant commented Mar 5, 2024 •

edited

alexcb commented Mar 5, 2024

alexcb Mar 5, 2024

hiddenmarten Mar 7, 2024

hiddenmarten commented Mar 7, 2024 •

edited

alexcb commented Mar 7, 2024

hiddenmarten commented Mar 9, 2024

alexcb Mar 11, 2024

hiddenmarten Mar 11, 2024 •

edited

alexcb Mar 11, 2024

hiddenmarten Mar 12, 2024

alexcb Mar 11, 2024

hiddenmarten Mar 11, 2024 •

edited

alexcb Mar 11, 2024

hiddenmarten Mar 11, 2024 •

edited

hiddenmarten Mar 11, 2024 •

edited

alexcb Mar 11, 2024

hiddenmarten Mar 11, 2024

hiddenmarten Mar 11, 2024

alexcb Mar 11, 2024

alexcb Mar 11, 2024

hiddenmarten Mar 11, 2024

hiddenmarten commented Mar 11, 2024

alexcb commented Mar 12, 2024

hiddenmarten commented Mar 12, 2024

	maxCacheExport, cacheExportAttrs, err = a.parseImageNameAndAttrs(a.cli.Flags().RemoteCache)
	maxCacheExport, maxCacheExportAttrs, err = a.parseImageNameAndAttrs(a.cli.Flags().RemoteCache)

Parse remote cache attributes #3869

Parse remote cache attributes #3869

Conversation

hiddenmarten commented Mar 5, 2024

CLAassistant commented Mar 5, 2024 • edited

alexcb commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddenmarten commented Mar 7, 2024 • edited

alexcb commented Mar 7, 2024

hiddenmarten commented Mar 9, 2024

Choose a reason for hiding this comment

hiddenmarten Mar 11, 2024 • edited

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

hiddenmarten Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddenmarten Mar 11, 2024 • edited

Choose a reason for hiding this comment

hiddenmarten Mar 11, 2024 • edited

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

hiddenmarten commented Mar 11, 2024

alexcb commented Mar 12, 2024

hiddenmarten commented Mar 12, 2024

CLAassistant commented Mar 5, 2024 •

edited

hiddenmarten commented Mar 7, 2024 •

edited

hiddenmarten Mar 11, 2024 •

edited

hiddenmarten Mar 11, 2024 •

edited

hiddenmarten Mar 11, 2024 •

edited

hiddenmarten Mar 11, 2024 •

edited