Merge pull request #364 from rohanpm/commit-mode

Support different commit modes [RHELDST-20490]

Author: rohanpm

README.md

@@ -10,10 +10,10 @@ exodus-aware drop-in replacement for rsync.
 - [Installation](#installation)
 - [Configuration](#configuration)
 - [Usage](#usage)
-  - [Differences from rsync](#differences-from-rsync)
-  - [Publish modes](#publish-modes)
-    - [Standalone publish](#standalone-publish)
-    - [Joined publish](#joined-publish)
+    - [Differences from rsync](#differences-from-rsync)
+    - [Publish modes](#publish-modes)
+        - [Standalone publish](#standalone-publish)
+        - [Joined publish](#joined-publish)
 - [License](#license)
 
 <!-- /TOC -->
@@ -87,6 +87,25 @@ gwurl: https://exodus-gw.example.com
 # The exodus-gw environment may be set using an environment variable.
 gwenv: prod
 
+# The commit mode for exodus-gw publish objects, one of the following:
+#
+# "" or "auto" (default):
+#    Commit occurs only if exodus-rsync created the publish (i.e. is operating
+#    in "standalone publish" mode). A phase2 (full) commit will be used.
+#
+# "none":
+#    Commit never occurs, regardless of whether exodus-rsync created
+#    the publish.
+#
+# "phase1", "phase2", <other>...:
+#    A commit of this type occurs, regardless of whether exodus-rsync created
+#    the publish. See exodus-gw documentation for details on the behavior
+#    of each mode:
+#    https://release-engineering.github.io/exodus-gw/api.html#operation/commit_publish__env__publish__publish_id__commit_post
+#
+# The `--exodus-commit=MODE` option overrides this value.
+gwcommit: auto
+
 ###############################################################################
 # Environment configuration
 ###############################################################################
@@ -254,6 +273,7 @@ is a summary of the differences:
   | -------- | ----- |
   | --exodus-conf=PATH | use this configuration file |
   | --exodus-publish=ID | join content to an existing publish (see "Publish modes") |
+  | --exodus-commit=MODE | commit mode for publish (see `gwcommit` in config file) |
   | --exodus-diag | diagnostic mode, outputs various info for troubleshooting |
 
 - exodus-rsync supports only the following rsync arguments, most of which do not have any
@@ -362,9 +382,11 @@ exposed from the CDN or that *none* of them are exposed at all, even if we are i
 in the middle of publishing.  None of the published content becomes visible from the CDN until
 the "commit" operation occurs, which exposes all content at once.
 
+More complex scenarios are possible when specifying a custom commit mode via
+the `gwcommit` config file option or the `--exodus-commit` argument.
 See [the exodus-gw documentation](https://release-engineering.github.io/exodus-gw/api.html#section/Atomicity)
-for more information on the atomicity guarantees when publishing with
-exodus-rsync and exodus-gw.
+for more information on the supported commit modes and the atomicity
+guarantees when publishing with exodus-rsync and exodus-gw.
 
 ## License
 

internal/args/parse.go

@@ -75,6 +75,8 @@ type ExodusConfig struct {
 
 	Publish string `help:"ID of existing exodus-gw publish to join." validate:"omitempty,uuid"`
 
+	Commit string `help:"Commit publish using this mode" validate:"omitempty,max=20"`
+
 	Diag bool `help:"Diagnostic mode, dumps various information about the environment."`
 }
 

internal/cmd/cmd_gw_errors_test.go

@@ -68,7 +68,7 @@ func setupFailedCommit(ctrl *gomock.Controller, client *gw.MockClient) {
 	publish.EXPECT().AddItems(gomock.Any(), gomock.Any()).Return(nil)
 
 	// Committing fails
-	publish.EXPECT().Commit(gomock.Any()).Return(fmt.Errorf("simulated error"))
+	publish.EXPECT().Commit(gomock.Any(), gomock.Any()).Return(fmt.Errorf("simulated error"))
 }
 
 func setupFailedJoinPublish(ctrl *gomock.Controller, client *gw.MockClient) {

internal/cmd/cmd_sync_test.go

@@ -55,9 +55,11 @@ type FakeClient struct {
 }
 
 type FakePublish struct {
-	items     []gw.ItemInput
-	committed int
-	id        string
+	items       []gw.ItemInput
+	committed   int
+	commitmodes []string
+	frozen      bool
+	id          string
 }
 
 type BrokenPublish struct {
@@ -111,7 +113,7 @@ func (c *FakeClient) WhoAmI(context.Context) (map[string]interface{}, error) {
 }
 
 func (p *FakePublish) AddItems(ctx context.Context, items []gw.ItemInput) error {
-	if p.committed != 0 {
+	if p.frozen {
 		return fmt.Errorf("attempted to modify committed publish")
 	}
 	p.items = append(p.items, items...)
@@ -122,12 +124,16 @@ func (p *BrokenPublish) AddItems(_ context.Context, _ []gw.ItemInput) error {
 	return fmt.Errorf("invalid publish")
 }
 
-func (p *BrokenPublish) Commit(_ context.Context) error {
+func (p *BrokenPublish) Commit(_ context.Context, _ string) error {
 	return fmt.Errorf("invalid publish")
 }
 
-func (p *FakePublish) Commit(ctx context.Context) error {
+func (p *FakePublish) Commit(ctx context.Context, mode string) error {
+	if mode == "" || mode == "phase2" {
+		p.frozen = true
+	}
 	p.committed++
+	p.commitmodes = append(p.commitmodes, mode)
 	return nil
 }
 
@@ -148,72 +154,102 @@ func TestMainTypicalSync(t *testing.T) {
 	SetConfig(t, CONFIG)
 	ctrl := MockController(t)
 
-	mockGw := gw.NewMockInterface(ctrl)
-	ext.gw = mockGw
-
-	client := FakeClient{blobs: make(map[string]string)}
-	mockGw.EXPECT().NewClient(gomock.Any(), EnvMatcher{"best-env"}).Return(&client, nil)
-
-	srcPath := path.Clean(wd + "/../../test/data/srctrees/just-files")
-
-	args := []string{
-		"rsync",
-		srcPath + "/",
-		"exodus:/some/target",
-	}
-
-	got := Main(args)
-
-	// It should complete successfully.
-	if got != 0 {
-		t.Error("returned incorrect exit code", got)
-	}
-
-	// Check paths of some blobs we expected to deal with.
-	binPath := client.blobs["c66f610d98b2c9fe0175a3e99ba64d7fc7de45046515ff325be56329a9347dd6"]
-	helloPath := client.blobs["5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03"]
-
-	// It should have uploaded the binary from here
-	if binPath != srcPath+"/subdir/some-binary" {
-		t.Error("binary uploaded from unexpected path", binPath)
-	}
-
-	// For the hello file, since there were two copies, it's undefined which one of them
-	// was used for the upload - but should be one of them.
-	if helloPath != srcPath+"/hello-copy-one" && helloPath != srcPath+"/hello-copy-two" {
-		t.Error("hello uploaded from unexpected path", helloPath)
-	}
-
-	// It should have created one publish.
-	if len(client.publishes) != 1 {
-		t.Error("expected to create 1 publish, instead created", len(client.publishes))
-	}
-
-	p := client.publishes[0]
-
-	// Build up a URI => Key mapping of what was published
-	itemMap := make(map[string]string)
-	for _, item := range p.items {
-		if _, ok := itemMap[item.WebURI]; ok {
-			t.Error("tried to publish this URI more than once:", item.WebURI)
-		}
-		itemMap[item.WebURI] = item.ObjectKey
-	}
-
-	// It should have been exactly this
-	expectedItems := map[string]string{
-		"/some/target/subdir/some-binary": "c66f610d98b2c9fe0175a3e99ba64d7fc7de45046515ff325be56329a9347dd6",
-		"/some/target/hello-copy-one":     "5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03",
-		"/some/target/hello-copy-two":     "5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03",
-	}
-
-	if !reflect.DeepEqual(itemMap, expectedItems) {
-		t.Error("did not publish expected items, published:", itemMap)
-	}
-
-	// It should have committed the publish (once)
-	if p.committed != 1 {
-		t.Error("expected to commit publish (once), instead p.committed ==", p.committed)
+	tests := []struct {
+		name               string
+		commitArg          string
+		expectedCommits    int
+		expectedCommitMode string
+	}{
+		{"typical", "", 1, ""},
+
+		{"explicit autocommit", "--exodus-commit=auto", 1, ""},
+
+		{"no commit", "--exodus-commit=none", 0, ""},
+
+		{"commit specified mode", "--exodus-commit=xyz", 1, "xyz"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockGw := gw.NewMockInterface(ctrl)
+			ext.gw = mockGw
+
+			client := FakeClient{blobs: make(map[string]string)}
+			mockGw.EXPECT().NewClient(gomock.Any(), EnvMatcher{"best-env"}).Return(&client, nil)
+
+			srcPath := path.Clean(wd + "/../../test/data/srctrees/just-files")
+
+			args := []string{
+				"rsync",
+				srcPath + "/",
+			}
+
+			if tt.commitArg != "" {
+				args = append(args, tt.commitArg)
+			}
+			args = append(args, "exodus:/some/target")
+
+			got := Main(args)
+
+			// It should complete successfully.
+			if got != 0 {
+				t.Error("returned incorrect exit code", got)
+			}
+
+			// Check paths of some blobs we expected to deal with.
+			binPath := client.blobs["c66f610d98b2c9fe0175a3e99ba64d7fc7de45046515ff325be56329a9347dd6"]
+			helloPath := client.blobs["5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03"]
+
+			// It should have uploaded the binary from here
+			if binPath != srcPath+"/subdir/some-binary" {
+				t.Error("binary uploaded from unexpected path", binPath)
+			}
+
+			// For the hello file, since there were two copies, it's undefined which one of them
+			// was used for the upload - but should be one of them.
+			if helloPath != srcPath+"/hello-copy-one" && helloPath != srcPath+"/hello-copy-two" {
+				t.Error("hello uploaded from unexpected path", helloPath)
+			}
+
+			// It should have created one publish.
+			if len(client.publishes) != 1 {
+				t.Error("expected to create 1 publish, instead created", len(client.publishes))
+			}
+
+			p := client.publishes[0]
+
+			// Build up a URI => Key mapping of what was published
+			itemMap := make(map[string]string)
+			for _, item := range p.items {
+				if _, ok := itemMap[item.WebURI]; ok {
+					t.Error("tried to publish this URI more than once:", item.WebURI)
+				}
+				itemMap[item.WebURI] = item.ObjectKey
+			}
+
+			// It should have been exactly this
+			expectedItems := map[string]string{
+				"/some/target/subdir/some-binary": "c66f610d98b2c9fe0175a3e99ba64d7fc7de45046515ff325be56329a9347dd6",
+				"/some/target/hello-copy-one":     "5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03",
+				"/some/target/hello-copy-two":     "5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03",
+			}
+
+			if !reflect.DeepEqual(itemMap, expectedItems) {
+				t.Error("did not publish expected items, published:", itemMap)
+			}
+
+			// It should have committed (or not) as expected
+			if p.committed != tt.expectedCommits {
+				t.Error("expected ", tt.expectedCommits, " commits, got ", p.committed)
+			}
+
+			// If a commit happened at all, it should have used the mode we expect.
+			if tt.expectedCommits != 0 {
+				if p.commitmodes[0] != tt.expectedCommitMode {
+					t.Error("expected ", tt.expectedCommitMode, " commit mode, got ", p.commitmodes[0])
+				}
+			}
+		})
 	}
 }
 

internal/cmd/exodus.go

@@ -58,6 +58,28 @@ func webURI(srcPath string, srcTree string, destTree string, srcIsDir bool) stri
 	return path.Join(destTree, relPath)
 }
 
+func commitMode(cfg conf.Config, args args.Config) (bool, string) {
+	// Calculates effective commit mode for current run, given arguments and config.
+	//
+	// Returns:
+	//   bool:   true if commit should happen at all
+	//   string: the commit_mode argument which should be passed to exodus-gw
+	//           (can be empty if no argument should be passed)
+	mode := cfg.GwCommit()
+	if mode == "" || mode == "auto" {
+		// 'auto' means commit with server-default mode if and only if we
+		// created the publish object during this run, which we did if no
+		// publish ID was passed in args.
+		return args.Publish == "", ""
+	}
+	if mode == "none" {
+		// 'none' means don't ever commit
+		return false, ""
+	}
+	// Anything else means commit, using the given value as the commit mode
+	return true, mode
+}
+
 func exodusMain(ctx context.Context, cfg conf.Config, args args.Config) int {
 	logger := log.FromContext(ctx)
 
@@ -223,10 +245,10 @@ func exodusMain(ctx context.Context, cfg conf.Config, args args.Config) int {
 
 	logger.F("publish", publish.ID(), "items", len(publishItems)).Info("Added publish items")
 
-	if args.Publish == "" {
-		// We created the publish, then we should commit it.
-		logger.F("publish", publish.ID()).Info("Preparing to commit publish")
-		err = publish.Commit(ctx)
+	shouldCommit, mode := commitMode(cfg, args)
+	if shouldCommit {
+		logger.F("publish", publish.ID(), "mode", mode).Info("Preparing to commit publish")
+		err = publish.Commit(ctx, mode)
 		if err != nil {
 			logger.F("error", err).Error("can't commit publish")
 			return 71

internal/conf/conf.go

@@ -40,6 +40,9 @@ type Config interface {
 	// Max number of items to include in a single HTTP request to exodus-gw.
 	GwBatchSize() int
 
+	// Commit mode for publishes.
+	GwCommit() string
+
 	// Execution mode for rsync.
 	RsyncMode() string
 

internal/conf/conf_test.go

@@ -32,13 +32,15 @@ gwurl: $TEST_EXODUS_GW_URL
 gwcert: global-cert
 gwkey: global-key
 gwbatchsize: 100
+gwcommit: abc
 strip: dest:/foo
 
 environments:
 - prefix: dest:/foo/bar/baz
   gwenv: $TEST_EXODUS_GW_ENV
   gwkey: override-key
   gwpollinterval: 123
+  gwcommit: cba
   rsyncmode: mixed
   strip: dest:/foo/bar
   uploadthreads: 6
@@ -97,6 +99,7 @@ environments:
 	assertEqual("global gwkey", cfg.GwKey(), "global-key")
 	assertEqual("global gwenv", cfg.GwEnv(), "global-env")
 	assertEqual("global gwpollinterval", cfg.GwPollInterval(), 5000)
+	assertEqual("global gwcommit", cfg.GwCommit(), "abc")
 	assertEqual("global rsyncmode", cfg.RsyncMode(), "exodus")
 	assertEqual("global strip", cfg.Strip(), "dest:/foo")
 	assertEqual("global uploadthreads", cfg.UploadThreads(), 4)
@@ -105,6 +108,7 @@ environments:
 	assertEqual("env gwenv", env.GwEnv(), "one-env")
 	assertEqual("env gwkey", env.GwKey(), "override-key")
 	assertEqual("env gwpollinterval", env.GwPollInterval(), 123)
+	assertEqual("env gwcommit", env.GwCommit(), "cba")
 	assertEqual("env rsyncmode", env.RsyncMode(), "mixed")
 	assertEqual("env strip", env.Strip(), "dest:/foo/bar")
 	assertEqual("env uploadthreads", env.UploadThreads(), 6)