Merge branch 'awslabs:main' into soci-instrumentation

Author: Swapnanil-Gupta

config/config.go

@@ -39,6 +39,7 @@
 package config
 
 import (
+	"errors"
 	"fmt"
 	"os"
 
@@ -84,8 +85,8 @@ func NewConfig() *Config {
 
 	// Set any defaults which do not align with Go zero values.
 	var initParsers = []configParser{defaultPullModes, defaultDirectoryCacheConfig}
-	for _, p := range append(initParsers, parsers...) {
-		p(cfg)
+	if err := parseConfig(cfg, append(initParsers, parsers...)); err != nil {
+		return nil
 	}
 
 	return cfg
@@ -102,18 +103,26 @@ func NewConfigFromToml(cfgPath string) (*Config, error) {
 	defer f.Close()
 
 	cfg := NewConfig()
+	if cfg == nil {
+		return nil, errors.New("error creating default config")
+	}
 	// Get configuration from specified file
 	if err = toml.NewDecoder(f).Decode(cfg); err != nil {
 		return nil, fmt.Errorf("failed to load config file %q: %w", cfgPath, err)
 	}
-	parseConfig(cfg)
+	if err := parseConfig(cfg, parsers); err != nil {
+		return nil, fmt.Errorf("config file at %q: %w", cfgPath, err)
+	}
 	return cfg, nil
 }
 
-func parseConfig(cfg *Config) {
-	for _, p := range parsers {
-		p(cfg)
+func parseConfig(cfg *Config, cfgParsers []configParser) error {
+	for _, p := range cfgParsers {
+		if err := p(cfg); err != nil {
+			return fmt.Errorf("failed to parse config: %v", err)
+		}
 	}
+	return nil
 }
 
 func parseRootConfig(cfg *Config) error {

config/config_test.go

@@ -265,6 +265,18 @@ concurrent_download_chunk_size = "1MB"
 				}
 			},
 		},
+		{
+			name: "IncorrectChunkConfig",
+			config: []byte(`
+[pull_modes.parallel_pull_unpack]
+concurrent_download_chunk_size = "badchunksize"
+`),
+			assert: func(t *testing.T, actual *Config, err error) {
+				if err == nil {
+					t.Error("Expected error, got none")
+				}
+			},
+		},
 	}
 
 	for _, test := range tests {

docs/soci-index-manifest-v2.md

@@ -45,8 +45,9 @@ There are some use-cases where the dynamic nature of SOCI Index Manifest v1 adds
 If you would like to enable SOCI v1, you can add the following to the soci snapshotter config (located at `/var/lib/soci-snapshotter-grpc/config.toml`):
 
 ```
-[pull_modes.soci_v1]
-enable = true
+[pull_modes]
+  [pull_modes.soci_v1]
+    enable = true
 ```
 
 ## Comparing SOCI Index Manifest v1 and SOCI Index Manifest v2

integration/pull_test.go

@@ -885,11 +885,7 @@ func TestPullWithParallelism(t *testing.T) {
 		},
 		{
 			name: "set chunk size",
-			opts: []snapshotterConfigOpt{
-				func(cfg *config.Config) {
-					cfg.PullModes.Parallel.ConcurrentDownloadChunkSize = 1_000_000
-				},
-			},
+			opts: []snapshotterConfigOpt{withConcurrentDownloadChunkSize(1_000_000)},
 		},
 	}
 
@@ -902,11 +898,7 @@ func TestPullWithParallelism(t *testing.T) {
 		},
 		{
 			name: "parallel unpacking",
-			opts: []snapshotterConfigOpt{
-				func(cfg *config.Config) {
-					cfg.PullModes.Parallel.MaxConcurrentUnpacks = 3
-				},
-			},
+			opts: []snapshotterConfigOpt{withConcurrentUnpacks(3)},
 		},
 	}
 

integration/startup_test.go

@@ -21,9 +21,11 @@ import (
 	"regexp"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/awslabs/soci-snapshotter/config"
 	shell "github.com/awslabs/soci-snapshotter/util/dockershell"
+	"github.com/awslabs/soci-snapshotter/util/testutil"
 	"github.com/rs/xid"
 )
 
@@ -137,3 +139,47 @@ func TestSnapshotterSystemdStartup(t *testing.T) {
 		})
 	}
 }
