Disable xattrs by default

Changed --optimizations xattr to be default behavior, thus removing this
flag and adding a new flag to disable this annotation on SOCI index
creation. Additionally, added a TOML variable (force_get_xattrs) to
optionally ignore this annotation at runtime.

Signed-off-by: David Son <davbson@amazon.com>

cmd/soci/commands/create.go

@@ -29,10 +29,11 @@ import (
 )
 
 const (
-	buildToolIdentifier = "AWS SOCI CLI v0.1"
-	spanSizeFlag        = "span-size"
-	minLayerSizeFlag    = "min-layer-size"
-	optimizationFlag    = "optimizations"
+	buildToolIdentifier    = "AWS SOCI CLI v0.1"
+	spanSizeFlag           = "span-size"
+	minLayerSizeFlag       = "min-layer-size"
+	forceXAttrsEnabledFlag = "force-xattrs-enabled"
+	optimizationFlag       = "optimizations"
 )
 
 // CreateCommand creates SOCI index for an image
@@ -55,6 +56,10 @@ 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.BoolFlag{
+			Name:  forceXAttrsEnabledFlag,
+			Usage: "Always get xattrs for every layer.",
+		},
 		cli.StringSliceFlag{
 			Name:  optimizationFlag,
 			Usage: fmt.Sprintf("(Experimental) Enable optional optimizations. Valid values are %v", soci.Optimizations),
@@ -122,6 +127,10 @@ var CreateCommand = cli.Command{
 			soci.WithOptimizations(optimizations),
 		}
 
+		if cliContext.Bool(forceXAttrsEnabledFlag) {
+			builderOpts = append(builderOpts, soci.WithForceGetXAttrs())
+		}
+
 		for _, plat := range ps {
 			builder, err := soci.NewIndexBuilder(cs, blobStore, artifactsDb, append(builderOpts, soci.WithPlatform(plat))...)
 

config/config.toml

@@ -19,10 +19,11 @@ 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
+force_get_xattrs=false
 
 ## config/config.go Config
 

config/fs.go

@@ -56,6 +56,10 @@ type FSConfig struct {
 	NoPrometheus                   bool   `toml:"no_prometheus"`
 	MountTimeoutSec                int64  `toml:"mount_timeout_sec"`
 	FuseMetricsEmitWaitDurationSec int64  `toml:"fuse_metrics_emit_wait_duration_sec"`
+	// ForceGetXAttrs will tell the snapshotter ignore DisableXAttrs annotation at pull time.
+	// Enabling this often results in degraded performance, as GetXAttrs is an expensive call,
+	// but it might be useful for debugging purposes.
+	ForceGetXAttrs bool `toml:"force_get_xattrs"`
 
 	RetryableHTTPClientConfig `toml:"http"`
 	BlobConfig                `toml:"blob"`

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
+ - ```--force-xattrs-enabled``` : When true, do not add DisableXAttrs annotation. This annotation often helps performance at pull time. Default is false.
 
 **Example:** 
 ```

docs/config.md

@@ -26,6 +26,7 @@ This set of variables must be at the top of your TOML file due to not belonging
 - `no_prometheus` (bool) — Toggle prometheus metrics. Default: false.
 - `mount_timeout_sec` (int) — Timeout for mount if a layer can't be resolved. Default: 30.
 - `fuse_metrics_emit_wait_duration_sec` (int) — The wait time before the snaphotter emits FUSE operation counts for an image. Default: 60.
+- `force_get_xattrs` (bool) — When true, will ignore DisableXAttrs annotation and check every layer for XAttrs, often at the cost of performance. Default: false.
 
 ## config/config.go
 ### Config

fs/layer/layer.go

@@ -345,9 +345,10 @@ 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)
+	// If ForceGetXAttrs is true, disableXAttrs should always be false.
+	disableXAttrs := !r.config.ForceGetXAttrs && 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()

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 (
@@ -216,8 +217,13 @@ type buildConfig struct {
 	artifactsDb         *ArtifactsDb
 	platform            ocispec.Platform
 	optimizations       []Optimization
+	forceGetXAttrs      bool
 }
 
+// Optimization represents an optional optimization to be applied when building the SOCI index
+type Optimization string
+
+// nolint:unused
 func (b *buildConfig) hasOptimization(o Optimization) bool {
 	for _, optimization := range b.optimizations {
 		if o == optimization {
@@ -227,17 +233,15 @@ func (b *buildConfig) hasOptimization(o Optimization) bool {
 	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"
+	// NoneOptimization is a placeholder for any future optimizations.
+	// If future optimizations are added, the below line can be commented/removed.
+	NoneOptimization = "none"
 	// Be sure to add any new optimizations to `Optimizations` below
 )
 
 // Optimizations contains the list of all known optimizations
-var Optimizations = []Optimization{XAttrOptimization}
+var Optimizations = []Optimization{NoneOptimization}
 
 // ParseOptimization parses a string into a known optimization.
 // If the string does not match a known optimization, an error is returned.
@@ -277,6 +281,14 @@ func WithOptimizations(optimizations []Optimization) BuildOption {
 	}
 }
 
+// WithForceGetXAttrs forces GetXAttrs calls on every layer
+func WithForceGetXAttrs() BuildOption {
+	return func(c *buildConfig) error {
+		c.forceGetXAttrs = true
+		return nil
+	}
+}
+
 // WithBuildToolIdentifier specifies the build tool annotation value.
 func WithBuildToolIdentifier(tool string) BuildOption {
 	return func(c *buildConfig) error {
@@ -511,6 +523,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 +663,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.forceGetXAttrs && 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
+		forceGetXAttrs      bool
 	}{
 		{
 			name: "ztoc with xattrs should not have xattrs disabled",
@@ -247,6 +248,7 @@ func TestDisableXattrs(t *testing.T) {
 				},
 			},
 			shouldDisableXattrs: false,
+			forceGetXAttrs:      false,
 		},
 		{
 			name: "ztoc with opaque dirs should not have xattrs disabled",
@@ -259,6 +261,7 @@ func TestDisableXattrs(t *testing.T) {
 				},
 			},
 			shouldDisableXattrs: false,
+			forceGetXAttrs:      false,
 		},
 		{
 			name: "ztoc with no xattrs or opaque dirs should have xattrs disabled",
@@ -271,6 +274,20 @@ func TestDisableXattrs(t *testing.T) {
 				},
 			},
 			shouldDisableXattrs: true,
+			forceGetXAttrs:      false,
+		},
+		{
+			name: "ztoc with no xattrs and forced GetXAttrs should have xattrs enabled",
+			metadata: []ztoc.FileMetadata{
+				{
+					Name: "dir/",
+				},
+				{
+					Name: "dir/file",
+				},
+			},
+			shouldDisableXattrs: false,
+			forceGetXAttrs:      true,
 		},
 	}
 	for _, tc := range testcases {
@@ -293,7 +310,11 @@ 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.forceGetXAttrs {
+				opts = append(opts, WithForceGetXAttrs())
+			}
+			builder, _ := NewIndexBuilder(cs, blobStore, artifactsDb, opts...)
 			builder.maybeAddDisableXattrAnnotation(&desc, &ztoc)
 			disableXattrs := desc.Annotations[IndexAnnotationDisableXAttrs] == "true"
 			if disableXattrs != tc.shouldDisableXattrs {