Disable xattrs by default

Change --optimizations xattr to be default behavior, and add a new flag
to disable this annotation when creating a SOCI index.

This change also eliminated the need for the optimizations structure in
the CLI.

Signed-off-by: David Son <[email protected]>

Author: sondavidb

cmd/soci/commands/create.go

@@ -18,7 +18,6 @@ package commands
 
 import (
 	"errors"
-	"fmt"
 	"os"
 
 	"github.com/awslabs/soci-snapshotter/cmd/soci/commands/internal"
@@ -32,7 +31,7 @@ const (
 	buildToolIdentifier = "AWS SOCI CLI v0.1"
 	spanSizeFlag        = "span-size"
 	minLayerSizeFlag    = "min-layer-size"
-	optimizationFlag    = "optimizations"
+	disableXAttrsFlag   = "disable-xattrs"
 )
 
 // CreateCommand creates SOCI index for an image
@@ -55,9 +54,9 @@ var CreateCommand = cli.Command{
 			Usage: "Minimum layer size to build zTOC for. Smaller layers won't have zTOC and not lazy pulled. Default is 10 MiB.",
 			Value: 10 << 20,
 		},
-		cli.StringSliceFlag{
-			Name:  optimizationFlag,
-			Usage: fmt.Sprintf("(Experimental) Enable optional optimizations. Valid values are %v", soci.Optimizations),
+		cli.BoolTFlag{
+			Name:  disableXAttrsFlag,
+			Usage: "When true, adds DisableXAttrs annotation to SOCI index. This annotation often helps performance at pull time. Default is true",
 		},
 	),
 	Action: func(cliContext *cli.Context) error {
@@ -66,15 +65,6 @@ var CreateCommand = cli.Command{
 			return errors.New("source image needs to be specified")
 		}
 
-		var optimizations []soci.Optimization
-		for _, o := range cliContext.StringSlice(optimizationFlag) {
-			optimization, err := soci.ParseOptimization(o)
-			if err != nil {
-				return err
-			}
-			optimizations = append(optimizations, optimization)
-		}
-
 		client, ctx, cancel, err := internal.NewClient(cliContext)
 		if err != nil {
 			return err
@@ -119,7 +109,10 @@ var CreateCommand = cli.Command{
 			soci.WithMinLayerSize(minLayerSize),
 			soci.WithSpanSize(spanSize),
 			soci.WithBuildToolIdentifier(buildToolIdentifier),
-			soci.WithOptimizations(optimizations),
+		}
+
+		if !cliContext.Bool(disableXAttrsFlag) {
+			builderOpts = append(builderOpts, soci.WithNoDisableXAttrs())
 		}
 
 		for _, plat := range ps {

config/config.toml

@@ -19,7 +19,7 @@ allow_no_verification=true
 # disable_verification=false
 # Causes TestRunWithDefaultConfig to break, but
 # fine to use in /etc/soci-snapshotter-grpc-config.toml
-max_concurrency=0  # Actually zero
+max_concurrency=0
 no_prometheus=false
 mount_timeout_sec=0
 fuse_metrics_emit_wait_duration_sec=0

docs/cli-usage.md

@@ -33,6 +33,7 @@ Flags:
 
  - ```--span-size``` : Span size that soci index uses to segment layer data. Default is 4MiB 
  - ```--min-layer-size``` : Minimum layer size to build zTOC for. Smaller layers won't have zTOC and not lazy pulled. Default is 10MiB
+ - ```--disable-xattrs``` : When true, adds DisableXAttrs annotation to SOCI index. This annotation often helps performance at pull time. Default is true.
 
 **Example:** 
 ```

fs/layer/layer.go

@@ -345,14 +345,14 @@ func (r *Resolver) Resolve(ctx context.Context, hosts []docker.RegistryHost, ref
 	if err != nil {
 		return nil, fmt.Errorf("failed to read layer: %w", err)
 	}
-	disableXattrs := getDisableXAttrAnnotation(sociDesc)
+	disableXAttrs := getDisableXAttrAnnotation(sociDesc)
 	// Combine layer information together and cache it.
-	l := newLayer(r, desc, blobR, vr, bgLayerResolver, opCounter, disableXattrs)
+	l := newLayer(r, desc, blobR, vr, bgLayerResolver, opCounter, disableXAttrs)
 	r.layerCacheMu.Lock()
 	cachedL, done2, added := r.layerCache.Add(name, l)
 	r.layerCacheMu.Unlock()
 	if !added {
-		l.close() // layer already exists in the cache. discrad this.
+		l.close() // layer already exists in the cache. discard this.
 	}
 
 	log.G(ctx).Debugf("resolved layer")

soci/soci_index.go

@@ -68,6 +68,7 @@ const (
 	// [soci-snapshotter] NOTE: this is a duplicate of fs/layer/layer.go so that the SOCI package and the snapshotter can
 	// be independent (and SOCI could be split out into it's own module in the future).
 	whiteoutOpaqueDir = ".wh..wh..opq"
+	disableXAttrsTrue = "true"
 )
 
 var (
@@ -215,39 +216,7 @@ type buildConfig struct {
 	buildToolIdentifier string
 	artifactsDb         *ArtifactsDb
 	platform            ocispec.Platform
-	optimizations       []Optimization
-}
-
-func (b *buildConfig) hasOptimization(o Optimization) bool {
-	for _, optimization := range b.optimizations {
-		if o == optimization {
-			return true
-		}
-	}
-	return false
-}
-
-// Optimization represents an optional optimization to be applied when building the SOCI index
-type Optimization string
-
-const (
-	// XAttrOptimization optimizes xattrs by disabling them for layers where there are no xattrs or opaque directories
-	XAttrOptimization Optimization = "xattr"
-	// Be sure to add any new optimizations to `Optimizations` below
-)
-
-// Optimizations contains the list of all known optimizations
-var Optimizations = []Optimization{XAttrOptimization}
-
-// ParseOptimization parses a string into a known optimization.
-// If the string does not match a known optimization, an error is returned.
-func ParseOptimization(s string) (Optimization, error) {
-	for _, optimization := range Optimizations {
-		if s == string(optimization) {
-			return optimization, nil
-		}
-	}
-	return "", fmt.Errorf("optimization %s is not a valid optimization %v", s, Optimizations)
+	disableXAttrs       bool
 }
 
 // BuildOption specifies a config change to build soci indices.
@@ -269,10 +238,10 @@ func WithMinLayerSize(minLayerSize int64) BuildOption {
 	}
 }
 
-// WithOptimizations enables optional optimizations when building the SOCI Index (experimental)
-func WithOptimizations(optimizations []Optimization) BuildOption {
+// WithNoDisableXAttrs will skip checking DisableXAttrs annotation
+func WithNoDisableXAttrs() BuildOption {
 	return func(c *buildConfig) error {
-		c.optimizations = optimizations
+		c.disableXAttrs = false
 		return nil
 	}
 }
@@ -318,6 +287,7 @@ func NewIndexBuilder(contentStore content.Store, blobStore orascontent.Storage,
 		minLayerSize:        defaultMinLayerSize,
 		buildToolIdentifier: defaultBuildToolIdentifier,
 		platform:            defaultPlatform,
+		disableXAttrs:       true,
 	}
 
 	for _, opt := range opts {
@@ -511,6 +481,9 @@ func (b *IndexBuilder) buildSociLayer(ctx context.Context, desc ocispec.Descript
 		IndexAnnotationImageLayerDigest:    desc.Digest.String(),
 	}
 	b.maybeAddDisableXattrAnnotation(&ztocDesc, toc)
+	if desc.Annotations[IndexAnnotationDisableXAttrs] == disableXAttrsTrue {
+		log.G(ctx).WithField("layer", ztocDesc.Digest.String()).Debug("xattrs disabled")
+	}
 	return &ztocDesc, err
 }
 
@@ -648,11 +621,11 @@ func WriteSociIndex(ctx context.Context, indexWithMetadata *IndexWithMetadata, c
 }
 
 func (b *IndexBuilder) maybeAddDisableXattrAnnotation(ztocDesc *ocispec.Descriptor, ztoc *ztoc.Ztoc) {
-	if b.config.hasOptimization(XAttrOptimization) && shouldDisableXattrs(ztoc) {
+	if b.config.disableXAttrs && shouldDisableXattrs(ztoc) {
 		if ztocDesc.Annotations == nil {
 			ztocDesc.Annotations = make(map[string]string, 1)
 		}
-		ztocDesc.Annotations[IndexAnnotationDisableXAttrs] = "true"
+		ztocDesc.Annotations[IndexAnnotationDisableXAttrs] = disableXAttrsTrue
 	}
 }
 

soci/soci_index_test.go

@@ -235,6 +235,7 @@ func TestDisableXattrs(t *testing.T) {
 		name                string
 		metadata            []ztoc.FileMetadata
 		shouldDisableXattrs bool
+		disableXAttrs       bool
 	}{
 		{
 			name: "ztoc with xattrs should not have xattrs disabled",
@@ -247,6 +248,7 @@ func TestDisableXattrs(t *testing.T) {
 				},
 			},
 			shouldDisableXattrs: false,
+			disableXAttrs:       true,
 		},
 		{
 			name: "ztoc with opaque dirs should not have xattrs disabled",
@@ -259,6 +261,7 @@ func TestDisableXattrs(t *testing.T) {
 				},
 			},
 			shouldDisableXattrs: false,
+			disableXAttrs:       true,
 		},
 		{
 			name: "ztoc with no xattrs or opaque dirs should have xattrs disabled",
@@ -271,6 +274,20 @@ func TestDisableXattrs(t *testing.T) {
 				},
 			},
 			shouldDisableXattrs: true,
+			disableXAttrs:       true,
+		},
+		{
+			name: "ztoc with no xattrs and forced GetXAttrs should have xattrs enabled",
+			metadata: []ztoc.FileMetadata{
+				{
+					Name: "dir/",
+				},
+				{
+					Name: "dir/file",
+				},
+			},
+			shouldDisableXattrs: false,
+			disableXAttrs:       false,
 		},
 	}
 	for _, tc := range testcases {
@@ -293,9 +310,13 @@ func TestDisableXattrs(t *testing.T) {
 			if err != nil {
 				t.Fatalf("can't create a test db")
 			}
-			builder, _ := NewIndexBuilder(cs, blobStore, artifactsDb, WithOptimizations([]Optimization{XAttrOptimization}))
+			opts := []BuildOption{}
+			if !tc.disableXAttrs {
+				opts = append(opts, WithNoDisableXAttrs())
+			}
+			builder, _ := NewIndexBuilder(cs, blobStore, artifactsDb, opts...)
 			builder.maybeAddDisableXattrAnnotation(&desc, &ztoc)
-			disableXattrs := desc.Annotations[IndexAnnotationDisableXAttrs] == "true"
+			disableXattrs := desc.Annotations[IndexAnnotationDisableXAttrs] == disableXAttrsTrue
 			if disableXattrs != tc.shouldDisableXattrs {
 				t.Fatalf("expected xattrs to be disabled = %v, actual = %v", tc.shouldDisableXattrs, disableXattrs)
 			}