+
+// TestSnapshotterStartupWithBadConfig ensures snapshotter does not start if snapshotter values are incorrect.
+// Note that incorrect fields are ignored by TOML and thus are expected to work
+func TestSnapshotterStartupWithBadConfig(t *testing.T) {
+	tests := []struct {
+		name           string
+		opts           []snapshotterConfigOpt
+		expectedErrStr string
+	}{
+		{
+			name:           "Bad parallel pull size",
+			opts:           []snapshotterConfigOpt{withParallelPullMode(), withConcurrentDownloadChunkSizeStr("badstring")},
+			expectedErrStr: "invalid size format",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(tt *testing.T) {
+			sh, cleanup := newSnapshotterBaseShell(t)
+			defer cleanup()
+			// rebootContainerd would be ideal here, but that always uses a config,
+			// so we just manually start SOCI with/without a config file.
+			err := testutil.KillMatchingProcess(sh, "soci-snapshotter-grpc")
+			if err != nil {
+				sh.Fatal("failed to kill soci: %v", err)
+			}
+			configToml := getSnapshotterConfigToml(t, tc.opts...)
+			snRunCmd := []string{"/usr/local/bin/soci-snapshotter-grpc", "--log-level", sociLogLevel}
+			snRunCmd = addConfig(t, sh, configToml, snRunCmd...)
+
+			errCh := make(chan error, 1)
+			go func() {
+				_, err := sh.OLog(snRunCmd...)
+				errCh <- err
+			}()
+
+			select {
+			case <-errCh:
+			case <-time.After(2 * time.Second):
+				t.Fatalf("expected err %s but snapshotter did not fail within 2 seconds", tc.expectedErrStr)
+			}
+		})
+	}
+}

integration/util_test.go

@@ -377,6 +377,24 @@ func withUnboundedPullUnpack() snapshotterConfigOpt {
 	}
 }
 
+func withConcurrentDownloadChunkSize(chunkSize int64) snapshotterConfigOpt {
+	return func(c *config.Config) {
+		c.PullModes.Parallel.ConcurrentDownloadChunkSize = chunkSize
+	}
+}
+
+func withConcurrentDownloadChunkSizeStr(chunkSizeStr string) snapshotterConfigOpt {
+	return func(c *config.Config) {
+		c.PullModes.Parallel.ConcurrentDownloadChunkSizeStr = chunkSizeStr
+	}
+}
+
+func withConcurrentUnpacks(n int64) snapshotterConfigOpt {
+	return func(c *config.Config) {
+		c.PullModes.Parallel.MaxConcurrentUnpacks = n
+	}
+}
+
 func withDiscardUnpackedLayers() snapshotterConfigOpt {
 	return func(c *config.Config) {
 		c.PullModes.Parallel.DiscardUnpackedLayers = true

soci/store/store.go

@@ -334,7 +334,11 @@ func (s *ContainerdStore) Push(ctx context.Context, expected ocispec.Descriptor,
 		return fmt.Errorf("unexpected copy size %d, expected %d: %w", totalWritten, expected.Size, errdefs.ErrFailedPrecondition)
 	}
 
-	return writer.Commit(ctx, expected.Size, expected.Digest)
+	err = writer.Commit(ctx, expected.Size, expected.Digest)
+	if err != nil && !errors.Is(err, errdefs.ErrAlreadyExists) {
+		return err
+	}
+	return nil
 }
 
 // LabelGCRoot labels the target resource to prevent garbage collection of itself.