diff --git a/docs/development/architecture.md b/docs/development/architecture.md index 2e8cdf9eb..83b72d028 100644 --- a/docs/development/architecture.md +++ b/docs/development/architecture.md @@ -260,7 +260,7 @@ Dokku uses a file-based state system for simplicity and transparency. Plugin-specific configuration is stored in the property system: ``` -/var/lib/dokku/data/// +/var/lib/dokku/config/// ``` Properties are managed via helper functions: @@ -277,6 +277,40 @@ common.PropertyWrite("git", appName, "deploy-branch", "main") common.PropertyGet("git", appName, "deploy-branch") ``` +Property writes for a given plugin and app are serialized by a lock on that app's property directory, and each write replaces the file atomically, so readers never see a partially written value. The list and map helpers, such as `PropertyListAdd` and `PropertyMapSet`, hold the lock while they read, change, and write a property. + +Code that reads a property and writes a value derived from it must hold the lock for the whole operation, or concurrent commands for the same app will overwrite each other's changes: + +```go +// Go: update a single list property +common.PropertyListUpdate("my-plugin", appName, "items", func(lines []string) ([]string, error) { + return append(lines, "new-item"), nil +}) + +// Go: read and write several properties together +common.PropertyLock("my-plugin", appName, func(p *common.LockedProperties) error { + current := common.PropertyGet("my-plugin", appName, "current") + if err := p.Write("previous", current); err != nil { + return err + } + return p.Write("current", "new-value") +}) +``` + +```bash +# Shell +fn-my-plugin-rotate() { + declare desc="moves the current value to previous, expecting the caller to hold the my-plugin property lock" + declare APP="$1" VALUE="$2" + fn-plugin-property-write "my-plugin" "$APP" "previous" "$(fn-plugin-property-get "my-plugin" "$APP" "current")" + fn-plugin-property-write "my-plugin" "$APP" "current" "$VALUE" +} + +fn-plugin-property-lock "my-plugin" "$APP" fn-my-plugin-rotate "$APP" "new-value" +``` + +Inside a `PropertyLock` callback, writes must go through the `LockedProperties` handle, as the public writers take the same lock. Shell callbacks may call the regular `fn-plugin-property-*` helpers. + ### Per-App Data Application-specific data sometimes lives in `$DOKKU_ROOT//`: diff --git a/plugins/checks/subcommands/disable b/plugins/checks/subcommands/disable index 6f0971ed6..ab131feb6 100755 --- a/plugins/checks/subcommands/disable +++ b/plugins/checks/subcommands/disable @@ -12,6 +12,12 @@ cmd-checks-disable() { verify_app_name "$APP" local PROCTYPES="${2:-_all_}" + fn-plugin-property-lock "checks" "$APP" fn-checks-disable "$APP" "$PROCTYPES" +} + +fn-checks-disable() { + declare desc="disables zero-downtime checks for app/proctypes, expecting the caller to hold the checks property lock" + declare APP="$1" PROCTYPES="$2" local DOKKU_CHECKS_DISABLED=$(fn-plugin-property-get-default "checks" "$APP" "disabled" "") local DOKKU_CHECKS_SKIPPED=$(fn-plugin-property-get-default "checks" "$APP" "skipped" "") diff --git a/plugins/checks/subcommands/enable b/plugins/checks/subcommands/enable index 858bf389c..8d59665bc 100755 --- a/plugins/checks/subcommands/enable +++ b/plugins/checks/subcommands/enable @@ -12,7 +12,12 @@ cmd-checks-enable() { verify_app_name "$APP" local PROCTYPES="${2:-_all_}" + fn-plugin-property-lock "checks" "$APP" fn-checks-enable "$APP" "$PROCTYPES" +} +fn-checks-enable() { + declare desc="enables zero-downtime checks for app/proctypes, expecting the caller to hold the checks property lock" + declare APP="$1" PROCTYPES="$2" local DOKKU_CHECKS_DISABLED=$(fn-plugin-property-get-default "checks" "$APP" "disabled" "") local DOKKU_CHECKS_SKIPPED=$(fn-plugin-property-get-default "checks" "$APP" "skipped" "") diff --git a/plugins/checks/subcommands/skip b/plugins/checks/subcommands/skip index 7a242b6c9..f8e98a0db 100755 --- a/plugins/checks/subcommands/skip +++ b/plugins/checks/subcommands/skip @@ -12,6 +12,12 @@ cmd-checks-skip() { verify_app_name "$APP" local PROCTYPES="${2:-_all_}" + fn-plugin-property-lock "checks" "$APP" fn-checks-skip "$APP" "$PROCTYPES" +} + +fn-checks-skip() { + declare desc="skips zero-downtime checks for app/proctypes, expecting the caller to hold the checks property lock" + declare APP="$1" PROCTYPES="$2" local DOKKU_CHECKS_DISABLED=$(fn-plugin-property-get-default "checks" "$APP" "disabled" "") local DOKKU_CHECKS_SKIPPED=$(fn-plugin-property-get-default "checks" "$APP" "skipped" "") diff --git a/plugins/common/lock.go b/plugins/common/lock.go new file mode 100644 index 000000000..a520af12f --- /dev/null +++ b/plugins/common/lock.go @@ -0,0 +1,60 @@ +package common + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "syscall" +) + +// LockedDirectoriesEnvVar names the environment variable a caller holding a +// directory lock exports to its child processes. It holds a newline-separated +// list of directories, and LockDirectory runs its callback without locking a +// directory found in that list, so a subprocess spawned while the lock is held +// does not deadlock waiting on its own parent. +const LockedDirectoriesEnvVar = "DOKKU_LOCKED_DIRECTORIES" + +// LockDirectory holds an exclusive flock on dir for the duration of fn. The +// directory must already exist. The lock is advisory: it serializes callers +// that go through LockDirectory, and does not block plain reads. Locks are +// taken per open file description, so goroutines in a single process exclude +// each other as well as separate processes do. +// +// A directory listed in DOKKU_LOCKED_DIRECTORIES is treated as already held by +// a parent process, and fn runs without taking the lock again. LockDirectory is +// not reentrant otherwise: fn must not call LockDirectory on the same +// directory, as that would block forever. +func LockDirectory(dir string, fn func() error) error { + dir = filepath.Clean(dir) + if isDirectoryLockHeld(dir) { + return fn() + } + + file, err := os.Open(dir) + if err != nil { + return fmt.Errorf("Unable to open %s for locking: %w", dir, err) + } + defer file.Close() + + if err := syscall.Flock(int(file.Fd()), syscall.LOCK_EX); err != nil { + return fmt.Errorf("Unable to lock %s: %w", dir, err) + } + defer syscall.Flock(int(file.Fd()), syscall.LOCK_UN) + + return fn() +} + +// isDirectoryLockHeld reports whether dir is listed in DOKKU_LOCKED_DIRECTORIES +func isDirectoryLockHeld(dir string) bool { + for _, held := range strings.Split(os.Getenv(LockedDirectoriesEnvVar), "\n") { + held = strings.TrimSpace(held) + if held == "" { + continue + } + if filepath.Clean(held) == dir { + return true + } + } + return false +} diff --git a/plugins/common/properties.go b/plugins/common/properties.go index 9e1da22d1..a477c2fec 100644 --- a/plugins/common/properties.go +++ b/plugins/common/properties.go @@ -129,7 +129,7 @@ func PropertyGetAll(pluginName string, appName string) (map[string]string, error } for _, file := range files { - if file.IsDir() { + if file.IsDir() || strings.HasPrefix(file.Name(), ".") { continue } property := file.Name() @@ -159,7 +159,7 @@ func PropertyGetAllByPrefix(pluginName string, appName string, prefix string) (m } for _, file := range files { - if file.IsDir() { + if file.IsDir() || strings.HasPrefix(file.Name(), ".") { continue } property := file.Name() @@ -192,64 +192,49 @@ func PropertyGetDefault(pluginName, appName, property, defaultValue string) (val // PropertyListAdd adds a property to a list at an optionally specified index func PropertyListAdd(pluginName string, appName string, property string, value string, index int) error { - if err := propertyTouch(pluginName, appName, property); err != nil { - return err - } - - scannedLines, err := PropertyListGet(pluginName, appName, property) - if err != nil { - return err - } - value = strings.TrimSpace(value) + return PropertyListUpdate(pluginName, appName, property, func(scannedLines []string) ([]string, error) { + var lines []string + for i, line := range scannedLines { + if index != 0 && i == (index-1) { + lines = append(lines, value) + } + lines = append(lines, line) + } - var lines []string - for i, line := range scannedLines { - if index != 0 && i == (index-1) { + if index == 0 || index > len(scannedLines) { lines = append(lines, value) } - lines = append(lines, line) - } - if index == 0 || index > len(scannedLines) { - lines = append(lines, value) - } + return lines, nil + }) +} - return PropertyListWrite(pluginName, appName, property, lines) +// PropertyListUpdate reads a list property, passes it to fn and writes back the +// list fn returns, holding the plugin/app property lock throughout so a +// concurrent update cannot be lost between the read and the write. Nothing is +// written when fn returns an error. +func PropertyListUpdate(pluginName string, appName string, property string, fn func(lines []string) ([]string, error)) error { + return PropertyLock(pluginName, appName, func(p *LockedProperties) error { + lines, err := PropertyListGet(pluginName, appName, property) + if err != nil { + return err + } + + lines, err = fn(lines) + if err != nil { + return err + } + + return p.ListWrite(property, lines) + }) } // PropertyListWrite completely rewrites a list property func PropertyListWrite(pluginName string, appName string, property string, values []string) error { - if err := propertyTouch(pluginName, appName, property); err != nil { - return err - } - - propertyPath := getPropertyPath(pluginName, appName, property) - file, err := os.OpenFile(propertyPath, os.O_RDWR|os.O_TRUNC, 0600) - if err != nil { - return err - } - - w := bufio.NewWriter(file) - for _, line := range values { - fmt.Fprintln(w, line) - } - if err = w.Flush(); err != nil { - return fmt.Errorf("Unable to write %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - - if err := file.Close(); err != nil { - return fmt.Errorf("Unable to close %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - - if err := SetPermissions(SetPermissionInput{ - Filename: propertyPath, - Mode: os.FileMode(0600), - }); err != nil { - return fmt.Errorf("Unable to set permissions for %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - - return nil + return PropertyLock(pluginName, appName, func(p *LockedProperties) error { + return p.ListWrite(property, values) + }) } // PropertyListGet returns a property list @@ -350,71 +335,142 @@ func PropertyListGetByValue(pluginName string, appName string, property string, // PropertyListRemove removes a value from a property list func PropertyListRemove(pluginName string, appName string, property string, value string) error { - lines, err := PropertyListGet(pluginName, appName, property) - if err != nil { - return err - } - - propertyPath := getPropertyPath(pluginName, appName, property) - file, err := os.OpenFile(propertyPath, os.O_RDWR|os.O_TRUNC, 0600) - if err != nil { - return err - } - - found := false - w := bufio.NewWriter(file) - for _, line := range lines { - if line == value { - found = true - continue - } - fmt.Fprintln(w, line) - } - if err = w.Flush(); err != nil { - return fmt.Errorf("Unable to write %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - - if err := file.Close(); err != nil { - return fmt.Errorf("Unable to close %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - - if err := SetPermissions(SetPermissionInput{ - Filename: propertyPath, - Mode: os.FileMode(0600), - }); err != nil { - return fmt.Errorf("Unable to set permissions for %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - - if !found { - return errors.New("Property not found, nothing was removed") - } - - return nil + return propertyListRemoveMatching(pluginName, appName, property, func(line string) bool { + return line == value + }) } // PropertyListRemoveByPrefix removes a value by prefix from a property list func PropertyListRemoveByPrefix(pluginName string, appName string, property string, prefix string) error { - lines, err := PropertyListGet(pluginName, appName, property) - if err != nil { - return err + return propertyListRemoveMatching(pluginName, appName, property, func(line string) bool { + return strings.HasPrefix(line, prefix) + }) +} + +// propertyListRemoveMatching drops every line of a list property that matches, +// returning an error when nothing matched. A missing property is reported as +// not found without taking the lock, so removing from an app with no +// properties does not create its property directory. +func propertyListRemoveMatching(pluginName string, appName string, property string, matches func(line string) bool) error { + errNotFound := errors.New("Property not found, nothing was removed") + if !PropertyExists(pluginName, appName, property) { + return errNotFound } - propertyPath := getPropertyPath(pluginName, appName, property) - file, err := os.OpenFile(propertyPath, os.O_RDWR|os.O_TRUNC, 0600) - if err != nil { - return err - } - - found := false - w := bufio.NewWriter(file) - for _, line := range lines { - if strings.HasPrefix(line, prefix) { - found = true - continue + return PropertyListUpdate(pluginName, appName, property, func(lines []string) ([]string, error) { + found := false + kept := []string{} + for _, line := range lines { + if matches(line) { + found = true + continue + } + kept = append(kept, line) } - fmt.Fprintln(w, line) + + if !found { + return nil, errNotFound + } + return kept, nil + }) +} + +// PropertyListSet sets a value within a property list at a specified index +func PropertyListSet(pluginName string, appName string, property string, value string, index int) error { + value = strings.TrimSpace(value) + return PropertyListUpdate(pluginName, appName, property, func(scannedLines []string) ([]string, error) { + var lines []string + if index >= len(scannedLines) { + lines = append(lines, scannedLines...) + lines = append(lines, value) + } else { + for i, line := range scannedLines { + if i == index { + lines = append(lines, value) + } else { + lines = append(lines, line) + } + } + } + + return lines, nil + }) +} + +// LockedProperties writes the properties of one plugin/app pair while the +// caller holds that pair's property lock. It is handed to the PropertyLock +// callback and is only valid until the callback returns. +type LockedProperties struct { + pluginName string + appName string +} + +// Write writes a value for a property +func (p *LockedProperties) Write(property string, value string) error { + return writePropertyFile(p.pluginName, p.appName, property, []byte(value)) +} + +// Delete deletes a property +func (p *LockedProperties) Delete(property string) error { + return PropertyDelete(p.pluginName, p.appName, property) +} + +// ListWrite completely rewrites a list property +func (p *LockedProperties) ListWrite(property string, values []string) error { + var b strings.Builder + for _, line := range values { + b.WriteString(line) + b.WriteString("\n") } - if err = w.Flush(); err != nil { + return writePropertyFile(p.pluginName, p.appName, property, []byte(b.String())) +} + +// MapWrite persists a string→string map for a property as a single JSON file. +// A nil map is persisted as "{}". +func (p *LockedProperties) MapWrite(property string, m map[string]string) error { + if m == nil { + m = map[string]string{} + } + + b, err := json.Marshal(m) + if err != nil { + return fmt.Errorf("Unable to marshal %s map property %s.%s: %w", p.pluginName, p.appName, property, err) + } + + return writePropertyFile(p.pluginName, p.appName, property, b) +} + +// PropertyLock runs fn while holding an exclusive lock on the properties of a +// plugin/app pair, creating the property directory when it does not exist yet. +// Use it to read properties and write values derived from them without a +// concurrent writer slipping in between. Reads inside fn go through the normal +// getters; writes must go through the LockedProperties handle, since the public +// writers take this same lock and would block forever. Locks for other +// plugin/app pairs may be taken inside fn. +func PropertyLock(pluginName string, appName string, fn func(p *LockedProperties) error) error { + if err := makePluginAppPropertyPath(pluginName, appName); err != nil { + return fmt.Errorf("Unable to create %s config directory for %s: %s", pluginName, appName, err.Error()) + } + + return LockDirectory(getPluginAppPropertyPath(pluginName, appName), func() error { + return fn(&LockedProperties{pluginName: pluginName, appName: appName}) + }) +} + +// writePropertyFile atomically replaces a property's contents. The data is +// written to a hidden temporary file in the property directory and renamed +// into place, so a concurrent reader sees either the old or the new contents +// and never a truncated file. +func writePropertyFile(pluginName string, appName string, property string, data []byte) error { + propertyPath := getPropertyPath(pluginName, appName, property) + file, err := os.CreateTemp(filepath.Dir(propertyPath), fmt.Sprintf(".%s.tmp-*", property)) + if err != nil { + return fmt.Errorf("Unable to write %s config value %s.%s: %s", pluginName, appName, property, err.Error()) + } + defer os.Remove(file.Name()) + + if _, err := file.Write(data); err != nil { + file.Close() return fmt.Errorf("Unable to write %s config value %s.%s: %s", pluginName, appName, property, err.Error()) } @@ -423,96 +479,24 @@ func PropertyListRemoveByPrefix(pluginName string, appName string, property stri } if err := SetPermissions(SetPermissionInput{ - Filename: propertyPath, + Filename: file.Name(), Mode: os.FileMode(0600), }); err != nil { return fmt.Errorf("Unable to set permissions for %s config value %s.%s: %s", pluginName, appName, property, err.Error()) } - if !found { - return errors.New("Property not found, nothing was removed") - } - - return nil -} - -// PropertyListSet sets a value within a property list at a specified index -func PropertyListSet(pluginName string, appName string, property string, value string, index int) error { - if err := propertyTouch(pluginName, appName, property); err != nil { - return err - } - - scannedLines, err := PropertyListGet(pluginName, appName, property) - if err != nil { - return err - } - - value = strings.TrimSpace(value) - - var lines []string - if index >= len(scannedLines) { - lines = append(lines, scannedLines...) - lines = append(lines, value) - } else { - for i, line := range scannedLines { - if i == index { - lines = append(lines, value) - } else { - lines = append(lines, line) - } - } - } - - return PropertyListWrite(pluginName, appName, property, lines) -} - -// propertyTouch ensures a given application property file exists -func propertyTouch(pluginName string, appName string, property string) error { - if err := makePluginAppPropertyPath(pluginName, appName); err != nil { - return fmt.Errorf("Unable to create %s config directory for %s: %s", pluginName, appName, err.Error()) - } - - propertyPath := getPropertyPath(pluginName, appName, property) - if PropertyExists(pluginName, appName, property) { - return nil - } - - file, err := os.Create(propertyPath) - if err != nil { + if err := os.Rename(file.Name(), propertyPath); err != nil { return fmt.Errorf("Unable to write %s config value %s.%s: %s", pluginName, appName, property, err.Error()) } - defer file.Close() return nil } // PropertyWrite writes a value for a given application property func PropertyWrite(pluginName string, appName string, property string, value string) error { - if err := propertyTouch(pluginName, appName, property); err != nil { - return err - } - - propertyPath := getPropertyPath(pluginName, appName, property) - file, err := os.Create(propertyPath) - if err != nil { - return fmt.Errorf("Unable to write %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - defer file.Close() - - fmt.Fprint(file, value) - - if err := file.Close(); err != nil { - return fmt.Errorf("Unable to close %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - - if err := SetPermissions(SetPermissionInput{ - Filename: propertyPath, - Mode: os.FileMode(0600), - }); err != nil { - return fmt.Errorf("Unable to set permissions for %s config value %s.%s: %s", pluginName, appName, property, err.Error()) - } - - return nil + return PropertyLock(pluginName, appName, func(p *LockedProperties) error { + return p.Write(property, value) + }) } // PropertyMapWrite persists a string→string map for a given property as a single @@ -520,32 +504,9 @@ func PropertyWrite(pluginName string, appName string, property string, value str // bytes (including '/' or '\n') because they live inside file content rather // than in the on-disk filename. A nil map is persisted as "{}". func PropertyMapWrite(pluginName string, appName string, property string, m map[string]string) error { - if err := propertyTouch(pluginName, appName, property); err != nil { - return err - } - - if m == nil { - m = map[string]string{} - } - - b, err := json.Marshal(m) - if err != nil { - return fmt.Errorf("Unable to marshal %s map property %s.%s: %w", pluginName, appName, property, err) - } - - propertyPath := getPropertyPath(pluginName, appName, property) - if err := os.WriteFile(propertyPath, b, 0600); err != nil { - return fmt.Errorf("Unable to write %s map property %s.%s: %w", pluginName, appName, property, err) - } - - if err := SetPermissions(SetPermissionInput{ - Filename: propertyPath, - Mode: os.FileMode(0600), - }); err != nil { - return fmt.Errorf("Unable to set permissions for %s map property %s.%s: %w", pluginName, appName, property, err) - } - - return nil + return PropertyLock(pluginName, appName, func(p *LockedProperties) error { + return p.MapWrite(property, m) + }) } // PropertyMapGet returns the string→string map persisted for a property. A @@ -573,12 +534,14 @@ func PropertyMapGet(pluginName string, appName string, property string) (map[str // PropertyMapSet reads the current map, sets one key, and writes the map back. func PropertyMapSet(pluginName string, appName string, property string, key string, value string) error { - m, err := PropertyMapGet(pluginName, appName, property) - if err != nil { - return err - } - m[key] = value - return PropertyMapWrite(pluginName, appName, property, m) + return PropertyLock(pluginName, appName, func(p *LockedProperties) error { + m, err := PropertyMapGet(pluginName, appName, property) + if err != nil { + return err + } + m[key] = value + return p.MapWrite(property, m) + }) } // PropertyMapDelete reads the current map, removes one key, and writes the map @@ -587,15 +550,17 @@ func PropertyMapDelete(pluginName string, appName string, property string, key s if !PropertyExists(pluginName, appName, property) { return nil } - m, err := PropertyMapGet(pluginName, appName, property) - if err != nil { - return err - } - if _, ok := m[key]; !ok { - return nil - } - delete(m, key) - return PropertyMapWrite(pluginName, appName, property, m) + return PropertyLock(pluginName, appName, func(p *LockedProperties) error { + m, err := PropertyMapGet(pluginName, appName, property) + if err != nil { + return err + } + if _, ok := m[key]; !ok { + return nil + } + delete(m, key) + return p.MapWrite(property, m) + }) } // PropertyMapLength returns the number of entries in the map persisted for a diff --git a/plugins/common/properties_test.go b/plugins/common/properties_test.go index cdf44a052..2182c3235 100644 --- a/plugins/common/properties_test.go +++ b/plugins/common/properties_test.go @@ -1,9 +1,14 @@ package common import ( + "errors" + "fmt" "os" "path/filepath" + "strconv" + "sync" "testing" + "time" . "github.com/onsi/gomega" ) @@ -168,3 +173,172 @@ func TestPropertyMapMalformedJSONReturnsError(t *testing.T) { _, err := PropertyMapGet(propertyTestPlugin, propertyTestApp, propertyTestName) Expect(err).To(HaveOccurred()) } + +func TestPropertyListAddConcurrent(t *testing.T) { + setupPropertyMapTest(t) + + Expect(runConcurrently(50, func(i int) error { + return PropertyListAdd(propertyTestPlugin, propertyTestApp, "list", fmt.Sprintf("value-%d", i), 0) + })).To(Succeed()) + + lines, err := PropertyListGet(propertyTestPlugin, propertyTestApp, "list") + Expect(err).NotTo(HaveOccurred()) + Expect(lines).To(HaveLen(50)) + for i := 0; i < 50; i++ { + Expect(lines).To(ContainElement(fmt.Sprintf("value-%d", i))) + } +} + +func TestPropertyListRemoveConcurrent(t *testing.T) { + setupPropertyMapTest(t) + + values := []string{} + for i := 0; i < 50; i++ { + values = append(values, fmt.Sprintf("value-%d", i)) + } + Expect(PropertyListWrite(propertyTestPlugin, propertyTestApp, "list", values)).To(Succeed()) + + Expect(runConcurrently(len(values), func(i int) error { + return PropertyListRemove(propertyTestPlugin, propertyTestApp, "list", values[i]) + })).To(Succeed()) + + lines, err := PropertyListGet(propertyTestPlugin, propertyTestApp, "list") + Expect(err).NotTo(HaveOccurred()) + Expect(lines).To(BeEmpty()) +} + +func TestPropertyListRemoveMissingDoesNotCreateDirectory(t *testing.T) { + setupPropertyMapTest(t) + + Expect(PropertyListRemove(propertyTestPlugin, "missing-app", "list", "value")).NotTo(Succeed()) + Expect(PropertyListRemoveByPrefix(propertyTestPlugin, "missing-app", "list", "value")).NotTo(Succeed()) + Expect(PropertyMapDelete(propertyTestPlugin, "missing-app", "map", "key")).To(Succeed()) + Expect(DirectoryExists(getPluginAppPropertyPath(propertyTestPlugin, "missing-app"))).To(BeFalse()) +} + +func TestPropertyListRemoveNotFoundLeavesList(t *testing.T) { + setupPropertyMapTest(t) + + Expect(PropertyListWrite(propertyTestPlugin, propertyTestApp, "list", []string{"a", "b"})).To(Succeed()) + Expect(PropertyListRemove(propertyTestPlugin, propertyTestApp, "list", "c")).To(MatchError("Property not found, nothing was removed")) + + lines, err := PropertyListGet(propertyTestPlugin, propertyTestApp, "list") + Expect(err).NotTo(HaveOccurred()) + Expect(lines).To(Equal([]string{"a", "b"})) +} + +func TestPropertyListUpdateErrorLeavesListUnchanged(t *testing.T) { + setupPropertyMapTest(t) + + Expect(PropertyListWrite(propertyTestPlugin, propertyTestApp, "list", []string{"a", "b"})).To(Succeed()) + err := PropertyListUpdate(propertyTestPlugin, propertyTestApp, "list", func(lines []string) ([]string, error) { + return nil, errors.New("boom") + }) + Expect(err).To(MatchError("boom")) + + lines, err := PropertyListGet(propertyTestPlugin, propertyTestApp, "list") + Expect(err).NotTo(HaveOccurred()) + Expect(lines).To(Equal([]string{"a", "b"})) +} + +func TestPropertyMapSetConcurrent(t *testing.T) { + setupPropertyMapTest(t) + + Expect(runConcurrently(50, func(i int) error { + return PropertyMapSet(propertyTestPlugin, propertyTestApp, propertyTestName, fmt.Sprintf("key-%d", i), "value") + })).To(Succeed()) + + got, err := PropertyMapGet(propertyTestPlugin, propertyTestApp, propertyTestName) + Expect(err).NotTo(HaveOccurred()) + Expect(got).To(HaveLen(50)) +} + +func TestPropertyLockSerializesReadModifyWrite(t *testing.T) { + setupPropertyMapTest(t) + + Expect(runConcurrently(50, func(i int) error { + return PropertyLock(propertyTestPlugin, propertyTestApp, func(p *LockedProperties) error { + current, err := strconv.Atoi(PropertyGetDefault(propertyTestPlugin, propertyTestApp, "counter", "0")) + if err != nil { + return err + } + return p.Write("counter", strconv.Itoa(current+1)) + }) + })).To(Succeed()) + + Expect(PropertyGet(propertyTestPlugin, propertyTestApp, "counter")).To(Equal("50")) +} + +func TestLockDirectorySkipsDirectoriesHeldByParent(t *testing.T) { + setupPropertyMapTest(t) + + dir := getPluginAppPropertyPath(propertyTestPlugin, propertyTestApp) + Expect(makePluginAppPropertyPath(propertyTestPlugin, propertyTestApp)).To(Succeed()) + t.Setenv(LockedDirectoriesEnvVar, "/some/other/dir\n"+dir+"/") + + done := make(chan error, 1) + go func() { + done <- LockDirectory(dir, func() error { + return PropertyWrite(propertyTestPlugin, propertyTestApp, "nested", "value") + }) + }() + + select { + case err := <-done: + Expect(err).NotTo(HaveOccurred()) + case <-time.After(5 * time.Second): + t.Fatal("nested lock on a directory listed in DOKKU_LOCKED_DIRECTORIES blocked") + } + Expect(PropertyGet(propertyTestPlugin, propertyTestApp, "nested")).To(Equal("value")) +} + +func TestPropertyWritesLeaveNoTemporaryFiles(t *testing.T) { + setupPropertyMapTest(t) + + Expect(PropertyWrite(propertyTestPlugin, propertyTestApp, "plain", "value")).To(Succeed()) + Expect(PropertyListWrite(propertyTestPlugin, propertyTestApp, "list", []string{"a"})).To(Succeed()) + Expect(PropertyMapSet(propertyTestPlugin, propertyTestApp, propertyTestName, "key", "value")).To(Succeed()) + + entries, err := os.ReadDir(getPluginAppPropertyPath(propertyTestPlugin, propertyTestApp)) + Expect(err).NotTo(HaveOccurred()) + names := []string{} + for _, entry := range entries { + names = append(names, entry.Name()) + } + Expect(names).To(ConsistOf("plain", "list", propertyTestName)) +} + +func TestPropertyGetAllIgnoresHiddenFiles(t *testing.T) { + setupPropertyMapTest(t) + + Expect(PropertyWrite(propertyTestPlugin, propertyTestApp, "plain", "value")).To(Succeed()) + stray := filepath.Join(getPluginAppPropertyPath(propertyTestPlugin, propertyTestApp), ".plain.tmp-123") + Expect(os.WriteFile(stray, []byte("partial"), 0600)).To(Succeed()) + + all, err := PropertyGetAll(propertyTestPlugin, propertyTestApp) + Expect(err).NotTo(HaveOccurred()) + Expect(all).To(Equal(map[string]string{"plain": "value"})) + + byPrefix, err := PropertyGetAllByPrefix(propertyTestPlugin, propertyTestApp, "") + Expect(err).NotTo(HaveOccurred()) + Expect(byPrefix).To(Equal(map[string]string{"plain": "value"})) +} + +// runConcurrently calls fn with 0 through n-1 from n goroutines started +// together, returning the joined errors of every call. +func runConcurrently(n int, fn func(i int) error) error { + var wg sync.WaitGroup + start := make(chan struct{}) + errs := make([]error, n) + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + errs[i] = fn(i) + }(i) + } + close(start) + wg.Wait() + return errors.Join(errs...) +} diff --git a/plugins/common/property-functions b/plugins/common/property-functions index b273ddfa4..0b1c1645a 100755 --- a/plugins/common/property-functions +++ b/plugins/common/property-functions @@ -116,3 +116,43 @@ fn-plugin-property-setup-app() { declare PLUGIN="$1" APP="$2" "$PLUGIN_CORE_AVAILABLE_PATH/common/prop" "setup-app" "$PLUGIN" "$APP" } + +fn-plugin-property-lock() { + declare desc="runs a callback while holding the exclusive property lock for an app" + declare PLUGIN="$1" APP="$2" CALLBACK="$3" + shift 3 + local lock_dir="${DOKKU_LIB_ROOT}/config/${PLUGIN}/${APP}" + local lock_fd previous_locked_directories="$DOKKU_LOCKED_DIRECTORIES" + + if fn-plugin-property-lock-held "$lock_dir"; then + "$CALLBACK" "$@" + return + fi + + fn-plugin-property-setup-app "$PLUGIN" "$APP" + exec {lock_fd}<"$lock_dir" + flock "$lock_fd" + + export DOKKU_LOCKED_DIRECTORIES="${previous_locked_directories:+$previous_locked_directories$'\n'}$lock_dir" + "$CALLBACK" "$@" + local status=$? + + if [[ -n "$previous_locked_directories" ]]; then + export DOKKU_LOCKED_DIRECTORIES="$previous_locked_directories" + else + unset DOKKU_LOCKED_DIRECTORIES + fi + exec {lock_fd}<&- + return "$status" +} + +fn-plugin-property-lock-held() { + declare desc="returns whether a property lock directory is already held by a parent process" + declare LOCK_DIR="$1" + local held + + while IFS= read -r held; do + [[ -n "$held" ]] && [[ "$held" == "$LOCK_DIR" ]] && return 0 + done <<<"$DOKKU_LOCKED_DIRECTORIES" + return 1 +} diff --git a/plugins/config/concurrency_test.go b/plugins/config/concurrency_test.go new file mode 100644 index 000000000..d807b971e --- /dev/null +++ b/plugins/config/concurrency_test.go @@ -0,0 +1,62 @@ +package config + +import ( + "errors" + "fmt" + "sync" + "testing" + + . "github.com/onsi/gomega" +) + +func TestConfigSetManyConcurrent(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t) + + const count = 20 + var wg sync.WaitGroup + errs := make([]error, count) + for i := 0; i < count; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + errs[i] = SetMany(testAppName, map[string]string{fmt.Sprintf("KEY%d", i): "value"}, false, false) + }(i) + } + wg.Wait() + Expect(errors.Join(errs...)).NotTo(HaveOccurred()) + + for i := 0; i < count; i++ { + expectValue(testAppName, fmt.Sprintf("KEY%d", i), "value") + } + expectValue(testAppName, "testKey", "TESTING") +} + +func TestConfigUnsetManyConcurrent(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t) + + const count = 20 + entries := map[string]string{} + for i := 0; i < count; i++ { + entries[fmt.Sprintf("KEY%d", i)] = "value" + } + Expect(SetMany(testAppName, entries, false, false)).To(Succeed()) + + var wg sync.WaitGroup + errs := make([]error, count) + for i := 0; i < count; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + errs[i] = UnsetMany(testAppName, []string{fmt.Sprintf("KEY%d", i)}, false) + }(i) + } + wg.Wait() + Expect(errors.Join(errs...)).NotTo(HaveOccurred()) + + for i := 0; i < count; i++ { + expectNoValue(testAppName, fmt.Sprintf("KEY%d", i)) + } + expectValue(testAppName, "testKey", "TESTING") +} diff --git a/plugins/config/config.go b/plugins/config/config.go index 83878ab67..1ebbb94c5 100644 --- a/plugins/config/config.go +++ b/plugins/config/config.go @@ -3,6 +3,7 @@ package config import ( "fmt" "os" + "path/filepath" "regexp" "github.com/dokku/dokku/plugins/common" @@ -32,10 +33,6 @@ func GetWithDefault(appName string, key string, defaultValue string) (value stri // SetMany variables in the environment. If appName is empty the global config is used. If restart is true the app is restarted. func SetMany(appName string, entries map[string]string, replace bool, restart bool) (err error) { global := appName == "" || appName == "--global" - env, err := loadAppOrGlobalEnv(appName) - if err != nil { - return - } keys := make([]string, 0, len(entries)) for k := range entries { if err = validateKey(k); err != nil { @@ -43,21 +40,28 @@ func SetMany(appName string, entries map[string]string, replace bool, restart bo } } - if replace { - env.Clear() - } - for k, v := range entries { - env.Set(k, v) - keys = append(keys, k) - } - if len(entries) != 0 { + changed, err := updateAppOrGlobalEnv(appName, func(env *Env) bool { + if replace { + env.Clear() + } + for k, v := range entries { + env.Set(k, v) + keys = append(keys, k) + } + if len(entries) == 0 { + return false + } + common.LogInfo1Quiet("Setting config vars") if os.Getenv("DOKKU_QUIET_OUTPUT") == "" { fmt.Println(prettyPrintEnvEntries(" ", entries)) } - if err := env.Write(); err != nil { - return fmt.Errorf("Unable to write config vars: %s", err.Error()) - } + return true + }) + if err != nil { + return err + } + if changed { triggerUpdate(appName, "set", keys) } if !global && restart && shouldRestart(appName) { @@ -69,29 +73,28 @@ func SetMany(appName string, entries map[string]string, replace bool, restart bo // UnsetMany a value in a config. If appName is empty the global config is used. If restart is true the app is restarted. func UnsetMany(appName string, keys []string, restart bool) (err error) { global := appName == "" || appName == "--global" - env, err := loadAppOrGlobalEnv(appName) - if err != nil { - return - } - var changed = false for _, k := range keys { if err = validateKey(k); err != nil { return } } - for _, k := range keys { - if _, hasKey := env.Map()[k]; hasKey { - common.LogInfo1Quiet(fmt.Sprintf("Unsetting %s", k)) - env.Unset(k) - changed = true - } else { - common.LogInfo1Quiet(fmt.Sprintf("Skipping %s, it is not set in the environment", k)) + changed, err := updateAppOrGlobalEnv(appName, func(env *Env) bool { + changed := false + for _, k := range keys { + if _, hasKey := env.Map()[k]; hasKey { + common.LogInfo1Quiet(fmt.Sprintf("Unsetting %s", k)) + env.Unset(k) + changed = true + } else { + common.LogInfo1Quiet(fmt.Sprintf("Skipping %s, it is not set in the environment", k)) + } } + return changed + }) + if err != nil { + return err } if changed { - if err := env.Write(); err != nil { - return fmt.Errorf("Unable to write config vars: %s", err.Error()) - } triggerUpdate(appName, "unset", keys) } if !global && restart && shouldRestart(appName) { @@ -103,20 +106,19 @@ func UnsetMany(appName string, keys []string, restart bool) (err error) { // UnsetAll removes all config keys func UnsetAll(appName string, restart bool) (err error) { global := appName == "" || appName == "--global" - env, err := loadAppOrGlobalEnv(appName) + changed, err := updateAppOrGlobalEnv(appName, func(env *Env) bool { + changed := false + for k := range env.Map() { + common.LogInfo1Quiet(fmt.Sprintf("Unsetting %s", k)) + env.Unset(k) + changed = true + } + return changed + }) if err != nil { - return - } - var changed = false - for k := range env.Map() { - common.LogInfo1Quiet(fmt.Sprintf("Unsetting %s", k)) - env.Unset(k) - changed = true + return err } if changed { - if err := env.Write(); err != nil { - return fmt.Errorf("Unable to write config vars: %s", err.Error()) - } triggerUpdate(appName, "clear", []string{}) } if !global && restart && shouldRestart(appName) { @@ -125,6 +127,48 @@ func UnsetAll(appName string, restart bool) (err error) { return } +// updateAppOrGlobalEnv loads the app or global environment, passes it to +// update and writes it back when update reports a change, returning whether it +// did. The directory holding the environment file stays locked from the load +// to the write, so concurrent config changes cannot drop each other's keys. +// Triggers reacting to the change must run after this returns, as a trigger +// that changes the same environment would otherwise wait on the lock forever. +func updateAppOrGlobalEnv(appName string, update func(env *Env) bool) (bool, error) { + filename := getGlobalFile() + if appName != "" && appName != "--global" { + var err error + if filename, err = getAppFile(appName); err != nil { + return false, err + } + } + + changed := false + apply := func() error { + env, err := loadAppOrGlobalEnv(appName) + if err != nil { + return err + } + + if !update(env) { + return nil + } + + if err := env.Write(); err != nil { + return fmt.Errorf("Unable to write config vars: %s", err.Error()) + } + changed = true + return nil + } + + dir := filepath.Dir(filename) + if !common.DirectoryExists(dir) { + return changed, apply() + } + + err := common.LockDirectory(dir, apply) + return changed, err +} + func shouldRestart(appName string) bool { results, _ := common.CallPlugnTrigger(common.PlugnTriggerInput{ Trigger: "ps-get-property", diff --git a/plugins/config/migrate.go b/plugins/config/migrate.go index 1e46340a1..0e593061d 100644 --- a/plugins/config/migrate.go +++ b/plugins/config/migrate.go @@ -54,12 +54,14 @@ func migrateGlobalEnv() error { } oldGlobalEnvFile := filepath.Join(common.MustGetEnv("DOKKU_ROOT"), "ENV") - globalEnv, err := LoadGlobalEnv() - if err != nil { - return fmt.Errorf("Unable to load global environment: %s", err.Error()) - } + return common.LockDirectory(filepath.Dir(getGlobalFile()), func() error { + globalEnv, err := LoadGlobalEnv() + if err != nil { + return fmt.Errorf("Unable to load global environment: %s", err.Error()) + } - return drainLegacyEnvFile("--global", oldGlobalEnvFile, globalEnv) + return drainLegacyEnvFile("--global", oldGlobalEnvFile, globalEnv) + }) } // migrateAppEnv drains $DOKKU_ROOT//ENV into the app's config property path. @@ -72,13 +74,20 @@ func migrateAppEnv(appName string) error { return fmt.Errorf("Unable to setup app config directory: %s", err.Error()) } - env, err := LoadAppEnv(appName) + appFile, err := getAppFile(appName) if err != nil { - return fmt.Errorf("Unable to load app environment: %s", err.Error()) + return err } oldEnvFile := filepath.Join(common.AppRoot(appName), "ENV") - return drainLegacyEnvFile(appName, oldEnvFile, env) + return common.LockDirectory(filepath.Dir(appFile), func() error { + env, err := LoadAppEnv(appName) + if err != nil { + return fmt.Errorf("Unable to load app environment: %s", err.Error()) + } + + return drainLegacyEnvFile(appName, oldEnvFile, env) + }) } // drainLegacyEnvFile merges the legacy ENV file at oldEnvFile into env, records diff --git a/plugins/docker-options/concurrency_test.go b/plugins/docker-options/concurrency_test.go new file mode 100644 index 000000000..10c53ae8c --- /dev/null +++ b/plugins/docker-options/concurrency_test.go @@ -0,0 +1,72 @@ +package dockeroptions + +import ( + "errors" + "fmt" + "sync" + "testing" +) + +func TestAddDockerOptionToPhasesConcurrent(t *testing.T) { + const appName = "race-test" + setupReportEnv(t, appName) + + const count = 20 + var wg sync.WaitGroup + errs := make([]error, count) + for i := 0; i < count; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + errs[i] = AddDockerOptionToPhases(appName, []string{"deploy", "run"}, fmt.Sprintf("--label=race%d", i)) + }(i) + } + wg.Wait() + if err := errors.Join(errs...); err != nil { + t.Fatalf("AddDockerOptionToPhases: %v", err) + } + + for _, phase := range []string{"deploy", "run"} { + options, err := GetDockerOptionsForPhase(appName, phase) + if err != nil { + t.Fatalf("GetDockerOptionsForPhase(%s): %v", phase, err) + } + if len(options) != count { + t.Fatalf("%s phase has %d options, want %d: %v", phase, len(options), count, options) + } + } +} + +func TestRemoveDockerOptionFromPhasesConcurrent(t *testing.T) { + const appName = "race-test" + setupReportEnv(t, appName) + + const count = 20 + for i := 0; i < count; i++ { + if err := AddDockerOptionToPhases(appName, []string{"deploy"}, fmt.Sprintf("--label=race%d", i)); err != nil { + t.Fatalf("AddDockerOptionToPhases: %v", err) + } + } + + var wg sync.WaitGroup + errs := make([]error, count) + for i := 0; i < count; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + errs[i] = RemoveDockerOptionFromPhases(appName, []string{"deploy"}, fmt.Sprintf("--label=race%d", i)) + }(i) + } + wg.Wait() + if err := errors.Join(errs...); err != nil { + t.Fatalf("RemoveDockerOptionFromPhases: %v", err) + } + + options, err := GetDockerOptionsForPhase(appName, "deploy") + if err != nil { + t.Fatalf("GetDockerOptionsForPhase: %v", err) + } + if len(options) != 0 { + t.Fatalf("deploy phase still has options: %v", options) + } +} diff --git a/plugins/docker-options/dockeroptions.go b/plugins/docker-options/dockeroptions.go index 92a9ac34c..7000a04b4 100644 --- a/plugins/docker-options/dockeroptions.go +++ b/plugins/docker-options/dockeroptions.go @@ -198,32 +198,17 @@ func SetDockerOptionForPhases(appName string, phases []string, name string, valu // SetDockerOptionForProcessPhases sets a `--name=value` option for the specified // process types and phases, replacing any existing entry with the same name. func SetDockerOptionForProcessPhases(appName string, processTypes []string, phases []string, name string, value string) error { - if len(processTypes) == 0 { - processTypes = []string{DefaultProcessType} - } - for _, processType := range processTypes { - for _, phase := range phases { - options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) - if err != nil { - return err - } - - newOptions := []string{} - for _, option := range options { - if strings.HasPrefix(option, fmt.Sprintf("--%s=", name)) { - continue - } - newOptions = append(newOptions, option) - } - - newOptions = append(newOptions, fmt.Sprintf("--%s=%s", name, value)) - sort.Strings(newOptions) - if err := writeDockerOptionsForProcessPhase(appName, processType, phase, newOptions); err != nil { - return err + return updateDockerOptionsForProcessPhases(appName, processTypes, phases, func(options []string) []string { + newOptions := []string{} + for _, option := range options { + if strings.HasPrefix(option, fmt.Sprintf("--%s=", name)) { + continue } + newOptions = append(newOptions, option) } - } - return nil + + return append(newOptions, fmt.Sprintf("--%s=%s", name, value)) + }) } // AddDockerOptionToPhases adds an option to the default scope for the specified phases. @@ -233,24 +218,9 @@ func AddDockerOptionToPhases(appName string, phases []string, option string) err // AddDockerOptionToProcessPhases adds an option to the specified process types and phases. func AddDockerOptionToProcessPhases(appName string, processTypes []string, phases []string, option string) error { - if len(processTypes) == 0 { - processTypes = []string{DefaultProcessType} - } - for _, processType := range processTypes { - for _, phase := range phases { - options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) - if err != nil { - return err - } - - options = append(options, option) - sort.Strings(options) - if err := writeDockerOptionsForProcessPhase(appName, processType, phase, options); err != nil { - return err - } - } - } - return nil + return updateDockerOptionsForProcessPhases(appName, processTypes, phases, func(options []string) []string { + return append(options, option) + }) } // GetDockerOptionsForPhase returns the docker options stored under the default @@ -290,30 +260,15 @@ func RemoveDockerOptionFromPhases(appName string, phases []string, option string // non-canonical form still matches the canonically re-serialized string the // CLI hands down. func RemoveDockerOptionFromProcessPhases(appName string, processTypes []string, phases []string, option string) error { - if len(processTypes) == 0 { - processTypes = []string{DefaultProcessType} - } - for _, processType := range processTypes { - for _, phase := range phases { - options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) - if err != nil { - return err - } - - newOptions := []string{} - for _, opt := range options { - if !optionsEqual(opt, option) { - newOptions = append(newOptions, opt) - } - } - - sort.Strings(newOptions) - if err := writeDockerOptionsForProcessPhase(appName, processType, phase, newOptions); err != nil { - return err + return updateDockerOptionsForProcessPhases(appName, processTypes, phases, func(options []string) []string { + newOptions := []string{} + for _, opt := range options { + if !optionsEqual(opt, option) { + newOptions = append(newOptions, opt) } } - } - return nil + return newOptions + }) } // GetSpecifiedDockerOptionsForPhase returns the docker options for the specified @@ -410,6 +365,31 @@ func splitPropertyKey(key string) (processType, phase string, ok bool) { return processType, phase, true } -func writeDockerOptionsForProcessPhase(appName, processType, phase string, options []string) error { - return common.PropertyListWrite("docker-options", appName, propertyKey(processType, phase), options) +// updateDockerOptionsForProcessPhases rewrites the stored options of every +// process type and phase combination with the sorted result of update, holding +// the app's docker-options property lock across every read and write so +// concurrent updates to the same app cannot drop each other's options. An +// empty processTypes list is treated as the default scope. +func updateDockerOptionsForProcessPhases(appName string, processTypes []string, phases []string, update func(options []string) []string) error { + if len(processTypes) == 0 { + processTypes = []string{DefaultProcessType} + } + + return common.PropertyLock("docker-options", appName, func(p *common.LockedProperties) error { + for _, processType := range processTypes { + for _, phase := range phases { + options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) + if err != nil { + return err + } + + options = update(options) + sort.Strings(options) + if err := p.ListWrite(propertyKey(processType, phase), options); err != nil { + return err + } + } + } + return nil + }) } diff --git a/plugins/docker-options/functions.go b/plugins/docker-options/functions.go index 33ba8bfa4..17a55b7f1 100644 --- a/plugins/docker-options/functions.go +++ b/plugins/docker-options/functions.go @@ -390,36 +390,42 @@ func migrateTraefikLabelBackticks() error { } for _, appName := range apps { - properties, err := common.PropertyGetAll("docker-options", appName) - if err != nil { - return err - } - - for key := range properties { - processType, phase, ok := splitPropertyKey(key) - if !ok { - continue - } - - options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) + err := common.PropertyLock("docker-options", appName, func(p *common.LockedProperties) error { + properties, err := common.PropertyGetAll("docker-options", appName) if err != nil { return err } - changed := false - for i, option := range options { - if fixed, ok := repairTraefikLabelBackticks(option); ok { - options[i] = fixed - changed = true + for key := range properties { + processType, phase, ok := splitPropertyKey(key) + if !ok { + continue } - } - if changed { - if err := writeDockerOptionsForProcessPhase(appName, processType, phase, options); err != nil { + options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) + if err != nil { return err } - common.LogInfo1(fmt.Sprintf("Repaired docker-options label backticks for %s (%s %s)", appName, processType, phase)) + + changed := false + for i, option := range options { + if fixed, ok := repairTraefikLabelBackticks(option); ok { + options[i] = fixed + changed = true + } + } + + if changed { + if err := p.ListWrite(key, options); err != nil { + return err + } + common.LogInfo1(fmt.Sprintf("Repaired docker-options label backticks for %s (%s %s)", appName, processType, phase)) + } } + return nil + }) + if err != nil { + return err } } @@ -494,31 +500,37 @@ func migrateNonCanonicalOptions() error { } for _, appName := range apps { - properties, err := common.PropertyGetAll("docker-options", appName) - if err != nil { - return err - } - - for key := range properties { - processType, phase, ok := splitPropertyKey(key) - if !ok { - continue - } - - options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) + err := common.PropertyLock("docker-options", appName, func(p *common.LockedProperties) error { + properties, err := common.PropertyGetAll("docker-options", appName) if err != nil { return err } - canonical := canonicalizeOptionLines(options) - if slices.Equal(canonical, options) { - continue - } + for key := range properties { + processType, phase, ok := splitPropertyKey(key) + if !ok { + continue + } - if err := writeDockerOptionsForProcessPhase(appName, processType, phase, canonical); err != nil { - return err + options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) + if err != nil { + return err + } + + canonical := canonicalizeOptionLines(options) + if slices.Equal(canonical, options) { + continue + } + + if err := p.ListWrite(key, canonical); err != nil { + return err + } + common.LogInfo1(fmt.Sprintf("Rewrote docker-options into canonical form for %s (%s %s)", appName, processType, phase)) } - common.LogInfo1(fmt.Sprintf("Rewrote docker-options into canonical form for %s (%s %s)", appName, processType, phase)) + return nil + }) + if err != nil { + return err } } diff --git a/plugins/git/functions b/plugins/git/functions index 2b23a75e9..277017369 100755 --- a/plugins/git/functions +++ b/plugins/git/functions @@ -175,11 +175,22 @@ fn-git-build-with-lock-held() { (git_build "$APP" "$REV") || exit_code="$?" if [[ "$exit_code" -ne 0 ]] && [[ "$has_source_image" == "true" ]]; then - fn-plugin-property-write "git" "$APP" "source-image" "$previous_source_image" + fn-plugin-property-lock "git" "$APP" fn-git-restore-source-image "$APP" "$source_image" "$previous_source_image" fi return "$exit_code" } +fn-git-restore-source-image() { + declare desc="restores the previous source-image unless it was changed since the failed build set it, expecting the caller to hold the git property lock" + declare APP="$1" BUILD_SOURCE_IMAGE="$2" PREVIOUS_SOURCE_IMAGE="$3" + + if [[ "$(fn-git-source-image "$APP")" != "$BUILD_SOURCE_IMAGE" ]]; then + return + fi + + fn-plugin-property-write "git" "$APP" "source-image" "$PREVIOUS_SOURCE_IMAGE" +} + cmd-git-upload-pack() { declare desc="executes git-upload-pack" declare cmd="git-upload-pack" diff --git a/plugins/nginx-vhosts/functions b/plugins/nginx-vhosts/functions index abd3da343..21eeb9351 100755 --- a/plugins/nginx-vhosts/functions +++ b/plugins/nginx-vhosts/functions @@ -428,6 +428,20 @@ fn-nginx-vhosts-pre-validate-custom-template() { fi } +fn-nginx-vhosts-install-config() { + declare desc="atomically replaces the nginx.conf for an app with a rendered config" + declare APP="$1" RENDERED_CONF="$2" + local STAGED_CONF + + STAGED_CONF="$(mktemp "$DOKKU_ROOT/$APP/.nginx.conf.XXXXXX")" + if ! cat "$RENDERED_CONF" >"$STAGED_CONF"; then + rm -f "$STAGED_CONF" + return 1 + fi + mv -f "$STAGED_CONF" "$DOKKU_ROOT/$APP/nginx.conf" + rm -f "$RENDERED_CONF" +} + nginx_build_config() { declare desc="build nginx config to proxy app containers using sigil" declare APP="$1" DOKKU_APP_LISTEN_PORT="$2" DOKKU_APP_LISTEN_IP="$3" @@ -545,7 +559,7 @@ nginx_build_config() { dokku_log_info1 "Creating $SCHEME nginx.conf" mkdir -p "$DOKKU_ROOT/$APP/nginx.conf.d" - mv "$NGINX_CONF" "$DOKKU_ROOT/$APP/nginx.conf" + fn-nginx-vhosts-install-config "$APP" "$NGINX_CONF" if [[ "$IS_DEPLOYED_WITH_LISTENERS" == "true" ]]; then fn-nginx-vhosts-manage-hsts "$APP" "$SSL_INUSE" diff --git a/plugins/ports/functions.go b/plugins/ports/functions.go index afeaf7120..3462ec2e0 100644 --- a/plugins/ports/functions.go +++ b/plugins/ports/functions.go @@ -15,10 +15,18 @@ import ( // addPortMaps adds port mappings to an app func addPortMaps(appName string, portMaps []PortMap) error { + return common.PropertyLock("ports", appName, func(p *common.LockedProperties) error { + return addPortMapsWithLock(p, appName, portMaps) + }) +} + +// addPortMapsWithLock adds port mappings to an app. The caller must hold the +// app's ports property lock. +func addPortMapsWithLock(p *common.LockedProperties, appName string, portMaps []PortMap) error { allPortMaps := getPortMaps(appName) allPortMaps = append(allPortMaps, portMaps...) - return setPortMaps(appName, allPortMaps) + return writePortMaps(p, allPortMaps) } // clearPorts clears all port mappings for an app @@ -365,6 +373,14 @@ func parsePortMapString(stringPortMap string) ([]PortMap, error) { // removePortMaps removes specific port mappings from an app func removePortMaps(appName string, portMaps []PortMap) error { + return common.PropertyLock("ports", appName, func(p *common.LockedProperties) error { + return removePortMapsWithLock(p, appName, portMaps) + }) +} + +// removePortMapsWithLock removes specific port mappings from an app. The caller +// must hold the app's ports property lock. +func removePortMapsWithLock(p *common.LockedProperties, appName string, portMaps []PortMap) error { toRemove := map[string]bool{} toRemoveByPort := map[int]bool{} @@ -390,10 +406,10 @@ func removePortMaps(appName string, portMaps []PortMap) error { } if len(toSet) == 0 { - return common.PropertyDelete("ports", appName, "map") + return p.Delete("map") } - return setPortMaps(appName, toSet) + return writePortMaps(p, toSet) } // reusesSchemeHostPort returns true if the port maps reuse the same scheme:host-port @@ -413,6 +429,14 @@ func reusesSchemeHostPort(portMaps []PortMap) error { // setPortMaps sets the port maps for an app func setPortMaps(appName string, portMaps []PortMap) error { + return common.PropertyLock("ports", appName, func(p *common.LockedProperties) error { + return writePortMaps(p, portMaps) + }) +} + +// writePortMaps writes the port maps for an app. The caller must hold the +// app's ports property lock. +func writePortMaps(p *common.LockedProperties, portMaps []PortMap) error { if err := reusesSchemeHostPort(portMaps); err != nil { return fmt.Errorf("Error validating port mappings: %s", err) } @@ -427,7 +451,7 @@ func setPortMaps(appName string, portMaps []PortMap) error { } sort.Strings(value) - return common.PropertyListWrite("ports", appName, "map", value) + return p.ListWrite("map", value) } // setProxyPort sets the proxy port for an app diff --git a/plugins/ports/triggers.go b/plugins/ports/triggers.go index 9596ae174..9a9b2a60a 100644 --- a/plugins/ports/triggers.go +++ b/plugins/ports/triggers.go @@ -160,11 +160,21 @@ func TriggerPostCertsRemove(appName string) error { return err } - return removePortMaps(appName, filterAppPortMaps(appName, "https", 443)) + return common.PropertyLock("ports", appName, func(p *common.LockedProperties) error { + return removePortMapsWithLock(p, appName, filterAppPortMaps(appName, "https", 443)) + }) } // TriggerPostCertsUpdate sets port properties after SSL cert is added func TriggerPostCertsUpdate(appName string) error { + return common.PropertyLock("ports", appName, func(p *common.LockedProperties) error { + return postCertsUpdateWithLock(p, appName) + }) +} + +// postCertsUpdateWithLock adds https:443 port maps mirroring the app's http:80 +// port maps. The caller must hold the app's ports property lock. +func postCertsUpdateWithLock(p *common.LockedProperties, appName string) error { port := common.PropertyGet("proxy", appName, "proxy-port") sslPort := common.PropertyGet("proxy", appName, "proxy-ssl-port") portMaps := getPortMaps(appName) @@ -201,7 +211,7 @@ func TriggerPostCertsUpdate(appName string) error { }) } - if err := addPortMaps(appName, toAdd); err != nil { + if err := addPortMapsWithLock(p, appName, toAdd); err != nil { return err } } diff --git a/plugins/proxy/command-functions b/plugins/proxy/command-functions index 6343b3bf8..b46f9a409 100755 --- a/plugins/proxy/command-functions +++ b/plugins/proxy/command-functions @@ -1,5 +1,6 @@ #!/usr/bin/env bash source "$PLUGIN_CORE_AVAILABLE_PATH/common/functions" +source "$PLUGIN_CORE_AVAILABLE_PATH/common/property-functions" source "$PLUGIN_AVAILABLE_PATH/proxy/functions" set -eo pipefail [[ $DOKKU_TRACE ]] && set -x @@ -32,12 +33,7 @@ cmd-proxy-labels-add() { [[ -z "$PASSED_LABEL_VALUE" ]] && dokku_log_fail "Please specify a value for the container label" verify_app_name "$APP" - local proxy_labels_file_path="$(fn-proxy-get-labels-file-path "$PROXY" "$APP")" - touch "$proxy_labels_file_path" - cmd-proxy-labels-remove "$PROXY" "$APP" "$PASSED_LABEL_NAME" >/dev/null - echo "${PASSED_LABEL_NAME}=${PASSED_LABEL_VALUE}" >>"$proxy_labels_file_path" - local all_proxy_labels="$(<"$proxy_labels_file_path")" - echo -e "${all_proxy_labels}" | sed '/^$/d' | sort -u >"$proxy_labels_file_path" + fn-plugin-property-lock "$PROXY" "$APP" fn-proxy-labels-add "$PROXY" "$APP" "$PASSED_LABEL_NAME" "$PASSED_LABEL_VALUE" } cmd-proxy-labels-remove() { @@ -55,8 +51,5 @@ cmd-proxy-labels-remove() { return fi - local all_proxy_labels="$(<"$proxy_labels_file_path")" - local escaped_label_name="$(fn-proxy-escape-extended-sed "$PASSED_LABEL_NAME")" - local all_proxy_labels="$(echo -e "${all_proxy_labels}" | sed -E "s/^${escaped_label_name}=.*\$//")" - echo -e "${all_proxy_labels}" | sed '/^$/d' | sort -u >"$proxy_labels_file_path" + fn-plugin-property-lock "$PROXY" "$APP" fn-proxy-labels-remove "$PROXY" "$APP" "$PASSED_LABEL_NAME" } diff --git a/plugins/proxy/functions b/plugins/proxy/functions index f73692b1d..c51192e98 100755 --- a/plugins/proxy/functions +++ b/plugins/proxy/functions @@ -56,3 +56,57 @@ fn-proxy-show-label() { echo "$proxy_label_value" } + +fn-proxy-labels-add() { + declare desc="adds a container label for the specified proxy, expecting the caller to hold the proxy property lock" + declare PROXY="$1" APP="$2" LABEL_NAME="$3" LABEL_VALUE="$4" + local proxy_labels_file_path="$(fn-proxy-get-labels-file-path "$PROXY" "$APP")" + local all_proxy_labels="" + + if [[ -s "$proxy_labels_file_path" ]]; then + all_proxy_labels="$(fn-proxy-labels-without "$proxy_labels_file_path" "$LABEL_NAME")" + fi + + all_proxy_labels="${all_proxy_labels}"$'\n'"${LABEL_NAME}=${LABEL_VALUE}" + echo -e "${all_proxy_labels}" | sed '/^$/d' | sort -u | fn-proxy-write-labels-file "$proxy_labels_file_path" +} + +fn-proxy-labels-remove() { + declare desc="removes a container label for the specified proxy, expecting the caller to hold the proxy property lock" + declare PROXY="$1" APP="$2" LABEL_NAME="$3" + local proxy_labels_file_path="$(fn-proxy-get-labels-file-path "$PROXY" "$APP")" + + if [[ ! -s "$proxy_labels_file_path" ]]; then + return + fi + + fn-proxy-labels-without "$proxy_labels_file_path" "$LABEL_NAME" | fn-proxy-write-labels-file "$proxy_labels_file_path" +} + +fn-proxy-labels-without() { + declare desc="prints the sorted labels in a proxy labels file without the named label" + declare LABELS_FILE="$1" LABEL_NAME="$2" + local all_proxy_labels="$(<"$LABELS_FILE")" + local escaped_label_name="$(fn-proxy-escape-extended-sed "$LABEL_NAME")" + + echo -e "${all_proxy_labels}" | sed -E "s/^${escaped_label_name}=.*\$//" | sed '/^$/d' | sort -u +} + +fn-proxy-write-labels-file() { + declare desc="atomically replaces a proxy labels file with the contents of stdin" + declare LABELS_FILE="$1" + local tmp_file + + tmp_file="$(mktemp "$(dirname "$LABELS_FILE")/.labels.XXXXXX")" + if ! cat >"$tmp_file"; then + rm -f "$tmp_file" + return 1 + fi + + if [[ -f "$LABELS_FILE" ]]; then + chmod --reference="$LABELS_FILE" "$tmp_file" + else + chmod 0644 "$tmp_file" + fi + mv -f "$tmp_file" "$LABELS_FILE" +} diff --git a/plugins/ps/concurrency_test.go b/plugins/ps/concurrency_test.go new file mode 100644 index 000000000..a544ae3db --- /dev/null +++ b/plugins/ps/concurrency_test.go @@ -0,0 +1,51 @@ +package ps + +import ( + "errors" + "fmt" + "os/user" + "sync" + "testing" + + "github.com/dokku/dokku/plugins/common" +) + +func TestUpdateScaleConcurrent(t *testing.T) { + t.Setenv("DOKKU_LIB_ROOT", t.TempDir()) + current, err := user.Current() + if err != nil { + t.Fatalf("user.Current: %v", err) + } + group, err := user.LookupGroupId(current.Gid) + if err != nil { + t.Fatalf("user.LookupGroupId: %v", err) + } + t.Setenv("DOKKU_SYSTEM_USER", current.Username) + t.Setenv("DOKKU_SYSTEM_GROUP", group.Name) + + const appName = "race-test" + const count = 20 + var wg sync.WaitGroup + errs := make([]error, count) + for i := 0; i < count; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + errs[i] = updateScale(appName, false, FormationSlice{ + {ProcessType: fmt.Sprintf("worker%d", i), Quantity: 1}, + }) + }(i) + } + wg.Wait() + if err := errors.Join(errs...); err != nil { + t.Fatalf("updateScale: %v", err) + } + + tuples, err := common.PropertyListGet("ps", appName, "scale") + if err != nil { + t.Fatalf("PropertyListGet: %v", err) + } + if len(tuples) != count { + t.Fatalf("scale has %d process types, want %d: %v", len(tuples), count, tuples) + } +} diff --git a/plugins/ps/functions.go b/plugins/ps/functions.go index 03a4567df..c64224354 100644 --- a/plugins/ps/functions.go +++ b/plugins/ps/functions.go @@ -411,7 +411,18 @@ func scaleSet(input scaleSetInput) error { return nil } +// updateScale merges formationUpdates into the stored formation, holding the +// app's ps property lock from the read of the stored formation to the write of +// the merged one so concurrent scale changes cannot drop each other. func updateScale(appName string, clearExisting bool, formationUpdates FormationSlice) error { + return common.PropertyLock("ps", appName, func(p *common.LockedProperties) error { + return updateScaleWithLock(p, appName, clearExisting, formationUpdates) + }) +} + +// updateScaleWithLock merges formationUpdates into the stored formation. The +// caller must hold the app's ps property lock. +func updateScaleWithLock(p *common.LockedProperties, appName string, clearExisting bool, formationUpdates FormationSlice) error { formations := FormationSlice{} if !clearExisting { processTuples, err := common.PropertyListGet("ps", appName, "scale") @@ -482,11 +493,11 @@ func updateScale(appName string, clearExisting bool, formationUpdates FormationS values = append(values, fmt.Sprintf("%s=%d", formation.ProcessType, formation.Quantity)) } - if err := common.PropertyListWrite("ps", appName, "scale.old", oldValues); err != nil { + if err := p.ListWrite("scale.old", oldValues); err != nil { return err } - return common.PropertyListWrite("ps", appName, "scale", values) + return p.ListWrite("scale", values) } // validateFormations returns an error when a user specified formation cannot be scaled to. diff --git a/plugins/registry/concurrency_test.go b/plugins/registry/concurrency_test.go new file mode 100644 index 000000000..561e98631 --- /dev/null +++ b/plugins/registry/concurrency_test.go @@ -0,0 +1,46 @@ +package registry + +import ( + "errors" + "os/user" + "sort" + "sync" + "testing" +) + +func TestIncrementTagVersionConcurrent(t *testing.T) { + t.Setenv("DOKKU_LIB_ROOT", t.TempDir()) + current, err := user.Current() + if err != nil { + t.Fatalf("user.Current: %v", err) + } + group, err := user.LookupGroupId(current.Gid) + if err != nil { + t.Fatalf("user.LookupGroupId: %v", err) + } + t.Setenv("DOKKU_SYSTEM_USER", current.Username) + t.Setenv("DOKKU_SYSTEM_GROUP", group.Name) + + const count = 20 + var wg sync.WaitGroup + versions := make([]int, count) + errs := make([]error, count) + for i := 0; i < count; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + versions[i], errs[i] = incrementTagVersion("race-test") + }(i) + } + wg.Wait() + if err := errors.Join(errs...); err != nil { + t.Fatalf("incrementTagVersion: %v", err) + } + + sort.Ints(versions) + for i, version := range versions { + if version != i+1 { + t.Fatalf("versions = %v, want 1 through %d with no duplicates", versions, count) + } + } +} diff --git a/plugins/registry/functions.go b/plugins/registry/functions.go index bd8d8098c..7e531f41d 100644 --- a/plugins/registry/functions.go +++ b/plugins/registry/functions.go @@ -123,21 +123,28 @@ func isPushEnabled(appName string) bool { return reportComputedPushOnRelease(appName) == "true" } +// incrementTagVersion bumps the app's tag version and returns the new value, +// holding the app's registry property lock across the read and the write so +// concurrent releases never hand out the same version. func incrementTagVersion(appName string) (int, error) { - tag := common.PropertyGet("registry", appName, "tag-version") - if tag == "" { - tag = "0" - } + version := 0 + err := common.PropertyLock("registry", appName, func(p *common.LockedProperties) error { + tag := common.PropertyGet("registry", appName, "tag-version") + if tag == "" { + tag = "0" + } - tag = strings.TrimSpace(tag) - version, err := strconv.Atoi(tag) + tag = strings.TrimSpace(tag) + current, err := strconv.Atoi(tag) + if err != nil { + return fmt.Errorf("Unable to convert existing tag version (%s) to integer: %v", tag, err) + } + + version = current + 1 + common.LogVerboseQuiet(fmt.Sprintf("Bumping tag to %d", version)) + return p.Write("tag-version", strconv.Itoa(version)) + }) if err != nil { - return 0, fmt.Errorf("Unable to convert existing tag version (%s) to integer: %v", tag, err) - } - - version++ - common.LogVerboseQuiet(fmt.Sprintf("Bumping tag to %d", version)) - if err = common.PropertyWrite("registry", appName, "tag-version", strconv.Itoa(version)); err != nil { return 0, err } diff --git a/plugins/registry/subcommands.go b/plugins/registry/subcommands.go index 471788515..9d13d86ce 100644 --- a/plugins/registry/subcommands.go +++ b/plugins/registry/subcommands.go @@ -126,7 +126,7 @@ func CommandLogin(appName string, server string, username string, password strin env["DOCKER_CONFIG"] = GetAppRegistryConfigDir(appName) } - result, err := common.CallExecCommand(common.ExecCommandInput{ + result, err := execDockerConfigCommand(env["DOCKER_CONFIG"], common.ExecCommandInput{ Command: common.DockerBin(), Args: []string{"login", "--username", username, "--password-stdin", server}, Env: env, @@ -184,7 +184,7 @@ func CommandLogout(appName string, server string) error { env["DOCKER_CONFIG"] = GetAppRegistryConfigDir(appName) } - result, err := common.CallExecCommand(common.ExecCommandInput{ + result, err := execDockerConfigCommand(env["DOCKER_CONFIG"], common.ExecCommandInput{ Command: common.DockerBin(), Args: []string{"logout", server}, Env: env, @@ -226,3 +226,22 @@ func CommandSet(appName string, property string, value string) error { common.CommandPropertySet("registry", appName, property, value, DefaultProperties, GlobalProperties) return nil } + +// execDockerConfigCommand runs a docker command that rewrites the credentials +// file in configDir. docker merges the file it reads with the change it makes, +// so the directory stays locked for the whole command to keep concurrent logins +// and logouts for one app from dropping each other's credentials. An empty +// configDir means docker's own config, which is left unlocked. +func execDockerConfigCommand(configDir string, input common.ExecCommandInput) (common.ExecCommandResponse, error) { + if configDir == "" { + return common.CallExecCommand(input) + } + + var result common.ExecCommandResponse + err := common.LockDirectory(configDir, func() error { + var err error + result, err = common.CallExecCommand(input) + return err + }) + return result, err +} diff --git a/plugins/scheduler-k3s/subcommands.go b/plugins/scheduler-k3s/subcommands.go index 4edecbec9..a6e506fb8 100644 --- a/plugins/scheduler-k3s/subcommands.go +++ b/plugins/scheduler-k3s/subcommands.go @@ -256,10 +256,22 @@ func CommandAutoscalingAuthSet(appName string, trigger string, metadata map[stri return fmt.Errorf("Must specify at least one --metadata flag, omit --replace to remove all metadata for the trigger") } - if len(metadata) == 0 || replace { - if err := deleteTriggerAuthProperties(appName, trigger); err != nil { - return err + err := common.PropertyLock("scheduler-k3s", appName, func(p *common.LockedProperties) error { + if len(metadata) == 0 || replace { + if err := deleteTriggerAuthProperties(p, appName, trigger); err != nil { + return err + } } + + for key, value := range metadata { + if err := p.Write(fmt.Sprintf("%s%s.%s", TriggerAuthPropertyPrefix, trigger, key), value); err != nil { + return fmt.Errorf("Unable to set property: %w", err) + } + } + return nil + }) + if err != nil { + return err } if len(metadata) == 0 { @@ -278,12 +290,6 @@ func CommandAutoscalingAuthSet(appName string, trigger string, metadata map[stri return nil } - for key, value := range metadata { - if err := common.PropertyWrite("scheduler-k3s", appName, fmt.Sprintf("%s%s.%s", TriggerAuthPropertyPrefix, trigger, key), value); err != nil { - return fmt.Errorf("Unable to set property: %w", err) - } - } - common.LogInfo1("Trigger authentication settings saved") common.LogVerbose("Resources will be created or updated on next deploy") @@ -297,15 +303,16 @@ func CommandAutoscalingAuthSet(appName string, trigger string, metadata map[stri return nil } -// deleteTriggerAuthProperties removes every stored metadata key for one trigger. -func deleteTriggerAuthProperties(appName string, trigger string) error { +// deleteTriggerAuthProperties removes every stored metadata key for one +// trigger. The caller must hold the app's scheduler-k3s property lock. +func deleteTriggerAuthProperties(p *common.LockedProperties, appName string, trigger string) error { properties, err := common.PropertyGetAllByPrefix("scheduler-k3s", appName, fmt.Sprintf("%s%s.", TriggerAuthPropertyPrefix, trigger)) if err != nil { return fmt.Errorf("Unable to get property list: %w", err) } for key := range properties { - if err := common.PropertyDelete("scheduler-k3s", appName, key); err != nil { + if err := p.Delete(key); err != nil { return fmt.Errorf("Unable to delete property: %w", err) } } diff --git a/plugins/scheduler-k3s/triggers.go b/plugins/scheduler-k3s/triggers.go index c528400d7..1aeb345a2 100644 --- a/plugins/scheduler-k3s/triggers.go +++ b/plugins/scheduler-k3s/triggers.go @@ -103,36 +103,38 @@ func TriggerInstall() error { // chart, and legacy values take precedence over any partial pre-existing // entries on re-runs. func migrateChartPropertiesToMapFormat() error { - for _, chart := range HelmCharts { - prefix := "chart." + chart.ReleaseName + "." - legacy, err := common.PropertyGetAllByPrefix("scheduler-k3s", "--global", prefix) - if err != nil { - return fmt.Errorf("Unable to read legacy chart properties for %s: %w", chart.ReleaseName, err) - } - if len(legacy) == 0 { - continue - } + return common.PropertyLock("scheduler-k3s", "--global", func(p *common.LockedProperties) error { + for _, chart := range HelmCharts { + prefix := "chart." + chart.ReleaseName + "." + legacy, err := common.PropertyGetAllByPrefix("scheduler-k3s", "--global", prefix) + if err != nil { + return fmt.Errorf("Unable to read legacy chart properties for %s: %w", chart.ReleaseName, err) + } + if len(legacy) == 0 { + continue + } - mapProperty := "chart-overrides." + chart.ReleaseName - merged, err := common.PropertyMapGet("scheduler-k3s", "--global", mapProperty) - if err != nil { - return fmt.Errorf("Unable to read existing chart-overrides for %s: %w", chart.ReleaseName, err) - } - for fullKey, value := range legacy { - merged[strings.TrimPrefix(fullKey, prefix)] = value - } + mapProperty := "chart-overrides." + chart.ReleaseName + merged, err := common.PropertyMapGet("scheduler-k3s", "--global", mapProperty) + if err != nil { + return fmt.Errorf("Unable to read existing chart-overrides for %s: %w", chart.ReleaseName, err) + } + for fullKey, value := range legacy { + merged[strings.TrimPrefix(fullKey, prefix)] = value + } - if err := common.PropertyMapWrite("scheduler-k3s", "--global", mapProperty, merged); err != nil { - return fmt.Errorf("Unable to write chart-overrides for %s: %w", chart.ReleaseName, err) - } + if err := p.MapWrite(mapProperty, merged); err != nil { + return fmt.Errorf("Unable to write chart-overrides for %s: %w", chart.ReleaseName, err) + } - for fullKey := range legacy { - if err := common.PropertyDelete("scheduler-k3s", "--global", fullKey); err != nil { - return fmt.Errorf("Unable to remove legacy chart property %s: %w", fullKey, err) + for fullKey := range legacy { + if err := p.Delete(fullKey); err != nil { + return fmt.Errorf("Unable to remove legacy chart property %s: %w", fullKey, err) + } } } - } - return nil + return nil + }) } // migrateAnnotationsLabelsToMapFormat converts the legacy line-formatted @@ -207,31 +209,33 @@ func migrateAnnotationsLabelsToMapFormat() error { // file as a JSON map. It is a no-op when the file is already valid JSON or // empty (idempotent). func migrateAnnotationLabelProperty(scope string, propertyName string) error { - if _, err := common.PropertyMapGet("scheduler-k3s", scope, propertyName); err == nil { + return common.PropertyLock("scheduler-k3s", scope, func(p *common.LockedProperties) error { + if _, err := common.PropertyMapGet("scheduler-k3s", scope, propertyName); err == nil { + return nil + } + + lines, err := common.PropertyListGet("scheduler-k3s", scope, propertyName) + if err != nil { + return fmt.Errorf("Unable to read legacy property %s/%s: %w", scope, propertyName, err) + } + + m := map[string]string{} + for _, line := range lines { + if line == "" { + continue + } + parts := strings.SplitN(line, ": ", 2) + if len(parts) != 2 { + return fmt.Errorf("Invalid legacy entry in %s/%s: %s", scope, propertyName, line) + } + m[parts[0]] = parts[1] + } + + if err := p.MapWrite(propertyName, m); err != nil { + return fmt.Errorf("Unable to write %s/%s as map: %w", scope, propertyName, err) + } return nil - } - - lines, err := common.PropertyListGet("scheduler-k3s", scope, propertyName) - if err != nil { - return fmt.Errorf("Unable to read legacy property %s/%s: %w", scope, propertyName, err) - } - - m := map[string]string{} - for _, line := range lines { - if line == "" { - continue - } - parts := strings.SplitN(line, ": ", 2) - if len(parts) != 2 { - return fmt.Errorf("Invalid legacy entry in %s/%s: %s", scope, propertyName, line) - } - m[parts[0]] = parts[1] - } - - if err := common.PropertyMapWrite("scheduler-k3s", scope, propertyName, m); err != nil { - return fmt.Errorf("Unable to write %s/%s as map: %w", scope, propertyName, err) - } - return nil + }) } // TriggerPostCertsUpdate handles post-certs-update trigger diff --git a/plugins/storage/attachment.go b/plugins/storage/attachment.go index 3fde0f01d..f2b5c687d 100644 --- a/plugins/storage/attachment.go +++ b/plugins/storage/attachment.go @@ -105,6 +105,11 @@ func LoadAttachments(appName string) ([]*Attachment, error) { return nil, fmt.Errorf("unable to read storage attachments for %q: %w", appName, err) } + return decodeAttachments(appName, lines) +} + +// decodeAttachments parses the stored attachment lines of an app. +func decodeAttachments(appName string, lines []string) ([]*Attachment, error) { attachments := []*Attachment{} for _, line := range lines { line = strings.TrimSpace(line) @@ -122,15 +127,44 @@ func LoadAttachments(appName string) ([]*Attachment, error) { // SaveAttachments overwrites the entire attachment list for an app. func SaveAttachments(appName string, attachments []*Attachment) error { + lines, err := encodeAttachments(appName, attachments) + if err != nil { + return err + } + return common.PropertyListWrite(PluginName, appName, AttachmentsProperty, lines) +} + +// UpdateAttachments loads the attachment list for an app, passes it to fn and +// saves the list fn returns, holding the app's storage property lock across +// the read and the write so concurrent mounts and unmounts cannot drop each +// other's changes. Nothing is saved when fn returns an error. +func UpdateAttachments(appName string, fn func(attachments []*Attachment) ([]*Attachment, error)) error { + return common.PropertyListUpdate(PluginName, appName, AttachmentsProperty, func(lines []string) ([]string, error) { + attachments, err := decodeAttachments(appName, lines) + if err != nil { + return nil, err + } + + attachments, err = fn(attachments) + if err != nil { + return nil, err + } + + return encodeAttachments(appName, attachments) + }) +} + +// encodeAttachments serializes attachments into stored attachment lines. +func encodeAttachments(appName string, attachments []*Attachment) ([]string, error) { lines := make([]string, 0, len(attachments)) for _, attachment := range attachments { data, err := json.Marshal(attachment) if err != nil { - return fmt.Errorf("unable to encode storage attachment for %q: %w", appName, err) + return nil, fmt.Errorf("unable to encode storage attachment for %q: %w", appName, err) } lines = append(lines, string(data)) } - return common.PropertyListWrite(PluginName, appName, AttachmentsProperty, lines) + return lines, nil } // ensureContainerPathFree rejects a candidate whose container path is already @@ -161,26 +195,22 @@ func AddAttachment(appName string, attachment *Attachment) error { return err } - attachments, err := LoadAttachments(appName) - if err != nil { - return err - } - - for _, existing := range attachments { - if existing.EntryName == attachment.EntryName && - existing.ContainerPath == attachment.ContainerPath && - existing.EffectiveProcessType() == attachment.EffectiveProcessType() { - return fmt.Errorf("storage entry %q is already mounted at %q for process type %q on app %q", - attachment.EntryName, attachment.ContainerPath, attachment.EffectiveProcessType(), appName) + return UpdateAttachments(appName, func(attachments []*Attachment) ([]*Attachment, error) { + for _, existing := range attachments { + if existing.EntryName == attachment.EntryName && + existing.ContainerPath == attachment.ContainerPath && + existing.EffectiveProcessType() == attachment.EffectiveProcessType() { + return nil, fmt.Errorf("storage entry %q is already mounted at %q for process type %q on app %q", + attachment.EntryName, attachment.ContainerPath, attachment.EffectiveProcessType(), appName) + } } - } - if err := ensureContainerPathFree(appName, attachments, attachment); err != nil { - return err - } + if err := ensureContainerPathFree(appName, attachments, attachment); err != nil { + return nil, err + } - attachments = append(attachments, attachment) - return SaveAttachments(appName, attachments) + return append(attachments, attachment), nil + }) } // UpsertAttachment inserts an attachment, or updates the mount-time fields @@ -201,49 +231,43 @@ func UpsertAttachment(appName string, attachment *Attachment) (bool, error) { return false, err } - attachments, err := LoadAttachments(appName) + created := false + err := UpdateAttachments(appName, func(attachments []*Attachment) ([]*Attachment, error) { + for _, existing := range attachments { + if existing.EntryName == attachment.EntryName && + existing.ContainerPath == attachment.ContainerPath && + existing.EffectiveProcessType() == attachment.EffectiveProcessType() { + existing.Phases = attachment.Phases + existing.Subpath = attachment.Subpath + existing.Readonly = attachment.Readonly + existing.VolumeOptions = attachment.VolumeOptions + existing.VolumeChown = attachment.VolumeChown + return attachments, nil + } + } + + // Only the append branch needs the overlap check: rewriting an existing + // tuple in place would otherwise trip over the attachment it is updating. + if err := ensureContainerPathFree(appName, attachments, attachment); err != nil { + return nil, err + } + + created = true + return append(attachments, attachment), nil + }) if err != nil { return false, err } - - for _, existing := range attachments { - if existing.EntryName == attachment.EntryName && - existing.ContainerPath == attachment.ContainerPath && - existing.EffectiveProcessType() == attachment.EffectiveProcessType() { - existing.Phases = attachment.Phases - existing.Subpath = attachment.Subpath - existing.Readonly = attachment.Readonly - existing.VolumeOptions = attachment.VolumeOptions - existing.VolumeChown = attachment.VolumeChown - return false, SaveAttachments(appName, attachments) - } - } - - // Only the append branch needs the overlap check: rewriting an existing - // tuple in place would otherwise trip over the attachment it is updating. - if err := ensureContainerPathFree(appName, attachments, attachment); err != nil { - return false, err - } - - attachments = append(attachments, attachment) - return true, SaveAttachments(appName, attachments) + return created, nil } // RemoveAttachment removes the attachment matching the given entry and // optional container path. If containerPath is empty and there is more // than one match, returns an error. func RemoveAttachment(appName string, entryName string, containerPath string) error { - attachments, err := LoadAttachments(appName) - if err != nil { - return err - } - - keep, err := removeMatchingAttachments(attachments, appName, entryName, containerPath) - if err != nil { - return err - } - - return SaveAttachments(appName, keep) + return UpdateAttachments(appName, func(attachments []*Attachment) ([]*Attachment, error) { + return removeMatchingAttachments(attachments, appName, entryName, containerPath) + }) } // removeMatchingAttachments returns the given list with the attachments diff --git a/plugins/storage/commands_entries.go b/plugins/storage/commands_entries.go index e35a6afcd..20248f066 100644 --- a/plugins/storage/commands_entries.go +++ b/plugins/storage/commands_entries.go @@ -274,32 +274,27 @@ func CommandSet(input CommandSetInput) error { if !EntryExists(input.Name) { return fmt.Errorf("storage entry %q does not exist", input.Name) } - entry, err := LoadEntry(input.Name) - if err != nil { - return err - } - touchesDirectory := false - for _, change := range input.Changes { - if err := applyPropertyChange(entry, change); err != nil { - return err + entry, err := UpdateEntry(input.Name, func(entry *Entry) error { + for _, change := range input.Changes { + if err := applyPropertyChange(entry, change); err != nil { + return err + } + if change.Property == "chown" || change.Property == "mode" { + touchesDirectory = true + } } - if change.Property == "chown" || change.Property == "mode" { - touchesDirectory = true + + if input.Annotations != nil { + entry.Annotations = input.Annotations + } + if input.Labels != nil { + entry.Labels = input.Labels } - } - if input.Annotations != nil { - entry.Annotations = input.Annotations - } - if input.Labels != nil { - entry.Labels = input.Labels - } - - if err := entry.Validate(); err != nil { - return err - } - if err := SaveEntry(entry); err != nil { + return entry.Validate() + }) + if err != nil { return err } // Only converge the directory when the caller actually asked to change diff --git a/plugins/storage/commands_metadata.go b/plugins/storage/commands_metadata.go index 7b4b6703f..005f77e3c 100644 --- a/plugins/storage/commands_metadata.go +++ b/plugins/storage/commands_metadata.go @@ -76,32 +76,27 @@ func setEntryMapKey(field entryMapField, name string, key string, value string) return fmt.Errorf("storage entry %q does not exist", name) } - entry, err := LoadEntry(name) - if err != nil { - return err - } - - values := field.Get(entry) - if value == "" { - delete(values, key) - common.LogInfo2Quiet(fmt.Sprintf("Unsetting %s %s", field.Singular, key)) - } else { - if values == nil { - values = map[string]string{} + entry, err := UpdateEntry(name, func(entry *Entry) error { + values := field.Get(entry) + if value == "" { + delete(values, key) + common.LogInfo2Quiet(fmt.Sprintf("Unsetting %s %s", field.Singular, key)) + } else { + if values == nil { + values = map[string]string{} + } + values[key] = value + common.LogInfo2Quiet(fmt.Sprintf("Setting %s %s to %s", field.Singular, key, value)) } - values[key] = value - common.LogInfo2Quiet(fmt.Sprintf("Setting %s %s to %s", field.Singular, key, value)) - } - if len(values) == 0 { - // Drop the empty map so the omitempty tag keeps it out of the JSON. - values = nil - } - field.Set(entry, values) + if len(values) == 0 { + // Drop the empty map so the omitempty tag keeps it out of the JSON. + values = nil + } + field.Set(entry, values) - if err := entry.Validate(); err != nil { - return err - } - if err := SaveEntry(entry); err != nil { + return entry.Validate() + }) + if err != nil { return err } diff --git a/plugins/storage/concurrency_test.go b/plugins/storage/concurrency_test.go new file mode 100644 index 000000000..a6fb3d6be --- /dev/null +++ b/plugins/storage/concurrency_test.go @@ -0,0 +1,92 @@ +package storage + +import ( + "errors" + "fmt" + "sync" + "testing" + + . "github.com/onsi/gomega" +) + +func TestAddAttachmentConcurrent(t *testing.T) { + RegisterTestingT(t) + withTempLibRoot(t) + + const count = 20 + var wg sync.WaitGroup + errs := make([]error, count) + for i := 0; i < count; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + errs[i] = AddAttachment("demo", &Attachment{ + EntryName: fmt.Sprintf("demo-data-%d", i), + ContainerPath: fmt.Sprintf("/data/%d", i), + Phases: []string{PhaseDeploy, PhaseRun}, + ProcessType: DefaultProcessType, + }) + }(i) + } + wg.Wait() + Expect(errors.Join(errs...)).NotTo(HaveOccurred()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(count)) +} + +func TestUpdateEntryConcurrent(t *testing.T) { + RegisterTestingT(t) + withTempLibRoot(t) + + Expect(SaveEntry(&Entry{ + Name: "demo-data", + Scheduler: SchedulerK3s, + Size: "2Gi", + })).To(Succeed()) + + const count = 20 + var wg sync.WaitGroup + errs := make([]error, count) + for i := 0; i < count; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + _, errs[i] = UpdateEntry("demo-data", func(entry *Entry) error { + if entry.Labels == nil { + entry.Labels = map[string]string{} + } + entry.Labels[fmt.Sprintf("label-%d", i)] = "value" + return nil + }) + }(i) + } + wg.Wait() + Expect(errors.Join(errs...)).NotTo(HaveOccurred()) + + entry, err := LoadEntry("demo-data") + Expect(err).NotTo(HaveOccurred()) + Expect(entry.Labels).To(HaveLen(count)) +} + +func TestUpdateEntryErrorLeavesEntryUnchanged(t *testing.T) { + RegisterTestingT(t) + withTempLibRoot(t) + + Expect(SaveEntry(&Entry{ + Name: "demo-data", + Scheduler: SchedulerK3s, + Size: "2Gi", + })).To(Succeed()) + + _, err := UpdateEntry("demo-data", func(entry *Entry) error { + entry.Size = "5Gi" + return errors.New("boom") + }) + Expect(err).To(MatchError("boom")) + + entry, err := LoadEntry("demo-data") + Expect(err).NotTo(HaveOccurred()) + Expect(entry.Size).To(Equal("2Gi")) +} diff --git a/plugins/storage/entry.go b/plugins/storage/entry.go index 0ed9db919..2729de71f 100644 --- a/plugins/storage/entry.go +++ b/plugins/storage/entry.go @@ -67,7 +67,7 @@ var ( ) // Entry is the source of truth for a storage volume. One file per entry -// lives at $DOKKU_LIB_ROOT/config/storage/entries/.json. +// lives at $DOKKU_LIB_ROOT/data/storage-registry/entries/.json. type Entry struct { Name string `json:"name"` Scheduler string `json:"scheduler"` @@ -162,6 +162,39 @@ func SaveEntry(entry *Entry) error { return nil } +// UpdateEntry loads a registered entry, passes it to fn and saves it, holding +// the entries directory lock across the read and the write so concurrent +// edits to one entry cannot drop each other's changes. Nothing is saved when +// fn returns an error. The saved entry is returned. +func UpdateEntry(name string, fn func(entry *Entry) error) (*Entry, error) { + if err := EnsureEntriesDirectory(); err != nil { + return nil, err + } + + var updated *Entry + err := common.LockDirectory(EntriesDirectory(), func() error { + entry, err := LoadEntry(name) + if err != nil { + return err + } + + if err := fn(entry); err != nil { + return err + } + + if err := SaveEntry(entry); err != nil { + return err + } + + updated = entry + return nil + }) + if err != nil { + return nil, err + } + return updated, nil +} + // DeleteEntry removes the on-disk record for an entry. func DeleteEntry(name string) error { if err := os.Remove(entryPath(name)); err != nil && !os.IsNotExist(err) { diff --git a/plugins/storage/migrate.go b/plugins/storage/migrate.go index b72d557b7..c8182497a 100644 --- a/plugins/storage/migrate.go +++ b/plugins/storage/migrate.go @@ -217,19 +217,18 @@ func migrateMount(appName string, processType string, mount string, phases []str VolumeOptions: parsed.VolumeOptions, } - existing, err := LoadAttachments(appName) - if err != nil { - return err - } // A named scope whose mount the default scope already covers needs no // attachment of its own - the default one reaches that process anyway, and // recording both would manufacture two -v flags for one container path. // The docker-options line is still drained so it stops being emitted twice. - if !attachmentExists(existing, attachment) && !coveredByDefaultScope(existing, attachment) { - existing = append(existing, attachment) - if err := SaveAttachments(appName, existing); err != nil { - return err + err := UpdateAttachments(appName, func(existing []*Attachment) ([]*Attachment, error) { + if attachmentExists(existing, attachment) || coveredByDefaultScope(existing, attachment) { + return existing, nil } + return append(existing, attachment), nil + }) + if err != nil { + return err } mountedPhases := []string{} diff --git a/plugins/storage/subcommands.go b/plugins/storage/subcommands.go index d621b7311..f73798261 100644 --- a/plugins/storage/subcommands.go +++ b/plugins/storage/subcommands.go @@ -390,34 +390,30 @@ func replaceMounts(input CommandMountInput) error { } } - existing, err := LoadAttachments(input.AppName) + err := UpdateAttachments(input.AppName, func(existing []*Attachment) ([]*Attachment, error) { + retained := []*Attachment{} + for _, attachment := range existing { + if attachment.EffectiveProcessType() != processType { + retained = append(retained, attachment) + } + } + + // The declared set is already unique on container path within its own + // scope; what is left to check is the scopes this call does not replace, + // so a declared path cannot collide with one held by another process type. + // Checking before the write keeps the all-or-nothing contract. + for _, attachment := range declared { + if err := ensureContainerPathFree(input.AppName, retained, attachment); err != nil { + return nil, err + } + } + + return append(retained, declared...), nil + }) if err != nil { return err } - retained := []*Attachment{} - for _, attachment := range existing { - if attachment.EffectiveProcessType() != processType { - retained = append(retained, attachment) - } - } - - // The declared set is already unique on container path within its own - // scope; what is left to check is the scopes this call does not replace, - // so a declared path cannot collide with one held by another process type. - // Checking before the write keeps the all-or-nothing contract. - for _, attachment := range declared { - if err := ensureContainerPathFree(input.AppName, retained, attachment); err != nil { - return err - } - } - - attachments := append(retained, declared...) - - if err := SaveAttachments(input.AppName, attachments); err != nil { - return err - } - common.LogInfo1(fmt.Sprintf("Storage mounts for process type %s replaced on %s", processType, input.AppName)) return nil } @@ -510,30 +506,27 @@ func resolveUnmountTarget(mount string, containerDir string) (unmountTarget, err // written, so an argument naming a mount the app does not have leaves the rest // of the set in place rather than removing a prefix of it. func unmountMounts(input CommandUnmountInput) error { - attachments, err := LoadAttachments(input.AppName) - if err != nil { - return err - } - - for _, mount := range input.Mounts { - target, err := resolveUnmountTarget(mount, input.ContainerDir) - if err != nil { - return err - } - - attachments, err = removeMatchingAttachments(attachments, input.AppName, target.EntryName, target.ContainerPath) - if err != nil { - // The legacy form historically said "Mount path does not - // exist." - preserve that exact wording so existing automation - // and the bats suite keep matching. - if target.Legacy && strings.Contains(err.Error(), "is not mounted") { - return errors.New("Mount path does not exist.") + return UpdateAttachments(input.AppName, func(attachments []*Attachment) ([]*Attachment, error) { + for _, mount := range input.Mounts { + target, err := resolveUnmountTarget(mount, input.ContainerDir) + if err != nil { + return nil, err } - return err - } - } - return SaveAttachments(input.AppName, attachments) + attachments, err = removeMatchingAttachments(attachments, input.AppName, target.EntryName, target.ContainerPath) + if err != nil { + // The legacy form historically said "Mount path does not + // exist." - preserve that exact wording so existing automation + // and the bats suite keep matching. + if target.Legacy && strings.Contains(err.Error(), "is not mounted") { + return nil, errors.New("Mount path does not exist.") + } + return nil, err + } + } + + return attachments, nil + }) } // unmountAll removes every attachment on an app, or every attachment for one @@ -560,19 +553,16 @@ func unmountAll(input CommandUnmountInput) error { return nil } - attachments, err := LoadAttachments(input.AppName) - if err != nil { - return err - } - - keep := []*Attachment{} - for _, attachment := range attachments { - if attachment.EffectiveProcessType() != input.ProcessType { - keep = append(keep, attachment) + err := UpdateAttachments(input.AppName, func(attachments []*Attachment) ([]*Attachment, error) { + keep := []*Attachment{} + for _, attachment := range attachments { + if attachment.EffectiveProcessType() != input.ProcessType { + keep = append(keep, attachment) + } } - } - - if err := SaveAttachments(input.AppName, keep); err != nil { + return keep, nil + }) + if err != nil { return err } diff --git a/tests/unit/buildpacks.bats b/tests/unit/buildpacks.bats index e52e700fb..732a3be27 100644 --- a/tests/unit/buildpacks.bats +++ b/tests/unit/buildpacks.bats @@ -476,6 +476,29 @@ teardown() { assert_output_contains "heroku-buildpack-python" } +@test "(buildpacks:add) concurrent calls for the same app keep every buildpack" { + run run_concurrently 20 /bin/bash -c 'dokku buildpacks:add "$1" "https://github.com/dokku/buildpack-race$2.git"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku --quiet buildpacks:list $TEST_APP | grep -cE '^ *https://github.com/dokku/buildpack-race[0-9]+\.git$'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "20" + + run run_concurrently 20 /bin/bash -c 'dokku buildpacks:remove "$1" "https://github.com/dokku/buildpack-race$2.git"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku --quiet buildpacks:list $TEST_APP | grep -cE '^ *https://github.com/dokku/buildpack-race[0-9]+\.git$'" + echo "output: $output" + echo "status: $status" + assert_output "0" +} + template_buildpacks_appjson() { local APP="$1" local APP_REPO_DIR="$2" diff --git a/tests/unit/caddy.bats b/tests/unit/caddy.bats index dd1e48658..e78a160a4 100644 --- a/tests/unit/caddy.bats +++ b/tests/unit/caddy.bats @@ -429,3 +429,26 @@ teardown() { assert_success assert_output_not_exists } + +@test "(caddy:labels:add) concurrent calls for the same app keep every label" { + run run_concurrently 20 /bin/bash -c 'dokku caddy:labels:add "$1" "caddy.race$2" value' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku caddy:labels:show $TEST_APP | grep -c 'caddy\.race[0-9]*=value'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "20" + + run run_concurrently 20 /bin/bash -c 'dokku caddy:labels:remove "$1" "caddy.race$2"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku caddy:labels:show $TEST_APP 2>/dev/null | grep -c 'caddy\.race[0-9]*=value'" + echo "output: $output" + echo "status: $status" + assert_output "0" +} diff --git a/tests/unit/checks.bats b/tests/unit/checks.bats index 365702002..f506aeb52 100644 --- a/tests/unit/checks.bats +++ b/tests/unit/checks.bats @@ -374,3 +374,26 @@ teardown() { assert_output_contains "Failure in name='port listening check': container listening on expected IPv4 interface with an unexpected port: expected=5000 actual=5001" assert_output_contains "Running healthcheck name='port listening check' attempts=3 port=5000 retries=2 timeout=5 type='listening' wait=5" } + +@test "(checks:disable) concurrent calls for the same app keep every process type" { + run run_concurrently 20 /bin/bash -c 'dokku checks:disable "$1" "worker$2"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku checks:report $TEST_APP --checks-disabled-list | tr ',' '\n' | grep -cx 'worker[0-9]*'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "20" + + run run_concurrently 20 /bin/bash -c 'dokku checks:enable "$1" "worker$2"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku checks:report $TEST_APP --checks-disabled-list | tr ',' '\n' | grep -cx 'worker[0-9]*'" + echo "output: $output" + echo "status: $status" + assert_output "0" +} diff --git a/tests/unit/config.bats b/tests/unit/config.bats index 24108fdbe..7545308b6 100644 --- a/tests/unit/config.bats +++ b/tests/unit/config.bats @@ -787,6 +787,29 @@ teardown() { assert_output_contains "Invalid key name" } +@test "(config:set) concurrent calls for the same app keep every key" { + run run_concurrently 20 /bin/bash -c 'dokku config:set --no-restart "$1" "RACE_KEY_$2=value"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku config:keys $TEST_APP | grep -cx 'RACE_KEY_[0-9]*'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "20" + + run run_concurrently 20 /bin/bash -c 'dokku config:unset --no-restart "$1" "RACE_KEY_$2"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku config:keys $TEST_APP | grep -cx 'RACE_KEY_[0-9]*'" + echo "output: $output" + echo "status: $status" + assert_output "0" +} + block_app_config_dir() { declare desc="makes the app config directory unwritable for the dokku user" diff --git a/tests/unit/docker-options-2.bats b/tests/unit/docker-options-2.bats index 620a8a105..16f494b0d 100644 --- a/tests/unit/docker-options-2.bats +++ b/tests/unit/docker-options-2.bats @@ -316,6 +316,29 @@ teardown() { assert_output_contains "running" } +@test "(docker-options:add) concurrent calls for the same app keep every option" { + run run_concurrently 20 /bin/bash -c 'dokku docker-options:add "$1" deploy "--label=race$2"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku docker-options:list $TEST_APP --phase deploy | grep -cx -- '--label=race[0-9]*'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "20" + + run run_concurrently 20 /bin/bash -c 'dokku docker-options:remove "$1" deploy "--label=race$2"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku docker-options:list $TEST_APP --phase deploy | grep -cx -- '--label=race[0-9]*'" + echo "output: $output" + echo "status: $status" + assert_output "0" +} + web_worker_callback() { local APP="$1" local APP_REPO_DIR="$2" diff --git a/tests/unit/ports.bats b/tests/unit/ports.bats index e47358063..761e65fe7 100644 --- a/tests/unit/ports.bats +++ b/tests/unit/ports.bats @@ -415,3 +415,26 @@ teardown() { assert_success assert_output "true" } + +@test "(ports:add) concurrent calls for the same app keep every port mapping" { + run run_concurrently 20 /bin/bash -c 'dokku ports:add "$1" "http:$((8000 + $2)):5000"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku --quiet ports:report $TEST_APP --ports-map | tr ' ' '\n' | grep -cx 'http:80[0-9][0-9]:5000'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "20" + + run run_concurrently 20 /bin/bash -c 'dokku ports:remove "$1" "http:$((8000 + $2)):5000"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku --quiet ports:report $TEST_APP --ports-map | tr ' ' '\n' | grep -cx 'http:80[0-9][0-9]:5000'" + echo "output: $output" + echo "status: $status" + assert_output "0" +} diff --git a/tests/unit/storage-3.bats b/tests/unit/storage-3.bats index 043f1c567..4fe6b0119 100644 --- a/tests/unit/storage-3.bats +++ b/tests/unit/storage-3.bats @@ -488,3 +488,53 @@ teardown() { run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" assert_success } + +@test "(storage:mount) concurrent calls for the same app keep every mount" { + run run_concurrently 20 /bin/bash -c 'dokku storage:create "rdmtest-race$1"' _ + echo "output: $output" + echo "status: $status" + assert_success + + run run_concurrently 20 /bin/bash -c 'dokku storage:mount "$1" "rdmtest-race$2" --container-dir "/race/$2"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "20" + + run run_concurrently 20 /bin/bash -c 'dokku storage:unmount "$1" "rdmtest-race$2"' _ "$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + echo "output: $output" + echo "status: $status" + assert_output "0" + + run run_concurrently 20 /bin/bash -c 'dokku storage:destroy "rdmtest-race$1" --force' _ + assert_success +} + +@test "(storage:labels:set) concurrent calls for the same entry keep every label" { + run /bin/bash -c "dokku storage:create rdmtest-race-label" + assert_success + + run run_concurrently 20 /bin/bash -c 'dokku storage:labels:set rdmtest-race-label "example.com/race$1" value' _ + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:info rdmtest-race-label --format json | jq -r '.labels | length'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "20" + + run /bin/bash -c "dokku storage:destroy rdmtest-race-label --force" + assert_success +} diff --git a/tests/unit/test_helper.bash b/tests/unit/test_helper.bash index d24ee09d2..44674a910 100644 --- a/tests/unit/test_helper.bash +++ b/tests/unit/test_helper.bash @@ -756,3 +756,23 @@ uninstall_k3s() { echo "status: $status" assert_success } + +run_concurrently() { + declare desc="runs a command COUNT times in parallel, appending the run number as the last argument, and fails if any run fails" + declare COUNT="$1" + shift 1 + local i pid failed=0 + local pids=() + + for i in $(seq 1 "$COUNT"); do + "$@" "$i" & + pids+=("$!") + done + + for pid in "${pids[@]}"; do + wait "$pid" || failed=$((failed + 1)) + done + + echo "failed runs: $failed" + [[ "$failed" -eq 0 ]] +}