daemon/graphdriver/zfs: ignore non-existent dataset on removal by arcenik · Pull Request #48520 · moby/moby

arcenik · 2024-09-17T18:50:49Z

- What I did

I updated the Remove function to ignore the "dataset does not exist" error

- How I did it

Doing a strings.Contains of "dataset does not exist" in err

- How to verify it

- Description for the changelog

Ignores "dataset does not exist" error when removing dataset on ZFS (#43080)

greut · 2024-09-18T09:58:07Z

daemon/graphdriver/zfs/zfs.go

 	name := d.zfsPath(id)
 	dataset := zfs.Dataset{Name: name}
 	err := dataset.Destroy(zfs.DestroyRecursive)
+	if err != nil && strings.Contains(fmt.Sprint(err), "dataset does not exist") {


you may probably do a stricter check.

errZfs, ok := err.(zfs.Error) if ok && errZfs.Stderr == "dataset does not exist" { ... }

or go with err.Error() instead of fmt.Sprint(err), see https://pkg.go.dev/[email protected]#error

Happy Go hacking mate!

Done. Unfortunately, the stderr field contains the whole output of the zfs command

Stderr: "cannot open 'rpool/zzztest/doesnotexist': dataset does not exist\n",

AkihiroSuda · 2024-09-18T23:43:31Z

daemon/graphdriver/zfs/zfs.go

+		errZfs, isZfsError := err.(*zfs.Error)
+		if isZfsError && strings.HasSuffix(errZfs.Stderr, "dataset does not exist\n") {
+			logger := log.G(context.TODO()).WithField("storage-driver", "zfs")
+			logger.Warnf("Tried to destroy inexistant dataset %s", name)


Suggested change

logger.Warnf("Tried to destroy inexistant dataset %s", name)

logger.WithError(err).Warnf("Tried to destroy inexistant dataset %q", name)

Also please squash commits

thaJeztah · 2024-11-27T13:28:33Z

daemon/graphdriver/zfs/zfs.go

+		var errZfs *zfs.Error
+		isZfsError := errors.As(err, &errZfs)


Did a quick rebase, and changed this to use errors.As;

diff --git a/daemon/graphdriver/zfs/zfs.go b/daemon/graphdriver/zfs/zfs.go index bd96463d60..63301629af 100644 --- a/daemon/graphdriver/zfs/zfs.go +++ b/daemon/graphdriver/zfs/zfs.go @@ -4,6 +4,7 @@ package zfs // import "github.com/docker/docker/daemon/graphdriver/zfs" import ( "context" + "errors" "fmt" "os" "os/exec" @@ -362,7 +363,8 @@ func (d *Driver) Remove(id string) error { dataset := zfs.Dataset{Name: name} err := dataset.Destroy(zfs.DestroyRecursive) if err != nil { - errZfs, isZfsError := err.(*zfs.Error) + var errZfs *zfs.Error + isZfsError := errors.As(err, &errZfs) if isZfsError && strings.HasSuffix(errZfs.Stderr, "dataset does not exist\n") { logger := log.G(context.TODO()).WithField("storage-driver", "zfs") logger.WithError(err).Warnf("Tried to destroy inexistant dataset %q", name)

thaJeztah · 2024-11-27T13:29:32Z

daemon/graphdriver/zfs/zfs.go

+			logger := log.G(context.TODO()).WithField("storage-driver", "zfs")
+			logger.WithError(err).Warnf("Tried to destroy inexistant dataset %q", name)


Actually; let me change this to WithFields instead of chaining WithField and WithError

thaJeztah

LGTM

I'll ask for a second review as I made the last changes now 🙈 😂

thaJeztah · 2024-11-27T13:36:11Z

oh! I messed up an import, probably my IDE tried to be helpful;

#23 49.72 daemon/graphdriver/zfs/zfs.go:27:2: errors redeclared in this block
#23 49.72 	daemon/graphdriver/zfs/zfs.go:7:2: other declaration of errors
#23 49.72 daemon/graphdriver/zfs/zfs.go:27:2: "github.com/pkg/errors" imported and not used
#23 49.72 daemon/graphdriver/zfs/zfs.go:416:21: undefined: errors.Wrap

thaJeztah · 2024-11-27T13:37:25Z

Should be good now 😅 🤞

thaJeztah · 2024-11-27T13:43:26Z

daemon/graphdriver/zfs/zfs.go

+	if err != nil {
+		var errZfs *zfs.Error
+		isZfsError := errors.As(err, &errZfs)
+		if isZfsError && strings.HasSuffix(errZfs.Stderr, "dataset does not exist\n") {


I'm slightly on the fence if we should consider stripping the newline from Stderr. Perhaps we should; I just had a quick peek at the zfs code and it already does some manipulation of the output it got, so it may be a bit brittle if we depend on the newline to be present.

Let me add a quick strings.TrimSpace here to prevent formatting changes from breaking this.

Ignore "dataset does not exist" error in Remove function Signed-off-by: François Scala <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

robmry

LGTM

arcenik force-pushed the 43080-zfs-destroy-missing-volume-fails branch from ffaaae8 to 2189e47 Compare September 17, 2024 18:53

greut reviewed Sep 18, 2024

View reviewed changes

AkihiroSuda reviewed Sep 18, 2024

View reviewed changes

arcenik force-pushed the 43080-zfs-destroy-missing-volume-fails branch from b402bed to 12431a9 Compare September 19, 2024 08:10

thaJeztah added status/2-code-review area/storage/zfs labels Sep 23, 2024

thaJeztah force-pushed the 43080-zfs-destroy-missing-volume-fails branch from 12431a9 to 446d648 Compare November 27, 2024 13:25

thaJeztah changed the title ~~(daemon/graphdriver/zfs) Fixes #43080~~ daemon/graphdriver/zfs: ignore non-existent dataset on removal Nov 27, 2024

thaJeztah force-pushed the 43080-zfs-destroy-missing-volume-fails branch from 446d648 to 543aa2e Compare November 27, 2024 13:27

thaJeztah reviewed Nov 27, 2024

View reviewed changes

thaJeztah force-pushed the 43080-zfs-destroy-missing-volume-fails branch from 543aa2e to e3fae0c Compare November 27, 2024 13:31

thaJeztah added this to the 28.0.0 milestone Nov 27, 2024

thaJeztah mentioned this pull request Nov 27, 2024

Driver "zfs" failed to remove root filesystem: "cannot open '<dataset-path>': dataset does not exist" #43080

Closed

thaJeztah approved these changes Nov 27, 2024

View reviewed changes

thaJeztah force-pushed the 43080-zfs-destroy-missing-volume-fails branch from e3fae0c to 2a9dc0d Compare November 27, 2024 13:37

thaJeztah added the process/cherry-pick/27.x label Nov 27, 2024

thaJeztah reviewed Nov 27, 2024

View reviewed changes

daemon/graphdriver/zfs: ignore non-existent dataset on removal

e7d15d4

Ignore "dataset does not exist" error in Remove function Signed-off-by: François Scala <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the 43080-zfs-destroy-missing-volume-fails branch from 2a9dc0d to e7d15d4 Compare November 27, 2024 13:44

robmry approved these changes Nov 27, 2024

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Nov 27, 2024

thaJeztah mentioned this pull request Nov 27, 2024

[27.x backport] daemon/graphdriver/zfs: ignore non-existent dataset on removal #48968

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick/27.x labels Nov 27, 2024

thaJeztah merged commit 7b8b63e into moby:master Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon/graphdriver/zfs: ignore non-existent dataset on removal#48520

daemon/graphdriver/zfs: ignore non-existent dataset on removal#48520
thaJeztah merged 1 commit intomoby:masterfrom
arcenik:43080-zfs-destroy-missing-volume-fails

arcenik commented Sep 17, 2024

Uh oh!

greut Sep 18, 2024

Uh oh!

arcenik Sep 18, 2024 •

edited

Loading

Uh oh!

AkihiroSuda Sep 18, 2024 •

edited

Loading

Uh oh!

AkihiroSuda Sep 18, 2024

Uh oh!

thaJeztah Nov 27, 2024

Uh oh!

thaJeztah Nov 27, 2024

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah commented Nov 27, 2024

Uh oh!

thaJeztah commented Nov 27, 2024

Uh oh!

thaJeztah Nov 27, 2024

Uh oh!

robmry left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	logger.Warnf("Tried to destroy inexistant dataset %s", name)
	logger.WithError(err).Warnf("Tried to destroy inexistant dataset %q", name)

		logger := log.G(context.TODO()).WithField("storage-driver", "zfs")
		logger.WithError(err).Warnf("Tried to destroy inexistant dataset %q", name)

Conversation

arcenik commented Sep 17, 2024

Uh oh!

greut Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

arcenik Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 27, 2024

Uh oh!

thaJeztah commented Nov 27, 2024

Uh oh!

thaJeztah Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

robmry left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arcenik Sep 18, 2024 •

edited

Loading

AkihiroSuda Sep 18, 2024 •

edited

Loading