From bb0d01096262eea9b5f04e53851963f7f45fe453 Mon Sep 17 00:00:00 2001 From: AnderG Date: Fri, 19 Jul 2024 21:40:32 +0200 Subject: [PATCH 01/19] Fix docker cp --- activemount.go | 110 +++++++++++++++++++++++++++++++++++++++++++++++++ driver.go | 84 ++++++------------------------------- 2 files changed, 122 insertions(+), 72 deletions(-) create mode 100644 activemount.go diff --git a/activemount.go b/activemount.go new file mode 100644 index 0000000..b59e076 --- /dev/null +++ b/activemount.go @@ -0,0 +1,110 @@ +package main + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "os" + + "github.com/docker/go-plugins-helpers/volume" +) + +type ActiveMount struct { + UsageCount int +} + +func (d *DockerOnTop) Activate(request *volume.MountRequest, activemountsdir lockedFile) (bool, error) { + var activeMount ActiveMount + var result bool + + _, readDirErr := activemountsdir.ReadDir(1) // Check if there are any files inside activemounts dir + if readDirErr == nil { + // There is something no need to mount the filesystem again + result = false + } else if errors.Is(readDirErr, io.EOF) { + // The directory is empty, mount the filesystem + result = true + } else { + log.Errorf("Failed to list the activemounts directory: %v", readDirErr) + return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) + } + + activemountFilePath := d.activemountsdir(request.Name) + request.ID + file, err := os.Open(activemountFilePath) + + if err == nil { + // The file can exist from a previous mount when doing a docker cp on an already mounted container, no need to mount the filesystem again + payload, _ := io.ReadAll(file) + json.Unmarshal(payload, &activeMount) + file.Close() + } else if os.IsNotExist(err) { + // Default case, we need to create a new active mount, the filesystem needs to be mounted + activeMount = ActiveMount{UsageCount: 0} + } else { + log.Errorf("Active mount file %s exists but cannot be read.", activemountFilePath) + return false, fmt.Errorf("active mount file %s exists but cannot be read", activemountFilePath) + } + + activeMount.UsageCount++ + + payload, _ := json.Marshal(activeMount) + err = os.WriteFile(activemountFilePath, payload, 0o666) + if err != nil { + log.Errorf("Active mount file %s cannot be written.", activemountFilePath) + return false, fmt.Errorf("active mount file %s cannot be written", activemountFilePath) + } + + return result, nil +} + +func (d *DockerOnTop) Deactivate(request *volume.UnmountRequest, activemountsdir lockedFile) (bool, error) { + + dirEntries, readDirErr := activemountsdir.ReadDir(2) // Check if there is any _other_ container using the volume + if errors.Is(readDirErr, io.EOF) { + // If directory is empty, unmount overlay and clean up + log.Errorf("There are no active mount files and one was expected. Unmounting.") + return true, fmt.Errorf("there are no active mount files and one was expected. Unmounting") + } else if readDirErr != nil { + log.Errorf("Failed to list the activemounts directory: %v", readDirErr) + return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) + } + + var activeMount ActiveMount + activemountFilePath := d.activemountsdir(request.Name) + request.ID + + otherVolumesPresent := len(dirEntries) > 1 || dirEntries[0].Name() != request.ID + + file, err := os.Open(activemountFilePath) + + if err == nil { + payload, _ := io.ReadAll(file) + json.Unmarshal(payload, &activeMount) + file.Close() + } else if os.IsNotExist(err) { + log.Errorf("The active mount file %s was expected but is not there", activemountFilePath) + return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there", activemountFilePath) + } else { + log.Errorf("The active mount file %s could not be opened", activemountFilePath) + return false, fmt.Errorf("the active mount file %s could not be opened", activemountFilePath) + } + + activeMount.UsageCount-- + + if activeMount.UsageCount == 0 { + err := os.Remove(activemountFilePath) + if err != nil { + log.Errorf("The active mount file %s could not be deleted", activemountFilePath) + return false, fmt.Errorf("the active mount file %s could not be deleted", activemountFilePath) + } + return !otherVolumesPresent, nil + } else { + payload, _ := json.Marshal(activeMount) + err = os.WriteFile(activemountFilePath, payload, 0o666) + if err != nil { + log.Errorf("The active mount file %s could not be updated", activemountFilePath) + return false, fmt.Errorf("the active mount file %s could not be updated", activemountFilePath) + } + return false, nil + } +} diff --git a/driver.go b/driver.go index a12abda..fd9dfc8 100644 --- a/driver.go +++ b/driver.go @@ -3,7 +3,6 @@ package main import ( "errors" "fmt" - "io" "os" "regexp" "strings" @@ -184,10 +183,10 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse } defer activemountsdir.Close() // There is nothing I could do about the error (logging is performed inside `Close()` anyway) - _, readDirErr := activemountsdir.ReadDir(1) // Check if there are any files inside activemounts dir - if errors.Is(readDirErr, io.EOF) { - // No files => no other containers are using the volume. Need to mount the overlay - + mount, err := d.Activate(request, activemountsdir) + if err != nil { + return nil, internalError("failed to activate the active mount:", err) + } else if mount { lowerdir := thisVol.BaseDirPath upperdir := d.upperdir(request.Name) workdir := d.workdir(request.Name) @@ -210,48 +209,9 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse log.Errorf("Failed to mount overlay for volume %s: %v", request.Name, err) return nil, internalError("failed to mount overlay", err) } - log.Debugf("Mounted volume %s at %s", request.Name, mountpoint) - } else if err == nil { - log.Debugf("Volume %s is already mounted for some other container. Indicating success without remounting", - request.Name) - } else { - log.Errorf("Failed to list the activemounts directory: %v", err) - return nil, internalError("failed to list activemounts/", err) - } - - activemountFilePath := d.activemountsdir(request.Name) + request.ID - f, err := os.Create(activemountFilePath) - if err == nil { - // We don't care about the file's contents - _ = f.Close() } else { - if os.IsExist(err) { - // Super weird. I can't imagine why this would happen. - log.Warningf("Active mount %s already exists (but it shouldn't...)", activemountFilePath) - } else { - // A really bad situation! - // We successfully mounted (`syscall.Mount`) the volume but failed to put information about the container - // using the volume. In the worst case (if we just created the volume) the following happens: - // Using the plugin, it is now impossible to unmount the volume (this container is not created, so there's - // no one to trigger `.Unmount()`) and impossible to remove (the directory mountpoint/ is a mountpoint, so - // attempting to remove it will fail with `syscall.EBUSY`). - // It is possible to mount the volume again: a new overlay will be mounted, shadowing the previous one. - // The new overlay will be possible to unmount but, as the old overlay remains, the Unmount method won't - // succeed because the attempt to remove mountpoint/ will result in `syscall.EBUSY`. - // - // Thus, a human interaction is required. - // - // (if it's not us who actually mounted the overlay, then the situation isn't too bad: no new container is - // started, the error is reported to the end user). - log.Criticalf("Failed to create active mount file: %v. If no other container was currently "+ - "using the volume, this volume's state is now invalid. A human interaction or a reboot is required", - err) - return nil, fmt.Errorf("docker-on-top internal error: failed to create an active mount file: %w. "+ - "The volume is now locked. Make sure that no other container is using the volume, then run "+ - "`unmount %s` to unlock it. Human interaction is required. Please, report this bug", - err, mountpoint) - } + log.Debugf("Volume %s already mounted at %s", request.Name, mountpoint) } return &response, nil @@ -273,40 +233,20 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { } defer activemountsdir.Close() // There's nothing I could do about the error if it occurs - dirEntries, readDirErr := activemountsdir.ReadDir(2) // Check if there is any _other_ container using the volume - if len(dirEntries) == 1 || errors.Is(readDirErr, io.EOF) { - // If just one entry or directory is empty, unmount overlay and clean up - + unmount, err := d.Deactivate(request, activemountsdir) + if err != nil { + return internalError("failed to deactivate the active mount:", err) + } else if unmount { err = syscall.Unmount(d.mountpointdir(request.Name), 0) if err != nil { log.Errorf("Failed to unmount %s: %v", d.mountpointdir(request.Name), err) return err } - err = d.volumeTreePostUnmount(request.Name) - // Don't return yet. The above error will be returned later - } else if readDirErr == nil { - log.Debugf("Volume %s is still mounted in some other container. Indicating success without unmounting", - request.Name) - } else { - log.Errorf("Failed to list the activemounts directory: %v", err) - return internalError("failed to list activemounts/", err) - } - activemountFilePath := d.activemountsdir(request.Name) + request.ID - err2 := os.Remove(activemountFilePath) - if os.IsNotExist(err2) { - log.Warningf("Failed to remove %s because it does not exist (but it should...)", activemountFilePath) - } else if err2 != nil { - // Another pretty bad situation. Even though we are no longer using the volume, it is seemingly in use by us - // because we failed to remove the file corresponding to this container. - log.Criticalf("Failed to remove the active mount file: %v. The volume is now considered used by a container "+ - "that no longer exists", err) - // The user most likely won't see this error message due to daemon not showing unmount errors to the - // `docker run` clients :(( - return fmt.Errorf("docker-on-top internal error: failed to remove the active mount file: %w. The volume is "+ - "now considered used by a container that no longer exists. Human interaction is required: remove the file "+ - "manually to fix the problem", err) + log.Debugf("Unmounted volume %s", request.Name) + } else { + log.Debugf("Volume %s is still mounted. Indicating success without unmounting", request.Name) } // Report an error during cleanup, if any From e55a292d8bb8cd724d94b99844426146c85cfe48 Mon Sep 17 00:00:00 2001 From: AnderG Date: Sun, 28 Jul 2024 22:09:14 +0200 Subject: [PATCH 02/19] Rename ActiveMount to activeMount as it is not intended to be exported. Change the variable names already using the activeMount name --- activemount.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/activemount.go b/activemount.go index b59e076..2d8a943 100644 --- a/activemount.go +++ b/activemount.go @@ -10,12 +10,12 @@ import ( "github.com/docker/go-plugins-helpers/volume" ) -type ActiveMount struct { +type activeMount struct { UsageCount int } func (d *DockerOnTop) Activate(request *volume.MountRequest, activemountsdir lockedFile) (bool, error) { - var activeMount ActiveMount + var activeMountInfo activeMount var result bool _, readDirErr := activemountsdir.ReadDir(1) // Check if there are any files inside activemounts dir @@ -36,19 +36,19 @@ func (d *DockerOnTop) Activate(request *volume.MountRequest, activemountsdir loc if err == nil { // The file can exist from a previous mount when doing a docker cp on an already mounted container, no need to mount the filesystem again payload, _ := io.ReadAll(file) - json.Unmarshal(payload, &activeMount) + json.Unmarshal(payload, &activeMountInfo) file.Close() } else if os.IsNotExist(err) { // Default case, we need to create a new active mount, the filesystem needs to be mounted - activeMount = ActiveMount{UsageCount: 0} + activeMountInfo = activeMount{UsageCount: 0} } else { log.Errorf("Active mount file %s exists but cannot be read.", activemountFilePath) return false, fmt.Errorf("active mount file %s exists but cannot be read", activemountFilePath) } - activeMount.UsageCount++ + activeMountInfo.UsageCount++ - payload, _ := json.Marshal(activeMount) + payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o666) if err != nil { log.Errorf("Active mount file %s cannot be written.", activemountFilePath) @@ -70,7 +70,7 @@ func (d *DockerOnTop) Deactivate(request *volume.UnmountRequest, activemountsdir return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) } - var activeMount ActiveMount + var activeMountInfo activeMount activemountFilePath := d.activemountsdir(request.Name) + request.ID otherVolumesPresent := len(dirEntries) > 1 || dirEntries[0].Name() != request.ID @@ -79,7 +79,7 @@ func (d *DockerOnTop) Deactivate(request *volume.UnmountRequest, activemountsdir if err == nil { payload, _ := io.ReadAll(file) - json.Unmarshal(payload, &activeMount) + json.Unmarshal(payload, &activeMountInfo) file.Close() } else if os.IsNotExist(err) { log.Errorf("The active mount file %s was expected but is not there", activemountFilePath) @@ -89,9 +89,9 @@ func (d *DockerOnTop) Deactivate(request *volume.UnmountRequest, activemountsdir return false, fmt.Errorf("the active mount file %s could not be opened", activemountFilePath) } - activeMount.UsageCount-- + activeMountInfo.UsageCount-- - if activeMount.UsageCount == 0 { + if activeMountInfo.UsageCount == 0 { err := os.Remove(activemountFilePath) if err != nil { log.Errorf("The active mount file %s could not be deleted", activemountFilePath) @@ -99,7 +99,7 @@ func (d *DockerOnTop) Deactivate(request *volume.UnmountRequest, activemountsdir } return !otherVolumesPresent, nil } else { - payload, _ := json.Marshal(activeMount) + payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o666) if err != nil { log.Errorf("The active mount file %s could not be updated", activemountFilePath) From b18043e55f229afdeb0243239d7f25d55d012173 Mon Sep 17 00:00:00 2001 From: AnderG Date: Sun, 28 Jul 2024 22:36:02 +0200 Subject: [PATCH 03/19] Rename Activate and Deactivate to activateVolume and deactivateVolume respectively. Also add a docstring for them. --- activemount.go | 34 ++++++++++++++++++++++++++++++++-- driver.go | 4 ++-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/activemount.go b/activemount.go index 2d8a943..617c260 100644 --- a/activemount.go +++ b/activemount.go @@ -14,7 +14,23 @@ type activeMount struct { UsageCount int } -func (d *DockerOnTop) Activate(request *volume.MountRequest, activemountsdir lockedFile) (bool, error) { +// activateVolume checks if the volume that has been requested to be mounted (as in docker volume mounting) +// actually requires to be mounted (as an overlay fs mount). For that purpose check if other containers +// have already mounted the volume (by reading in `activemountsdir`). It is also possible that the volume +// has already been been mounted by the same container (when doing a `docker cp` while the container is running), +// in that case the file named with the `request.ID` will contain the number of times the container has +// been requested the volume to be mounted. That number will be increased each time `activateVolume` is +// called and decreased on `deactivateVolume`. +// Parameters: +// +// request: Incoming Docker mount request. +// activemountsdir: Folder where Docker-On-Top mounts are tracked. +// +// Return: +// +// mount: Returns true if the request requires the filesystem to be mounted, false if not. +// err: If the function encountered an error, the error itself, nil if everything went right. +func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsdir lockedFile) (bool, error) { var activeMountInfo activeMount var result bool @@ -58,7 +74,21 @@ func (d *DockerOnTop) Activate(request *volume.MountRequest, activemountsdir loc return result, nil } -func (d *DockerOnTop) Deactivate(request *volume.UnmountRequest, activemountsdir lockedFile) (bool, error) { +// deactivateVolume checks if the volume that has been requested to be unmounted (as in docker volume unmounting) +// actually requires to be unmounted (as an overlay fs unmount). It will check the number of times the container +// has been requested to mount the volume in the file named `request.ID` and decrease the number, when the number +// reaches zero it will delete the `request.ID` file since this container no longer mounts the volume. It will +// also check if other containers are mounting this volume by checking for other files in the active mounts folder. +// Parameters: +// +// request: Incoming Docker unmount request. +// activemountsdir: Folder where Docker-On-Top mounts are tracked. +// +// Return: +// +// unmount: Returns true if there are not other usages of this volume and the filesystem can be unmounted. +// err: If the function encountered an error, the error itself, nil if everything went right. +func (d *DockerOnTop) DeactivateVolume(request *volume.UnmountRequest, activemountsdir lockedFile) (bool, error) { dirEntries, readDirErr := activemountsdir.ReadDir(2) // Check if there is any _other_ container using the volume if errors.Is(readDirErr, io.EOF) { diff --git a/driver.go b/driver.go index fd9dfc8..83ceefd 100644 --- a/driver.go +++ b/driver.go @@ -183,7 +183,7 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse } defer activemountsdir.Close() // There is nothing I could do about the error (logging is performed inside `Close()` anyway) - mount, err := d.Activate(request, activemountsdir) + mount, err := d.activateVolume(request, activemountsdir) if err != nil { return nil, internalError("failed to activate the active mount:", err) } else if mount { @@ -233,7 +233,7 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { } defer activemountsdir.Close() // There's nothing I could do about the error if it occurs - unmount, err := d.Deactivate(request, activemountsdir) + unmount, err := d.DeactivateVolume(request, activemountsdir) if err != nil { return internalError("failed to deactivate the active mount:", err) } else if unmount { From 7043a09038fa4b8000db47c9ab3f1d4d99ea832d Mon Sep 17 00:00:00 2001 From: AnderG Date: Sun, 28 Jul 2024 22:48:37 +0200 Subject: [PATCH 04/19] Declare activeMountInfo closer to it's usage --- activemount.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activemount.go b/activemount.go index 617c260..a2abdfc 100644 --- a/activemount.go +++ b/activemount.go @@ -31,7 +31,6 @@ type activeMount struct { // mount: Returns true if the request requires the filesystem to be mounted, false if not. // err: If the function encountered an error, the error itself, nil if everything went right. func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsdir lockedFile) (bool, error) { - var activeMountInfo activeMount var result bool _, readDirErr := activemountsdir.ReadDir(1) // Check if there are any files inside activemounts dir @@ -46,6 +45,7 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) } + var activeMountInfo activeMount activemountFilePath := d.activemountsdir(request.Name) + request.ID file, err := os.Open(activemountFilePath) From c0d28c03e0834a7b8b2fda8039376bbf41fe25ea Mon Sep 17 00:00:00 2001 From: AnderG Date: Sun, 28 Jul 2024 22:51:45 +0200 Subject: [PATCH 05/19] Refactor activemountFilePath definition and surrounding lines --- activemount.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/activemount.go b/activemount.go index a2abdfc..ec88885 100644 --- a/activemount.go +++ b/activemount.go @@ -100,11 +100,10 @@ func (d *DockerOnTop) DeactivateVolume(request *volume.UnmountRequest, activemou return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) } - var activeMountInfo activeMount - activemountFilePath := d.activemountsdir(request.Name) + request.ID - otherVolumesPresent := len(dirEntries) > 1 || dirEntries[0].Name() != request.ID + var activeMountInfo activeMount + activemountFilePath := d.activemountsdir(request.Name) + request.ID file, err := os.Open(activemountFilePath) if err == nil { From 03f2054204f32b32c670c3b866301210b7518652 Mon Sep 17 00:00:00 2001 From: AnderG Date: Sun, 28 Jul 2024 22:59:24 +0200 Subject: [PATCH 06/19] Safely ignore json.Marshal errors --- activemount.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/activemount.go b/activemount.go index ec88885..074f8c7 100644 --- a/activemount.go +++ b/activemount.go @@ -64,6 +64,9 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd activeMountInfo.UsageCount++ + // Convert activeMountInfo to JSON to store it in a file. We can safely ignore Marshal errors, since the + // activeMount structure is simple enought not to contain "strage" floats, unsupported datatypes or cycles + // which are the error causes for json.Marshal payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o666) if err != nil { @@ -128,6 +131,9 @@ func (d *DockerOnTop) DeactivateVolume(request *volume.UnmountRequest, activemou } return !otherVolumesPresent, nil } else { + // Convert activeMountInfo to JSON to store it in a file. We can safely ignore Marshal errors, since the + // activeMount structure is simple enought not to contain "strage" floats, unsupported datatypes or cycles + // which are the error causes for json.Marshal payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o666) if err != nil { From 0764601aad29eeb7de353eb28e26a5f3be7aad89 Mon Sep 17 00:00:00 2001 From: AnderG Date: Sun, 28 Jul 2024 23:05:49 +0200 Subject: [PATCH 07/19] Reduce activeMount file permissions --- activemount.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/activemount.go b/activemount.go index 074f8c7..ba4eb87 100644 --- a/activemount.go +++ b/activemount.go @@ -68,7 +68,7 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd // activeMount structure is simple enought not to contain "strage" floats, unsupported datatypes or cycles // which are the error causes for json.Marshal payload, _ := json.Marshal(activeMountInfo) - err = os.WriteFile(activemountFilePath, payload, 0o666) + err = os.WriteFile(activemountFilePath, payload, 0o644) if err != nil { log.Errorf("Active mount file %s cannot be written.", activemountFilePath) return false, fmt.Errorf("active mount file %s cannot be written", activemountFilePath) @@ -135,7 +135,7 @@ func (d *DockerOnTop) DeactivateVolume(request *volume.UnmountRequest, activemou // activeMount structure is simple enought not to contain "strage" floats, unsupported datatypes or cycles // which are the error causes for json.Marshal payload, _ := json.Marshal(activeMountInfo) - err = os.WriteFile(activemountFilePath, payload, 0o666) + err = os.WriteFile(activemountFilePath, payload, 0o644) if err != nil { log.Errorf("The active mount file %s could not be updated", activemountFilePath) return false, fmt.Errorf("the active mount file %s could not be updated", activemountFilePath) From 1985b5201fee166a4fba0d11e1a8c1026c247f02 Mon Sep 17 00:00:00 2001 From: AnderG Date: Sun, 28 Jul 2024 23:14:58 +0200 Subject: [PATCH 08/19] Rename returning variables for activateVolume and deactivateVolume --- activemount.go | 12 ++++++------ driver.go | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/activemount.go b/activemount.go index ba4eb87..097761e 100644 --- a/activemount.go +++ b/activemount.go @@ -28,18 +28,18 @@ type activeMount struct { // // Return: // -// mount: Returns true if the request requires the filesystem to be mounted, false if not. +// doMountFs: Returns true if the request requires the filesystem to be mounted, false if not. // err: If the function encountered an error, the error itself, nil if everything went right. func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsdir lockedFile) (bool, error) { - var result bool + var doMountFs bool _, readDirErr := activemountsdir.ReadDir(1) // Check if there are any files inside activemounts dir if readDirErr == nil { // There is something no need to mount the filesystem again - result = false + doMountFs = false } else if errors.Is(readDirErr, io.EOF) { // The directory is empty, mount the filesystem - result = true + doMountFs = true } else { log.Errorf("Failed to list the activemounts directory: %v", readDirErr) return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) @@ -74,7 +74,7 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd return false, fmt.Errorf("active mount file %s cannot be written", activemountFilePath) } - return result, nil + return doMountFs, nil } // deactivateVolume checks if the volume that has been requested to be unmounted (as in docker volume unmounting) @@ -89,7 +89,7 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd // // Return: // -// unmount: Returns true if there are not other usages of this volume and the filesystem can be unmounted. +// doUnmountFs: Returns true if there are not other usages of this volume and the filesystem can be unmounted. // err: If the function encountered an error, the error itself, nil if everything went right. func (d *DockerOnTop) DeactivateVolume(request *volume.UnmountRequest, activemountsdir lockedFile) (bool, error) { diff --git a/driver.go b/driver.go index 83ceefd..b7762f0 100644 --- a/driver.go +++ b/driver.go @@ -183,10 +183,10 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse } defer activemountsdir.Close() // There is nothing I could do about the error (logging is performed inside `Close()` anyway) - mount, err := d.activateVolume(request, activemountsdir) + doMountFs, err := d.activateVolume(request, activemountsdir) if err != nil { return nil, internalError("failed to activate the active mount:", err) - } else if mount { + } else if doMountFs { lowerdir := thisVol.BaseDirPath upperdir := d.upperdir(request.Name) workdir := d.workdir(request.Name) @@ -233,10 +233,10 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { } defer activemountsdir.Close() // There's nothing I could do about the error if it occurs - unmount, err := d.DeactivateVolume(request, activemountsdir) + doUnmountFs, err := d.DeactivateVolume(request, activemountsdir) if err != nil { return internalError("failed to deactivate the active mount:", err) - } else if unmount { + } else if doUnmountFs { err = syscall.Unmount(d.mountpointdir(request.Name), 0) if err != nil { log.Errorf("Failed to unmount %s: %v", d.mountpointdir(request.Name), err) From 3a67652e35100e08b72481ca830ffc1c62d54e19 Mon Sep 17 00:00:00 2001 From: AnderG Date: Sun, 28 Jul 2024 23:16:09 +0200 Subject: [PATCH 09/19] Rename DeactivateVolume to deactivateVolume as it should have been in a previous commit --- activemount.go | 2 +- driver.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/activemount.go b/activemount.go index 097761e..2b8a92a 100644 --- a/activemount.go +++ b/activemount.go @@ -91,7 +91,7 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd // // doUnmountFs: Returns true if there are not other usages of this volume and the filesystem can be unmounted. // err: If the function encountered an error, the error itself, nil if everything went right. -func (d *DockerOnTop) DeactivateVolume(request *volume.UnmountRequest, activemountsdir lockedFile) (bool, error) { +func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemountsdir lockedFile) (bool, error) { dirEntries, readDirErr := activemountsdir.ReadDir(2) // Check if there is any _other_ container using the volume if errors.Is(readDirErr, io.EOF) { diff --git a/driver.go b/driver.go index b7762f0..09c8471 100644 --- a/driver.go +++ b/driver.go @@ -233,7 +233,7 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { } defer activemountsdir.Close() // There's nothing I could do about the error if it occurs - doUnmountFs, err := d.DeactivateVolume(request, activemountsdir) + doUnmountFs, err := d.deactivateVolume(request, activemountsdir) if err != nil { return internalError("failed to deactivate the active mount:", err) } else if doUnmountFs { From c57da7724d6098d4bcb9ec34cb4a554b370576a6 Mon Sep 17 00:00:00 2001 From: AnderG Date: Mon, 29 Jul 2024 22:10:00 +0200 Subject: [PATCH 10/19] Remove log messages from activemount and place them in the calls to the activateVolume and deactivateVolume functions --- activemount.go | 9 --------- driver.go | 2 ++ 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/activemount.go b/activemount.go index 2b8a92a..48fb830 100644 --- a/activemount.go +++ b/activemount.go @@ -41,7 +41,6 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd // The directory is empty, mount the filesystem doMountFs = true } else { - log.Errorf("Failed to list the activemounts directory: %v", readDirErr) return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) } @@ -58,7 +57,6 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd // Default case, we need to create a new active mount, the filesystem needs to be mounted activeMountInfo = activeMount{UsageCount: 0} } else { - log.Errorf("Active mount file %s exists but cannot be read.", activemountFilePath) return false, fmt.Errorf("active mount file %s exists but cannot be read", activemountFilePath) } @@ -70,7 +68,6 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o644) if err != nil { - log.Errorf("Active mount file %s cannot be written.", activemountFilePath) return false, fmt.Errorf("active mount file %s cannot be written", activemountFilePath) } @@ -96,10 +93,8 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou dirEntries, readDirErr := activemountsdir.ReadDir(2) // Check if there is any _other_ container using the volume if errors.Is(readDirErr, io.EOF) { // If directory is empty, unmount overlay and clean up - log.Errorf("There are no active mount files and one was expected. Unmounting.") return true, fmt.Errorf("there are no active mount files and one was expected. Unmounting") } else if readDirErr != nil { - log.Errorf("Failed to list the activemounts directory: %v", readDirErr) return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) } @@ -114,10 +109,8 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou json.Unmarshal(payload, &activeMountInfo) file.Close() } else if os.IsNotExist(err) { - log.Errorf("The active mount file %s was expected but is not there", activemountFilePath) return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there", activemountFilePath) } else { - log.Errorf("The active mount file %s could not be opened", activemountFilePath) return false, fmt.Errorf("the active mount file %s could not be opened", activemountFilePath) } @@ -126,7 +119,6 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou if activeMountInfo.UsageCount == 0 { err := os.Remove(activemountFilePath) if err != nil { - log.Errorf("The active mount file %s could not be deleted", activemountFilePath) return false, fmt.Errorf("the active mount file %s could not be deleted", activemountFilePath) } return !otherVolumesPresent, nil @@ -137,7 +129,6 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o644) if err != nil { - log.Errorf("The active mount file %s could not be updated", activemountFilePath) return false, fmt.Errorf("the active mount file %s could not be updated", activemountFilePath) } return false, nil diff --git a/driver.go b/driver.go index 09c8471..770003c 100644 --- a/driver.go +++ b/driver.go @@ -185,6 +185,7 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse doMountFs, err := d.activateVolume(request, activemountsdir) if err != nil { + log.Errorf("Error while activating the filesystem mount: %w", err) return nil, internalError("failed to activate the active mount:", err) } else if doMountFs { lowerdir := thisVol.BaseDirPath @@ -235,6 +236,7 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { doUnmountFs, err := d.deactivateVolume(request, activemountsdir) if err != nil { + log.Errorf("Error while activating the filesystem mount: %w", err) return internalError("failed to deactivate the active mount:", err) } else if doUnmountFs { err = syscall.Unmount(d.mountpointdir(request.Name), 0) From f9aadb4df4833d2d81d15aee3ed93c30c52f9ba6 Mon Sep 17 00:00:00 2001 From: AnderG Date: Mon, 29 Jul 2024 22:13:45 +0200 Subject: [PATCH 11/19] Extend error information on activateVolume and deactivateVolume --- activemount.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/activemount.go b/activemount.go index 48fb830..1bdc8be 100644 --- a/activemount.go +++ b/activemount.go @@ -57,7 +57,7 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd // Default case, we need to create a new active mount, the filesystem needs to be mounted activeMountInfo = activeMount{UsageCount: 0} } else { - return false, fmt.Errorf("active mount file %s exists but cannot be read", activemountFilePath) + return false, fmt.Errorf("active mount file %s exists but cannot be read %w", activemountFilePath, err) } activeMountInfo.UsageCount++ @@ -68,7 +68,7 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o644) if err != nil { - return false, fmt.Errorf("active mount file %s cannot be written", activemountFilePath) + return false, fmt.Errorf("active mount file %s cannot be written %w", activemountFilePath, err) } return doMountFs, nil @@ -109,9 +109,9 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou json.Unmarshal(payload, &activeMountInfo) file.Close() } else if os.IsNotExist(err) { - return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there", activemountFilePath) + return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there %w", activemountFilePath, err) } else { - return false, fmt.Errorf("the active mount file %s could not be opened", activemountFilePath) + return false, fmt.Errorf("the active mount file %s could not be opened %w", activemountFilePath, err) } activeMountInfo.UsageCount-- @@ -119,7 +119,7 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou if activeMountInfo.UsageCount == 0 { err := os.Remove(activemountFilePath) if err != nil { - return false, fmt.Errorf("the active mount file %s could not be deleted", activemountFilePath) + return false, fmt.Errorf("the active mount file %s could not be deleted %w", activemountFilePath, err) } return !otherVolumesPresent, nil } else { @@ -129,7 +129,7 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o644) if err != nil { - return false, fmt.Errorf("the active mount file %s could not be updated", activemountFilePath) + return false, fmt.Errorf("the active mount file %s could not be updated %w", activemountFilePath, err) } return false, nil } From 38424e80e9daf97dd7c4cf5d80f9257fe30cdc6e Mon Sep 17 00:00:00 2001 From: AnderG Date: Mon, 29 Jul 2024 22:18:20 +0200 Subject: [PATCH 12/19] Improve log message when the filesystem needs no unmounting --- driver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/driver.go b/driver.go index 770003c..445366b 100644 --- a/driver.go +++ b/driver.go @@ -248,7 +248,7 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { log.Debugf("Unmounted volume %s", request.Name) } else { - log.Debugf("Volume %s is still mounted. Indicating success without unmounting", request.Name) + log.Debugf("Volume %s is still mounted in some other container. Indicating success without unmounting", request.Name) } // Report an error during cleanup, if any From 197c6feb30581a079b201dab667119abb8009af1 Mon Sep 17 00:00:00 2001 From: AnderG Date: Mon, 29 Jul 2024 22:19:40 +0200 Subject: [PATCH 13/19] Improve log message when the filesystem needs no unmounting. Actual change --- driver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/driver.go b/driver.go index 445366b..d172469 100644 --- a/driver.go +++ b/driver.go @@ -248,7 +248,7 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { log.Debugf("Unmounted volume %s", request.Name) } else { - log.Debugf("Volume %s is still mounted in some other container. Indicating success without unmounting", request.Name) + log.Debugf("Volume %s is still used in another container. Indicating success without unmounting", request.Name) } // Report an error during cleanup, if any From f6634d9b5dabdaaa5322ea9b4f000a8fed5bbcc0 Mon Sep 17 00:00:00 2001 From: AnderG Date: Mon, 29 Jul 2024 22:31:30 +0200 Subject: [PATCH 14/19] Improve error handling when reading from the activemount file --- activemount.go | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/activemount.go b/activemount.go index 1bdc8be..bd8e8de 100644 --- a/activemount.go +++ b/activemount.go @@ -50,14 +50,22 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd if err == nil { // The file can exist from a previous mount when doing a docker cp on an already mounted container, no need to mount the filesystem again - payload, _ := io.ReadAll(file) - json.Unmarshal(payload, &activeMountInfo) - file.Close() + payload, readErr := io.ReadAll(file) + closeErr := file.Close() + if readErr != nil { + return false, fmt.Errorf("active mount file %s has been opened but cannot be read %w", activemountFilePath, readErr) + } else if closeErr != nil { + return false, fmt.Errorf("active mount file %s has been opened but cannot be closed %w", activemountFilePath, readErr) + } + unmarshalErr := json.Unmarshal(payload, &activeMountInfo) + if unmarshalErr != nil { + return false, fmt.Errorf("active mount file %s contents are invalid %w", activemountFilePath, unmarshalErr) + } } else if os.IsNotExist(err) { // Default case, we need to create a new active mount, the filesystem needs to be mounted activeMountInfo = activeMount{UsageCount: 0} } else { - return false, fmt.Errorf("active mount file %s exists but cannot be read %w", activemountFilePath, err) + return false, fmt.Errorf("active mount file %s exists but cannot be opened %w", activemountFilePath, err) } activeMountInfo.UsageCount++ @@ -105,9 +113,17 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou file, err := os.Open(activemountFilePath) if err == nil { - payload, _ := io.ReadAll(file) - json.Unmarshal(payload, &activeMountInfo) - file.Close() + payload, readErr := io.ReadAll(file) + closeErr := file.Close() + if readErr != nil { + return false, fmt.Errorf("active mount file %s has been opened but cannot be read %w", activemountFilePath, readErr) + } else if closeErr != nil { + return false, fmt.Errorf("active mount file %s has been opened but cannot be closed %w", activemountFilePath, readErr) + } + unmarshalErr := json.Unmarshal(payload, &activeMountInfo) + if unmarshalErr != nil { + return false, fmt.Errorf("active mount file %s contents are invalid %w", activemountFilePath, unmarshalErr) + } } else if os.IsNotExist(err) { return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there %w", activemountFilePath, err) } else { From e441320f90110aebe797cc8e412f493e2a59cd81 Mon Sep 17 00:00:00 2001 From: AnderG Date: Mon, 29 Jul 2024 22:38:46 +0200 Subject: [PATCH 15/19] Indicate on unmount errors that the filesystem won't be unmounted --- activemount.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/activemount.go b/activemount.go index bd8e8de..ab7a40f 100644 --- a/activemount.go +++ b/activemount.go @@ -116,18 +116,18 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou payload, readErr := io.ReadAll(file) closeErr := file.Close() if readErr != nil { - return false, fmt.Errorf("active mount file %s has been opened but cannot be read %w", activemountFilePath, readErr) + return false, fmt.Errorf("active mount file %s has been opened but cannot be read %w, the filesystem won't be unmounted", activemountFilePath, readErr) } else if closeErr != nil { - return false, fmt.Errorf("active mount file %s has been opened but cannot be closed %w", activemountFilePath, readErr) + return false, fmt.Errorf("active mount file %s has been opened but cannot be closed %w, the filesystem won't be unmounted", activemountFilePath, readErr) } unmarshalErr := json.Unmarshal(payload, &activeMountInfo) if unmarshalErr != nil { - return false, fmt.Errorf("active mount file %s contents are invalid %w", activemountFilePath, unmarshalErr) + return false, fmt.Errorf("active mount file %s contents are invalid %w, the filesystem won't be unmounted", activemountFilePath, unmarshalErr) } } else if os.IsNotExist(err) { - return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there %w", activemountFilePath, err) + return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there %w, the filesystem won't be unmounted", activemountFilePath, err) } else { - return false, fmt.Errorf("the active mount file %s could not be opened %w", activemountFilePath, err) + return false, fmt.Errorf("the active mount file %s could not be opened %w, the filesystem won't be unmounted", activemountFilePath, err) } activeMountInfo.UsageCount-- @@ -135,7 +135,7 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou if activeMountInfo.UsageCount == 0 { err := os.Remove(activemountFilePath) if err != nil { - return false, fmt.Errorf("the active mount file %s could not be deleted %w", activemountFilePath, err) + return false, fmt.Errorf("the active mount file %s could not be deleted %w, the filesystem won't be unmounted", activemountFilePath, err) } return !otherVolumesPresent, nil } else { @@ -145,7 +145,7 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o644) if err != nil { - return false, fmt.Errorf("the active mount file %s could not be updated %w", activemountFilePath, err) + return false, fmt.Errorf("the active mount file %s could not be updated %w, the filesystem won't be unmounted", activemountFilePath, err) } return false, nil } From 149fe1a0d3ec5f477729e783d8f9b35a554df13c Mon Sep 17 00:00:00 2001 From: AnderG Date: Mon, 29 Jul 2024 22:50:11 +0200 Subject: [PATCH 16/19] Change activateVolume and deactivateVolume parameters not to depend on Docker structures. This way we can call them without an actual request. --- activemount.go | 24 ++++++++++++------------ driver.go | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/activemount.go b/activemount.go index ab7a40f..63cdf49 100644 --- a/activemount.go +++ b/activemount.go @@ -6,8 +6,6 @@ import ( "fmt" "io" "os" - - "github.com/docker/go-plugins-helpers/volume" ) type activeMount struct { @@ -18,19 +16,20 @@ type activeMount struct { // actually requires to be mounted (as an overlay fs mount). For that purpose check if other containers // have already mounted the volume (by reading in `activemountsdir`). It is also possible that the volume // has already been been mounted by the same container (when doing a `docker cp` while the container is running), -// in that case the file named with the `request.ID` will contain the number of times the container has +// in that case the file named with the `requestId` will contain the number of times the container has // been requested the volume to be mounted. That number will be increased each time `activateVolume` is // called and decreased on `deactivateVolume`. // Parameters: // -// request: Incoming Docker mount request. +// requestName: Name of the volume to be mounted +// requestID: ID of the container requesting the mount // activemountsdir: Folder where Docker-On-Top mounts are tracked. // // Return: // // doMountFs: Returns true if the request requires the filesystem to be mounted, false if not. // err: If the function encountered an error, the error itself, nil if everything went right. -func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsdir lockedFile) (bool, error) { +func (d *DockerOnTop) activateVolume(requestName string, requestId string, activemountsdir lockedFile) (bool, error) { var doMountFs bool _, readDirErr := activemountsdir.ReadDir(1) // Check if there are any files inside activemounts dir @@ -45,7 +44,7 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd } var activeMountInfo activeMount - activemountFilePath := d.activemountsdir(request.Name) + request.ID + activemountFilePath := d.activemountsdir(requestName) + requestId file, err := os.Open(activemountFilePath) if err == nil { @@ -84,19 +83,20 @@ func (d *DockerOnTop) activateVolume(request *volume.MountRequest, activemountsd // deactivateVolume checks if the volume that has been requested to be unmounted (as in docker volume unmounting) // actually requires to be unmounted (as an overlay fs unmount). It will check the number of times the container -// has been requested to mount the volume in the file named `request.ID` and decrease the number, when the number -// reaches zero it will delete the `request.ID` file since this container no longer mounts the volume. It will +// has been requested to mount the volume in the file named `requestId` and decrease the number, when the number +// reaches zero it will delete the `requestId` file since this container no longer mounts the volume. It will // also check if other containers are mounting this volume by checking for other files in the active mounts folder. // Parameters: // -// request: Incoming Docker unmount request. +// requestName: Name of the volume to be unmounted +// requestID: ID of the container requesting the unmount // activemountsdir: Folder where Docker-On-Top mounts are tracked. // // Return: // // doUnmountFs: Returns true if there are not other usages of this volume and the filesystem can be unmounted. // err: If the function encountered an error, the error itself, nil if everything went right. -func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemountsdir lockedFile) (bool, error) { +func (d *DockerOnTop) deactivateVolume(requestName string, requestId string, activemountsdir lockedFile) (bool, error) { dirEntries, readDirErr := activemountsdir.ReadDir(2) // Check if there is any _other_ container using the volume if errors.Is(readDirErr, io.EOF) { @@ -106,10 +106,10 @@ func (d *DockerOnTop) deactivateVolume(request *volume.UnmountRequest, activemou return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) } - otherVolumesPresent := len(dirEntries) > 1 || dirEntries[0].Name() != request.ID + otherVolumesPresent := len(dirEntries) > 1 || dirEntries[0].Name() != requestId var activeMountInfo activeMount - activemountFilePath := d.activemountsdir(request.Name) + request.ID + activemountFilePath := d.activemountsdir(requestName) + requestId file, err := os.Open(activemountFilePath) if err == nil { diff --git a/driver.go b/driver.go index d172469..59efab4 100644 --- a/driver.go +++ b/driver.go @@ -183,7 +183,7 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse } defer activemountsdir.Close() // There is nothing I could do about the error (logging is performed inside `Close()` anyway) - doMountFs, err := d.activateVolume(request, activemountsdir) + doMountFs, err := d.activateVolume(request.Name, request.ID, activemountsdir) if err != nil { log.Errorf("Error while activating the filesystem mount: %w", err) return nil, internalError("failed to activate the active mount:", err) @@ -234,7 +234,7 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { } defer activemountsdir.Close() // There's nothing I could do about the error if it occurs - doUnmountFs, err := d.deactivateVolume(request, activemountsdir) + doUnmountFs, err := d.deactivateVolume(request.Name, request.ID, activemountsdir) if err != nil { log.Errorf("Error while activating the filesystem mount: %w", err) return internalError("failed to deactivate the active mount:", err) From 01de48d2fe2c0cfc4fb01f1fd34247ca88a11e98 Mon Sep 17 00:00:00 2001 From: AnderG Date: Mon, 29 Jul 2024 22:56:35 +0200 Subject: [PATCH 17/19] Improve Mount error mangement --- driver.go | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/driver.go b/driver.go index 59efab4..43d52fa 100644 --- a/driver.go +++ b/driver.go @@ -201,14 +201,25 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse options := "lowerdir=" + lowerdir + ",upperdir=" + upperdir + ",workdir=" + workdir err = syscall.Mount("docker-on-top_"+request.Name, mountpoint, "overlay", 0, options) - if os.IsNotExist(err) { - log.Errorf("Failed to mount overlay for volume %s because something does not exist: %v", - request.Name, err) - return nil, errors.New("failed to mount volume: something is missing (does the base directory " + - "exist?)") - } else if err != nil { - log.Errorf("Failed to mount overlay for volume %s: %v", request.Name, err) - return nil, internalError("failed to mount overlay", err) + if err != nil { + // The filesystem could not be mounted so undo the activateVolume call so it does not appear as if + // we are using a volume that we couln't mount. We can ignore the doUnmountFs because we know the volume + // is not mounted. + _, deactivateErr := d.deactivateVolume(request.Name, request.ID, activemountsdir) + if deactivateErr != nil { + log.Errorf("Additional error while deactivating the filesystem mount: %w", err) + // Do not return the error since we are dealing with a more important one + } + + if os.IsNotExist(err) { + log.Errorf("Failed to mount overlay for volume %s because something does not exist: %v", + request.Name, err) + return nil, errors.New("failed to mount volume: something is missing (does the base directory " + + "exist?)") + } else { + log.Errorf("Failed to mount overlay for volume %s: %v", request.Name, err) + return nil, internalError("failed to mount overlay", err) + } } log.Debugf("Mounted volume %s at %s", request.Name, mountpoint) } else { From ebe57ea0ae15389ac1b4fdd349e9fa168d7bd154 Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Wed, 31 Jul 2024 12:54:33 +0300 Subject: [PATCH 18/19] Minor docs/comments/error-messages fixes --- activemount.go | 16 +++++++--------- driver.go | 4 ++-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/activemount.go b/activemount.go index 63cdf49..c92c6b9 100644 --- a/activemount.go +++ b/activemount.go @@ -19,14 +19,13 @@ type activeMount struct { // in that case the file named with the `requestId` will contain the number of times the container has // been requested the volume to be mounted. That number will be increased each time `activateVolume` is // called and decreased on `deactivateVolume`. -// Parameters: // +// Parameters: // requestName: Name of the volume to be mounted // requestID: ID of the container requesting the mount // activemountsdir: Folder where Docker-On-Top mounts are tracked. // // Return: -// // doMountFs: Returns true if the request requires the filesystem to be mounted, false if not. // err: If the function encountered an error, the error itself, nil if everything went right. func (d *DockerOnTop) activateVolume(requestName string, requestId string, activemountsdir lockedFile) (bool, error) { @@ -52,25 +51,25 @@ func (d *DockerOnTop) activateVolume(requestName string, requestId string, activ payload, readErr := io.ReadAll(file) closeErr := file.Close() if readErr != nil { - return false, fmt.Errorf("active mount file %s has been opened but cannot be read %w", activemountFilePath, readErr) + return false, fmt.Errorf("active mount file %s has been opened but cannot be read: %w", activemountFilePath, readErr) } else if closeErr != nil { - return false, fmt.Errorf("active mount file %s has been opened but cannot be closed %w", activemountFilePath, readErr) + return false, fmt.Errorf("active mount file %s has been opened but cannot be closed: %w", activemountFilePath, readErr) } unmarshalErr := json.Unmarshal(payload, &activeMountInfo) if unmarshalErr != nil { - return false, fmt.Errorf("active mount file %s contents are invalid %w", activemountFilePath, unmarshalErr) + return false, fmt.Errorf("active mount file %s contents are invalid: %w", activemountFilePath, unmarshalErr) } } else if os.IsNotExist(err) { // Default case, we need to create a new active mount, the filesystem needs to be mounted activeMountInfo = activeMount{UsageCount: 0} } else { - return false, fmt.Errorf("active mount file %s exists but cannot be opened %w", activemountFilePath, err) + return false, fmt.Errorf("active mount file %s exists but cannot be opened: %w", activemountFilePath, err) } activeMountInfo.UsageCount++ // Convert activeMountInfo to JSON to store it in a file. We can safely ignore Marshal errors, since the - // activeMount structure is simple enought not to contain "strage" floats, unsupported datatypes or cycles + // activeMount structure is simple enought not to contain "strange" floats, unsupported datatypes or cycles, // which are the error causes for json.Marshal payload, _ := json.Marshal(activeMountInfo) err = os.WriteFile(activemountFilePath, payload, 0o644) @@ -86,14 +85,13 @@ func (d *DockerOnTop) activateVolume(requestName string, requestId string, activ // has been requested to mount the volume in the file named `requestId` and decrease the number, when the number // reaches zero it will delete the `requestId` file since this container no longer mounts the volume. It will // also check if other containers are mounting this volume by checking for other files in the active mounts folder. -// Parameters: // +// Parameters: // requestName: Name of the volume to be unmounted // requestID: ID of the container requesting the unmount // activemountsdir: Folder where Docker-On-Top mounts are tracked. // // Return: -// // doUnmountFs: Returns true if there are not other usages of this volume and the filesystem can be unmounted. // err: If the function encountered an error, the error itself, nil if everything went right. func (d *DockerOnTop) deactivateVolume(requestName string, requestId string, activemountsdir lockedFile) (bool, error) { diff --git a/driver.go b/driver.go index 43d52fa..c0423df 100644 --- a/driver.go +++ b/driver.go @@ -186,7 +186,7 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse doMountFs, err := d.activateVolume(request.Name, request.ID, activemountsdir) if err != nil { log.Errorf("Error while activating the filesystem mount: %w", err) - return nil, internalError("failed to activate the active mount:", err) + return nil, internalError("failed to activate an active mount:", err) } else if doMountFs { lowerdir := thisVol.BaseDirPath upperdir := d.upperdir(request.Name) @@ -207,7 +207,7 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse // is not mounted. _, deactivateErr := d.deactivateVolume(request.Name, request.ID, activemountsdir) if deactivateErr != nil { - log.Errorf("Additional error while deactivating the filesystem mount: %w", err) + log.Errorf("Additional error while deactivating the filesystem mount: %v", err) // Do not return the error since we are dealing with a more important one } From 10b7f9c63358d5dca9b73c27f9fa5f9dff8ea390 Mon Sep 17 00:00:00 2001 From: AnderG Date: Wed, 21 Aug 2024 22:30:56 +0200 Subject: [PATCH 19/19] Fix issues raised in the review --- activemount.go | 77 +++++++++++++++++++++++--------------------------- driver.go | 4 +-- 2 files changed, 38 insertions(+), 43 deletions(-) diff --git a/activemount.go b/activemount.go index c92c6b9..df86cf0 100644 --- a/activemount.go +++ b/activemount.go @@ -21,13 +21,16 @@ type activeMount struct { // called and decreased on `deactivateVolume`. // // Parameters: +// // requestName: Name of the volume to be mounted -// requestID: ID of the container requesting the mount +// requestID: Unique ID for the volume-container pair requesting the mount // activemountsdir: Folder where Docker-On-Top mounts are tracked. // // Return: -// doMountFs: Returns true if the request requires the filesystem to be mounted, false if not. -// err: If the function encountered an error, the error itself, nil if everything went right. +// +// doMountFs: `true` if the caller should mount the filesystem, `false` otherwise. +// If an error is returned, `doMountFs` is always `false`. +// err: An error, if encountered, `nil` otherwise. func (d *DockerOnTop) activateVolume(requestName string, requestId string, activemountsdir lockedFile) (bool, error) { var doMountFs bool @@ -39,31 +42,25 @@ func (d *DockerOnTop) activateVolume(requestName string, requestId string, activ // The directory is empty, mount the filesystem doMountFs = true } else { - return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) + return false, fmt.Errorf("failed to list activemounts/ : %w", readDirErr) } var activeMountInfo activeMount activemountFilePath := d.activemountsdir(requestName) + requestId - file, err := os.Open(activemountFilePath) - if err == nil { + payload, readErr := os.ReadFile(activemountFilePath) + + if readErr == nil { // The file can exist from a previous mount when doing a docker cp on an already mounted container, no need to mount the filesystem again - payload, readErr := io.ReadAll(file) - closeErr := file.Close() - if readErr != nil { - return false, fmt.Errorf("active mount file %s has been opened but cannot be read: %w", activemountFilePath, readErr) - } else if closeErr != nil { - return false, fmt.Errorf("active mount file %s has been opened but cannot be closed: %w", activemountFilePath, readErr) - } unmarshalErr := json.Unmarshal(payload, &activeMountInfo) if unmarshalErr != nil { return false, fmt.Errorf("active mount file %s contents are invalid: %w", activemountFilePath, unmarshalErr) } - } else if os.IsNotExist(err) { + } else if os.IsNotExist(readErr) { // Default case, we need to create a new active mount, the filesystem needs to be mounted activeMountInfo = activeMount{UsageCount: 0} } else { - return false, fmt.Errorf("active mount file %s exists but cannot be opened: %w", activemountFilePath, err) + return false, fmt.Errorf("active mount file %s exists but cannot be read: %w", activemountFilePath, readErr) } activeMountInfo.UsageCount++ @@ -71,10 +68,10 @@ func (d *DockerOnTop) activateVolume(requestName string, requestId string, activ // Convert activeMountInfo to JSON to store it in a file. We can safely ignore Marshal errors, since the // activeMount structure is simple enought not to contain "strange" floats, unsupported datatypes or cycles, // which are the error causes for json.Marshal - payload, _ := json.Marshal(activeMountInfo) - err = os.WriteFile(activemountFilePath, payload, 0o644) - if err != nil { - return false, fmt.Errorf("active mount file %s cannot be written %w", activemountFilePath, err) + payload, _ = json.Marshal(activeMountInfo) + writeErr := os.WriteFile(activemountFilePath, payload, 0o644) + if writeErr != nil { + return false, fmt.Errorf("active mount file %s cannot be written %w", activemountFilePath, writeErr) } return doMountFs, nil @@ -87,45 +84,43 @@ func (d *DockerOnTop) activateVolume(requestName string, requestId string, activ // also check if other containers are mounting this volume by checking for other files in the active mounts folder. // // Parameters: +// // requestName: Name of the volume to be unmounted -// requestID: ID of the container requesting the unmount +// requestID: Unique ID for the volume-container pair requesting the mount // activemountsdir: Folder where Docker-On-Top mounts are tracked. // // Return: -// doUnmountFs: Returns true if there are not other usages of this volume and the filesystem can be unmounted. -// err: If the function encountered an error, the error itself, nil if everything went right. +// +// doUnmountFs: `true` if there are no other usages of this volume and the filesystem should be unmounted +// by the caller. If an error is returned, `doMountFs` is always `false`. +// err: An error, if encountered, `nil` otherwise. func (d *DockerOnTop) deactivateVolume(requestName string, requestId string, activemountsdir lockedFile) (bool, error) { dirEntries, readDirErr := activemountsdir.ReadDir(2) // Check if there is any _other_ container using the volume if errors.Is(readDirErr, io.EOF) { // If directory is empty, unmount overlay and clean up - return true, fmt.Errorf("there are no active mount files and one was expected. Unmounting") + log.Warning("there are no active mount files and one was expected. the filesystem will be unmounted") + return true, nil } else if readDirErr != nil { - return false, fmt.Errorf("failed to list activemounts/ %v", readDirErr) + return false, fmt.Errorf("failed to list activemounts/ %w", readDirErr) } otherVolumesPresent := len(dirEntries) > 1 || dirEntries[0].Name() != requestId - var activeMountInfo activeMount + var activeMountInfo activeMount activemountFilePath := d.activemountsdir(requestName) + requestId - file, err := os.Open(activemountFilePath) - - if err == nil { - payload, readErr := io.ReadAll(file) - closeErr := file.Close() - if readErr != nil { - return false, fmt.Errorf("active mount file %s has been opened but cannot be read %w, the filesystem won't be unmounted", activemountFilePath, readErr) - } else if closeErr != nil { - return false, fmt.Errorf("active mount file %s has been opened but cannot be closed %w, the filesystem won't be unmounted", activemountFilePath, readErr) - } + + payload, readErr := os.ReadFile(activemountFilePath) + + if readErr == nil { unmarshalErr := json.Unmarshal(payload, &activeMountInfo) if unmarshalErr != nil { return false, fmt.Errorf("active mount file %s contents are invalid %w, the filesystem won't be unmounted", activemountFilePath, unmarshalErr) } - } else if os.IsNotExist(err) { - return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there %w, the filesystem won't be unmounted", activemountFilePath, err) + } else if os.IsNotExist(readErr) { + return !otherVolumesPresent, fmt.Errorf("the active mount file %s was expected but is not there %w, the filesystem won't be unmounted", activemountFilePath, readErr) } else { - return false, fmt.Errorf("the active mount file %s could not be opened %w, the filesystem won't be unmounted", activemountFilePath, err) + return false, fmt.Errorf("the active mount file %s could not be opened %w, the filesystem won't be unmounted", activemountFilePath, readErr) } activeMountInfo.UsageCount-- @@ -141,9 +136,9 @@ func (d *DockerOnTop) deactivateVolume(requestName string, requestId string, act // activeMount structure is simple enought not to contain "strage" floats, unsupported datatypes or cycles // which are the error causes for json.Marshal payload, _ := json.Marshal(activeMountInfo) - err = os.WriteFile(activemountFilePath, payload, 0o644) - if err != nil { - return false, fmt.Errorf("the active mount file %s could not be updated %w, the filesystem won't be unmounted", activemountFilePath, err) + writeErr := os.WriteFile(activemountFilePath, payload, 0o644) + if writeErr != nil { + return false, fmt.Errorf("the active mount file %s could not be updated %w, the filesystem won't be unmounted", activemountFilePath, writeErr) } return false, nil } diff --git a/driver.go b/driver.go index c0423df..9f57363 100644 --- a/driver.go +++ b/driver.go @@ -185,7 +185,7 @@ func (d *DockerOnTop) Mount(request *volume.MountRequest) (*volume.MountResponse doMountFs, err := d.activateVolume(request.Name, request.ID, activemountsdir) if err != nil { - log.Errorf("Error while activating the filesystem mount: %w", err) + log.Errorf("Error while activating the filesystem mount: %v", err) return nil, internalError("failed to activate an active mount:", err) } else if doMountFs { lowerdir := thisVol.BaseDirPath @@ -247,7 +247,7 @@ func (d *DockerOnTop) Unmount(request *volume.UnmountRequest) error { doUnmountFs, err := d.deactivateVolume(request.Name, request.ID, activemountsdir) if err != nil { - log.Errorf("Error while activating the filesystem mount: %w", err) + log.Errorf("Error while activating the filesystem mount: %v", err) return internalError("failed to deactivate the active mount:", err) } else if doUnmountFs { err = syscall.Unmount(d.mountpointdir(request.Name), 0)