The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/6647
This e-mail was sent by the LXC bot, direct replies will not reach the author
unless they happen to be subscribed to this list.
=== Description (from pull-request) ===
As I've been working on the new storage layer I've found myself using a revert pattern that worked well but had a fair amount of boiler plate, so this package removes some of the duplication and makes it clearer to read.
Original pattern
```go
func main() {
revertFuncs := []func{}
defer func() {
range _, f := revertFuncs {
f()
}
}
// Do step 1
revertFuncs := append(revertFuncs, func() {
//something that undoes step 1
})
// Do step 2
revertFuncs = nil
return
}
```
This also has the disadvantage that the revert steps are not run in reverse order.
This PR introduces the `revert` package which reduces boiler plate and runs revert steps in reverse order:
```go
func main() {
revert := revert.New()
defer revert.Fail()
// Do step 1
revert.Add( func() {
//something that undoes step 1
})
// Do step 2
revert.Success()
return
}
```
From 40a26daa80f7b29c266d070100320afaca27b5a0 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 19 Dec 2019 14:50:31 +0000
Subject: [PATCH 1/4] lxd/revert: Adds revert helper package for running revert
functions in reverse order
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/revert/revert.go | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
create mode 100644 lxd/revert/revert.go
diff --git a/lxd/revert/revert.go b/lxd/revert/revert.go
new file mode 100644
index 0000000000..df4a3b2578
--- /dev/null
+++ b/lxd/revert/revert.go
@@ -0,0 +1,33 @@
+package revert
+
+// Reverter is a helper type to manage revert functions.
+type Reverter struct {
+ revertFuncs []func()
+}
+
+// New returns a new Reverter.
+func New() *Reverter {
+ return &Reverter{}
+}
+
+// Add adds a revert function to the list to be run when Revert() is called.
+func (r *Reverter) Add(f func()) {
+ r.revertFuncs = append(r.revertFuncs, f)
+}
+
+// Fail runs any revert functions in the reverse order they were added.
+// Should be used with defer or when a task has encountered an error and needs
to be reverted.
+func (r *Reverter) Fail() {
+ funcCount := len(r.revertFuncs)
+ for k := range r.revertFuncs {
+ // Run the revert functions in reverse order.
+ k = funcCount - 1 - k
+ r.revertFuncs[k]()
+ }
+}
+
+// Success clears the revert functions previously added.
+// Should be called on successful completion of a task to prevent revert
functions from being run.
+func (r *Reverter) Success() {
+ r.revertFuncs = nil
+}
From 9acf534017cb167ca134ffe99abcf2a388b63df3 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 19 Dec 2019 15:05:19 +0000
Subject: [PATCH 2/4] lxd/revert/revert/test: Adds revert tests
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/revert/revert_test.go | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 lxd/revert/revert_test.go
diff --git a/lxd/revert/revert_test.go b/lxd/revert/revert_test.go
new file mode 100644
index 0000000000..aa46597536
--- /dev/null
+++ b/lxd/revert/revert_test.go
@@ -0,0 +1,31 @@
+package revert_test
+
+import (
+ "fmt"
+
+ "github.com/lxc/lxd/lxd/revert"
+)
+
+func ExampleRevertFail() {
+ revert := revert.New()
+ defer revert.Fail()
+
+ revert.Add(func() { fmt.Println("1st step") })
+ revert.Add(func() { fmt.Println("2nd step") })
+
+ return // Revert functions are run in reverse order on return.
+ // Output: 2nd step
+ // 1st step
+}
+
+func ExampleRevertSuccess() {
+ revert := revert.New()
+ defer revert.Fail()
+
+ revert.Add(func() { fmt.Println("1st step") })
+ revert.Add(func() { fmt.Println("2nd step") })
+
+ revert.Success() // Revert functions added are not run on return.
+ return
+ // Output:
+}
From 2ffc42d15112051f6f6ea272bc655700a0b68762 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 19 Dec 2019 14:59:34 +0000
Subject: [PATCH 3/4] lxd/storage/backend/lxd: Updates to use revert pkg rather
than custom revertFuncs slice
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/backend_lxd.go | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/lxd/storage/backend_lxd.go b/lxd/storage/backend_lxd.go
index 3bc2029119..1c7a0794cb 100644
--- a/lxd/storage/backend_lxd.go
+++ b/lxd/storage/backend_lxd.go
@@ -15,6 +15,7 @@ import (
"github.com/lxc/lxd/lxd/migration"
"github.com/lxc/lxd/lxd/operations"
"github.com/lxc/lxd/lxd/project"
+ "github.com/lxc/lxd/lxd/revert"
"github.com/lxc/lxd/lxd/state"
"github.com/lxc/lxd/lxd/storage/drivers"
"github.com/lxc/lxd/lxd/storage/locking"
@@ -449,12 +450,8 @@ func (b *lxdBackend) CreateInstanceFromBackup(srcBackup
backup.Info, srcData io.
// We will apply the config as part of the post hook function returned
if driver needs to.
vol := b.newVolume(drivers.VolumeTypeContainer, drivers.ContentTypeFS,
volStorageName, nil)
- revertFuncs := []func(){}
- defer func() {
- for _, revertFunc := range revertFuncs {
- revertFunc()
- }
- }()
+ revert := revert.New()
+ defer revert.Fail()
// Unpack the backup into the new storage volume(s).
volPostHook, revertHook, err := b.driver.CreateVolumeFromBackup(vol,
srcBackup.Snapshots, srcData, srcBackup.OptimizedStorage, op)
@@ -463,7 +460,7 @@ func (b *lxdBackend) CreateInstanceFromBackup(srcBackup
backup.Info, srcData io.
}
if revertHook != nil {
- revertFuncs = append(revertFuncs, revertHook)
+ revert.Add(revertHook)
}
err = b.ensureInstanceSymlink(instancetype.Container,
srcBackup.Project, srcBackup.Name, vol.MountPath())
@@ -471,7 +468,7 @@ func (b *lxdBackend) CreateInstanceFromBackup(srcBackup
backup.Info, srcData io.
return nil, nil, err
}
- revertFuncs = append(revertFuncs, func() {
+ revert.Add(func() {
b.removeInstanceSymlink(instancetype.Container,
srcBackup.Project, srcBackup.Name)
})
@@ -481,7 +478,7 @@ func (b *lxdBackend) CreateInstanceFromBackup(srcBackup
backup.Info, srcData io.
return nil, nil, err
}
- revertFuncs = append(revertFuncs, func() {
+ revert.Add(func() {
b.removeInstanceSnapshotSymlinkIfUnused(instancetype.Container,
srcBackup.Project, srcBackup.Name)
})
}
@@ -520,7 +517,7 @@ func (b *lxdBackend) CreateInstanceFromBackup(srcBackup
backup.Info, srcData io.
}
}
- revertFuncs = nil
+ revert.Success()
return postHook, revertHook, nil
}
From 5aa391deaf686f9c835d60cf954b9c56de223dcf Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 19 Dec 2019 15:00:08 +0000
Subject: [PATCH 4/4] lxd/storage/drivers/driver/dir: Updates to use revert pkg
rather than custom revertFuncs slice
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_dir_utils.go | 18 +++++++-----------
lxd/storage/drivers/driver_dir_volumes.go | 20 +++++++-------------
2 files changed, 14 insertions(+), 24 deletions(-)
diff --git a/lxd/storage/drivers/driver_dir_utils.go
b/lxd/storage/drivers/driver_dir_utils.go
index aee2a289d6..f13bb1cae7 100644
--- a/lxd/storage/drivers/driver_dir_utils.go
+++ b/lxd/storage/drivers/driver_dir_utils.go
@@ -3,6 +3,7 @@ package drivers
import (
"fmt"
+ "github.com/lxc/lxd/lxd/revert"
"github.com/lxc/lxd/lxd/storage/quota"
log "github.com/lxc/lxd/shared/log15"
"github.com/lxc/lxd/shared/units"
@@ -29,10 +30,8 @@ func (d *dir) setupInitialQuota(vol Volume) (func(), error) {
return nil, err
}
- // Define a function to revert the quota being setup.
- revertFunc := func() {
- d.deleteQuota(volPath, volID)
- }
+ revert := revert.New()
+ defer revert.Fail()
// Initialise the volume's quota using the volume ID.
err = d.initQuota(volPath, volID)
@@ -40,12 +39,9 @@ func (d *dir) setupInitialQuota(vol Volume) (func(), error) {
return nil, err
}
- revert := true
- defer func() {
- if revert {
- revertFunc()
- }
- }()
+ // Define a function to revert the quota being setup.
+ revertFunc := func() { d.deleteQuota(volPath, volID) }
+ revert.Add(revertFunc)
// Set the quota.
err = d.setQuota(volPath, volID, vol.config["size"])
@@ -53,7 +49,7 @@ func (d *dir) setupInitialQuota(vol Volume) (func(), error) {
return nil, err
}
- revert = false
+ revert.Success()
return revertFunc, nil
}
diff --git a/lxd/storage/drivers/driver_dir_volumes.go
b/lxd/storage/drivers/driver_dir_volumes.go
index bda0866ef1..53291ee4a4 100644
--- a/lxd/storage/drivers/driver_dir_volumes.go
+++ b/lxd/storage/drivers/driver_dir_volumes.go
@@ -8,6 +8,7 @@ import (
"github.com/lxc/lxd/lxd/migration"
"github.com/lxc/lxd/lxd/operations"
+ "github.com/lxc/lxd/lxd/revert"
"github.com/lxc/lxd/lxd/rsync"
"github.com/lxc/lxd/lxd/storage/quota"
"github.com/lxc/lxd/shared"
@@ -18,18 +19,15 @@ import (
func (d *dir) CreateVolume(vol Volume, filler *VolumeFiller, op
*operations.Operation) error {
volPath := vol.MountPath()
+ revert := revert.New()
+ defer revert.Fail()
+
// Create the volume itself.
err := vol.EnsureMountPath()
if err != nil {
return err
}
-
- revertPath := true
- defer func() {
- if revertPath {
- os.RemoveAll(volPath)
- }
- }()
+ revert.Add(func() { os.RemoveAll(volPath) })
// Create sparse loopback file if volume is block.
rootBlockPath := ""
@@ -46,11 +44,7 @@ func (d *dir) CreateVolume(vol Volume, filler *VolumeFiller,
op *operations.Oper
}
if revertFunc != nil {
- defer func() {
- if revertPath {
- revertFunc()
- }
- }()
+ revert.Add(revertFunc)
}
}
@@ -72,7 +66,7 @@ func (d *dir) CreateVolume(vol Volume, filler *VolumeFiller,
op *operations.Oper
}
}
- revertPath = false
+ revert.Success()
return nil
}
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
