RemoveVSMB uses the wrong hostPath for file shares #1974

helsaawy · 2023-11-30T20:25:58Z

When attaching a file, foo/bar, uvm.AddVSMB adds the VSMBShareto uvm.vsmbFileShares as foo/, and then also sets VSMBShare.HostPath to foo/.
So, when (*VSMBShare).Release calls uvm.RemoveVSMB, it uses foo/ as the hostPath, which, it then assumes is a directory share (stored in uvm.vsmbDirShares) instead of a file share (stored in uvm.vsmbFileShares) since foo/ is a directory. This then fails since it cannot find the VSMBShare in uvm.vsmbDirShares.

Fix this by adding a VSMBShare.isDirShare field to indicate which uvm.vsmb*Shares map it is stored under.

When attaching a file, `foo/bar`, `uvm.AddVSMB` adds the `VSMBShare`to `uvm.vsmbFileShares` as `foo/`, and then also sets `VSMBShare.HostPath` to `foo/`. So, when `(*VSMBShare).Release` calls `uvm.RemoveVSMB`, it uses `foo/` as the `hostPath`, which, it then assumes is a directory share (stored in `uvm.vsmbDirShares`) instead of a file share (stored in `uvm.vsmbFileShares`) since `foo/` is a directory. This then fails since it cannot find the VSMBShare in `uvm.vsmbDirShares`. Fix this by adding a `VSMBShare.isDirShare` field to indicate which `uvm.vsmb*Shares` map it is stored under. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

katiewasnothere · 2023-11-30T21:10:11Z

internal/uvm/vsmb.go

+	st, err := os.Stat(hostPath)
+	if err != nil {
+		return err
+	}
+	isDir := st.IsDir()
+	if !isDir {
+		hostPath = filepath.Dir(hostPath)
+	}


why do we need to do this if we've already determined that a share is or isn't a directory in AddVSMB?

Mostly backwards compat, but also, RemoveVSMB may be called on the original file name (instead of via Release), so we still need the logic

Update and un-skip WCOW uVM and container tests (and add WCOW uVM benchmarks), as well as WCOW vSMB and LCOW boto files tests. Add WCOW host process tests, including dedicated tests for setting username, and verifying hostname and volume mounts. Moved: - `lcow_bench_test.go` to `uvm_bench_test.go` - `lcow_container_test.go` to `container_test.go` - `lcow_test.go` to `lcow_uvm_test.go` and `uvm_test.go` Fix bug where removing a direct-mapped vSMB share fails. Run (non-virtualization/uVM) functional tests within CI. Add `util.Context` function to create context that times out before test timeout, to help with timing issues and allow time for cleanup and logging. Make sure container specs are created with the default working directory (`C:\`), similar to how `internal\cmd` works). Rename `cri_util` to `criutil`, since underscores are frowned upon in package names. Relies on PR: microsoft#1974 Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

Update and un-skip WCOW uVM and container tests (and add WCOW uVM benchmarks), as well as WCOW vSMB and LCOW boto files tests. Add WCOW host process tests, including dedicated tests for setting username, and verifying hostname and volume mounts. Moved: - `lcow_bench_test.go` to `uvm_bench_test.go` - `lcow_container_test.go` to `container_test.go` - `lcow_test.go` to `lcow_uvm_test.go` and `uvm_test.go` Fix bug where removing a direct-mapped vSMB share fails. Run (non-virtualization/uVM) functional tests within CI. Make sure container specs are created with the default working directory (`C:\`), similar to how `internal\cmd` works). Relies on PR: microsoft#1974 Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

helsaawy requested a review from a team as a code owner November 30, 2023 20:25

katiewasnothere reviewed Nov 30, 2023

View reviewed changes

helsaawy changed the title ~~Remove vSMB uses the wrong hostPath for file shares~~ RemoveVSMB uses the wrong hostPath for file shares Dec 4, 2023

katiewasnothere approved these changes Dec 5, 2023

View reviewed changes

anmaxvl approved these changes Dec 5, 2023

View reviewed changes

helsaawy merged commit 04f386f into microsoft:main Dec 5, 2023
18 of 19 checks passed

helsaawy deleted the vsmb-remove-bug branch December 5, 2023 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RemoveVSMB uses the wrong hostPath for file shares #1974

RemoveVSMB uses the wrong hostPath for file shares #1974

helsaawy commented Nov 30, 2023

katiewasnothere Nov 30, 2023

helsaawy Dec 1, 2023

RemoveVSMB uses the wrong hostPath for file shares #1974

RemoveVSMB uses the wrong hostPath for file shares #1974

Conversation

helsaawy commented Nov 30, 2023

katiewasnothere Nov 30, 2023

Choose a reason for hiding this comment

helsaawy Dec 1, 2023

Choose a reason for hiding this comment