Skip to content

plan9: allow distinct Plan9 share configurations for the same host path #2939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Harsh Rawat (rawahars) wants to merge 1 commit into
base: main
Choose a base branch
from
Open

plan9: allow distinct Plan9 share configurations for the same host path #2939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 51 additions & 60 deletions internal/controller/device/plan9/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,15 @@ type Controller struct {
// Immutable after construction.
noWritableFileShares bool

// reservations maps a reservation ID to its share host path.
// reservations maps a reservation ID to its share and guest mount.
// Guarded by mu.
reservations map[guid.GUID]*reservation

// sharesByHostPath maps a host path to its share for fast deduplication
// of share additions. Guarded by mu.
sharesByHostPath map[string]*share.Share
// sharesByHostPath groups shares by host path. Different share configurations
// create separate shares. The same share configuration reuses an existing share,
// where identical guest mount configurations share a reference-counted mount.
// Guarded by mu.
sharesByHostPath map[string]map[*share.Share]struct{}

// nameCounter is the monotonically increasing index used to generate
// unique share names. Guarded by mu.
Expand All @@ -69,7 +71,7 @@ func New(vm vmPlan9, guest guestPlan9, noWritableFileShares bool) *Controller {
guest: guest,
noWritableFileShares: noWritableFileShares,
reservations: make(map[guid.GUID]*reservation),
sharesByHostPath: make(map[string]*share.Share),
sharesByHostPath: make(map[string]map[*share.Share]struct{}),
}
}

Expand Down Expand Up @@ -103,51 +105,40 @@ func (c *Controller) Reserve(ctx context.Context, shareConfig share.Config, moun
return guid.GUID{}, fmt.Errorf("reservation ID already exists: %s", id)
}

// Create the reservation entry.
res := &reservation{
hostPath: shareConfig.HostPath,
}

// Check whether this host path already has an allocated share.
existingShare, ok := c.sharesByHostPath[shareConfig.HostPath]

// We have an existing share for this host path — reserve a mount on it for this caller.
if ok {
// Verify the caller is requesting the same share configuration.
if !existingShare.Config().Equals(shareConfig) {
return guid.GUID{}, fmt.Errorf("cannot reserve ref on share with different config")
}

// Set the share name.
res.name = existingShare.Name()

// We have a share, now reserve a mount on it.
if _, err = existingShare.ReserveMount(ctx, mountConfig); err != nil {
return guid.GUID{}, fmt.Errorf("reserve mount on share %s: %w", existingShare.Name(), err)
// Look for a matching configuration among shares registered for this host path.
shares := c.sharesByHostPath[shareConfig.HostPath]
var selected *share.Share
for existing := range shares {
if existing.Config().Equals(shareConfig) {
selected = existing
break
}
}

// If we don't have an existing share, we need to create one and reserve a mount on it.
if !ok {
// No existing share for this path — allocate a new one.
// Allocate a new share when this host path has no matching configuration.
if selected == nil {
name := strconv.FormatUint(c.nameCounter, 10)
c.nameCounter++
selected = share.NewReserved(name, shareConfig)
}

// Create the Share and Mount in the reserved states.
newShare := share.NewReserved(name, shareConfig)
if _, err = newShare.ReserveMount(ctx, mountConfig); err != nil {
return guid.GUID{}, fmt.Errorf("reserve mount on share %s: %w", name, err)
}
// Reserve the requested guest mount before registering any new share.
guestMount, err := selected.ReserveMount(ctx, mountConfig)
if err != nil {
return guid.GUID{}, fmt.Errorf("reserve mount on share %s: %w", selected.Name(), err)
}

c.sharesByHostPath[shareConfig.HostPath] = newShare
res.name = newShare.Name()
// Register the share under its real host path after reservation succeeds.
if shares == nil {
shares = make(map[*share.Share]struct{})
c.sharesByHostPath[shareConfig.HostPath] = shares
}
shares[selected] = struct{}{}

// Ensure our reservation is saved for all future operations.
c.reservations[id] = res
// Record the exact share and mount for subsequent mapping and cleanup.
c.reservations[id] = &reservation{share: selected, mount: guestMount}
log.G(ctx).WithField("reservation", id).Debug("Plan9 share reserved")

// Return the reserved guest path in addition to the reservation ID for caller convenience.
return id, nil
}

Expand All @@ -164,12 +155,11 @@ func (c *Controller) MapToGuest(ctx context.Context, id guid.GUID) (string, erro
return "", fmt.Errorf("reservation %s not found", id)
}

// Validate if the host path has an associated share.
// This should be reserved by the Reserve() call.
existingShare, ok := c.sharesByHostPath[res.hostPath]
if !ok {
return "", fmt.Errorf("share for host path %s not found", res.hostPath)
// Reject remapping a reservation whose guest-mount reference was already released.
if res.mount == nil {
return "", fmt.Errorf("reservation %s is being released", id)
}
existingShare := res.share

log.G(ctx).WithField(logfields.HostPath, existingShare.HostPath()).Debug("mapping Plan9 share to guest")

Expand All @@ -178,8 +168,8 @@ func (c *Controller) MapToGuest(ctx context.Context, id guid.GUID) (string, erro
return "", fmt.Errorf("add share to VM: %w", err)
}

// Mount the share inside the guest.
guestPath, err := existingShare.MountToGuest(ctx, c.guest)
// Mount the guest configuration selected by this reservation.
guestPath, err := existingShare.MountToGuest(ctx, c.guest, res.mount)
if err != nil {
return "", fmt.Errorf("mount share to guest: %w", err)
}
Expand All @@ -203,30 +193,31 @@ func (c *Controller) UnmapFromGuest(ctx context.Context, id guid.GUID) error {
return fmt.Errorf("reservation %s not found", id)
}

// Validate that the share exists before proceeding with teardown.
// This should be reserved by the Reserve() call.
existingShare, ok := c.sharesByHostPath[res.hostPath]
if !ok {
return fmt.Errorf("share for host path %s not found", res.hostPath)
}

// Use the reserved share, not another configuration registered for the same host path.
existingShare := res.share
log.G(ctx).WithField(logfields.HostPath, existingShare.HostPath()).Debug("unmapping Plan9 share from guest")

// Unmount the share from the guest (ref-counted; only issues the guest
// call when this is the last res on the share).
if err := existingShare.UnmountFromGuest(ctx, c.guest); err != nil {
return fmt.Errorf("unmount share from guest: %w", err)
// Release only this caller's guest-mount reference; other mounts keep the share alive.
if res.mount != nil {
if err := existingShare.UnmountFromGuest(ctx, c.guest, res.mount); err != nil {
return fmt.Errorf("unmount share from guest: %w", err)
}
// A host-removal retry must not release another mount reference.
res.mount = nil
}

// Remove the share from the VM when no mounts remain active.
if err := existingShare.RemoveFromVM(ctx, c.vmPlan9); err != nil {
return fmt.Errorf("remove share from VM: %w", err)
}

// If the share is now fully removed, free its entry for reuse.
// If it's used in other reservations, it will remain until the last one is released.
// Remove only this share; retain the host-path entry while other variants remain.
if existingShare.State() == share.StateRemoved {
delete(c.sharesByHostPath, existingShare.HostPath())
shares := c.sharesByHostPath[existingShare.HostPath()]
delete(shares, existingShare)
if len(shares) == 0 {
delete(c.sharesByHostPath, existingShare.HostPath())
}
log.G(ctx).Debug("Plan9 share freed")
}

Expand Down
158 changes: 140 additions & 18 deletions internal/controller/device/plan9/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ package plan9
import (
"context"
"errors"
"fmt"
"strings"
"testing"

"github.com/Microsoft/go-winio/pkg/guid"
"go.uber.org/mock/gomock"

"github.com/Microsoft/hcsshim/internal/controller/device/plan9/mount"
Expand All @@ -15,6 +18,7 @@ import (
sharemocks "github.com/Microsoft/hcsshim/internal/controller/device/plan9/share/mocks"
hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
"github.com/Microsoft/hcsshim/internal/vm/vmutils"
)

var (
Expand Down Expand Up @@ -148,19 +152,116 @@ func TestReserve_DifferentHostPaths_CreatesSeparateShares(t *testing.T) {
}
}

// TestReserve_DifferentConfig_SameHostPath_Errors verifies that attempting to
// reserve a host path with a different config (e.g., ReadOnly differs) when a
// share already exists for that path returns an error.
func TestReserve_DifferentConfig_SameHostPath_Errors(t *testing.T) {
// TestFullLifecycle_ShareAndMountConfigs verifies configuration-based reuse and
// independent cleanup of shares and guest mounts for the same host path.
func TestFullLifecycle_ShareAndMountConfigs(t *testing.T) {
t.Parallel()
tc := newTestController(t, false)

_, _ = tc.c.Reserve(tc.ctx, share.Config{HostPath: "/host/path"}, mount.Config{})

// Same host path but read-only flag differs.
_, err := tc.c.Reserve(tc.ctx, share.Config{HostPath: "/host/path", ReadOnly: true}, mount.Config{})
if err == nil {
t.Fatal("expected error when re-reserving same host path with different config")
tests := []struct {
name string
shares [2]share.Config
mounts [2]mount.Config
wantShares int
wantMounts int
}{
{name: "identical", wantShares: 1, wantMounts: 1},
{
name: "host-access", wantShares: 2, wantMounts: 2,
shares: [2]share.Config{{}, {ReadOnly: true}},
mounts: [2]mount.Config{{}, {ReadOnly: true}},
},
{
name: "guest-access", wantShares: 1, wantMounts: 2,
mounts: [2]mount.Config{{}, {ReadOnly: true}},
},
{
name: "guest-readonly-first", wantShares: 1, wantMounts: 2,
mounts: [2]mount.Config{{ReadOnly: true}, {}},
},
{
name: "allowed-files", wantShares: 2, wantMounts: 2,
shares: [2]share.Config{
{Restrict: true, AllowedNames: []string{"first"}},
{Restrict: true, AllowedNames: []string{"second"}},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
tc := newTestController(t, false)
var ids [2]guid.GUID
var guestSettings [2]guestresource.LCOWMappedDirectory
var removals [2]hcsschema.Plan9Share
// Create separate host shares and guest mounts only when their settings differ.
for i := range ids {
tt.shares[i].HostPath = "/host/path"
id, err := tc.c.Reserve(tc.ctx, tt.shares[i], tt.mounts[i])
if err != nil {
t.Fatalf("reserve %d: %v", i, err)
}
ids[i] = id
name := "0"
if tt.wantShares == 2 {
name = fmt.Sprint(i)
}
flags := hcsschema.Plan9ShareFlagsLinuxMetadata
if tt.shares[i].ReadOnly {
flags |= hcsschema.Plan9ShareFlagsReadOnly
}
if tt.shares[i].Restrict {
flags |= hcsschema.Plan9ShareFlagsRestrictFileAccess
}
if i == 0 || tt.wantShares == 2 {
tc.vmAdd.EXPECT().AddPlan9(gomock.Any(), hcsschema.Plan9Share{
Name: name, AccessName: name, Path: "/host/path", Port: vmutils.Plan9Port,
Flags: flags, AllowedFiles: tt.shares[i].AllowedNames,
}).Return(nil)
}
guestSettings[i] = guestresource.LCOWMappedDirectory{
MountPath: fmt.Sprintf(mount.GuestPathFmt, name, tt.mounts[i].Key()),
ShareName: name, Port: vmutils.Plan9Port, ReadOnly: tt.mounts[i].ReadOnly,
}
removals[i] = hcsschema.Plan9Share{Name: name, AccessName: name, Port: vmutils.Plan9Port}
if i == 0 || tt.wantMounts == 2 {
tc.guestMount.EXPECT().AddMappedDirectory(gomock.Any(), guestSettings[i]).Return(nil)
}
if got, err := tc.c.MapToGuest(tc.ctx, id); err != nil || got != guestSettings[i].MountPath {
t.Fatalf("map %d = (%q, %v), want %q", i, got, err, guestSettings[i].MountPath)
}
}
// Count share variants, rather than host paths, when checking reuse.
if ids[0] == ids[1] || len(tc.c.sharesByHostPath["/host/path"]) != tt.wantShares {
t.Fatal("unexpected reservation identity or host-share count")
}
if err := tc.c.Save(); err == nil || !strings.Contains(err.Error(), "1 host paths, 2 reservations") {
t.Fatalf("unexpected save result: %v", err)
}
// Release the first caller without disrupting the remaining share or mount.
if tt.wantMounts == 2 {
tc.guestUnmount.EXPECT().RemoveMappedDirectory(gomock.Any(), guestSettings[0]).Return(nil)
}
if tt.wantShares == 2 {
tc.vmRemove.EXPECT().RemovePlan9(gomock.Any(), removals[0]).Return(nil)
}
if err := tc.c.UnmapFromGuest(tc.ctx, ids[0]); err != nil {
t.Fatal(err)
}
if len(tc.c.sharesByHostPath["/host/path"]) != 1 {
t.Fatal("release removed the surviving share")
}
if got, err := tc.c.MapToGuest(tc.ctx, ids[1]); err != nil || got != guestSettings[1].MountPath {
t.Fatalf("surviving mount = (%q, %v)", got, err)
}
// The last caller releases the remaining guest mount and host share.
tc.guestUnmount.EXPECT().RemoveMappedDirectory(gomock.Any(), guestSettings[1]).Return(nil)
tc.vmRemove.EXPECT().RemovePlan9(gomock.Any(), removals[1]).Return(nil)
if err := tc.c.UnmapFromGuest(tc.ctx, ids[1]); err != nil {
t.Fatal(err)
}
if len(tc.c.reservations) != 0 || len(tc.c.sharesByHostPath) != 0 {
t.Fatal("resources remain after both callers release their mounts")
}
})
}
}

Expand Down Expand Up @@ -403,10 +504,8 @@ func TestUnmapFromGuest_GuestUnmountFails_Retryable(t *testing.T) {
}
}

// TestUnmapFromGuest_VMRemoveFails_Retryable verifies that when the guest
// unmount succeeds but VM removal fails, the reservation is preserved for
// retry. On retry only VM removal is re-attempted — the guest unmount is not
// re-issued.
// TestUnmapFromGuest_VMRemoveFails_Retryable verifies that teardown retries
// preserve a guest mount acquired by a later caller.
func TestUnmapFromGuest_VMRemoveFails_Retryable(t *testing.T) {
t.Parallel()
tc := newTestController(t, false)
Expand All @@ -427,11 +526,23 @@ func TestUnmapFromGuest_VMRemoveFails_Retryable(t *testing.T) {
t.Error("reservation should remain for retry after failed VM remove")
}

// Retry succeeds.
tc.vmRemove.EXPECT().RemovePlan9(gomock.Any(), gomock.Any()).Return(nil)
// A new caller reuses the host share while the old removal is pending.
next, err := tc.c.Reserve(tc.ctx, share.Config{HostPath: "/host/path"}, mount.Config{})
if err != nil {
t.Fatal(err)
}
tc.guestMount.EXPECT().AddMappedDirectory(gomock.Any(), gomock.Any()).Return(nil)
if _, err := tc.c.MapToGuest(tc.ctx, next); err != nil {
t.Fatal(err)
}
if err := tc.c.UnmapFromGuest(tc.ctx, id); err != nil {
t.Fatalf("retry UnmapFromGuest failed: %v", err)
}
tc.guestUnmount.EXPECT().RemoveMappedDirectory(gomock.Any(), gomock.Any()).Return(nil)
tc.vmRemove.EXPECT().RemovePlan9(gomock.Any(), gomock.Any()).Return(nil)
if err := tc.c.UnmapFromGuest(tc.ctx, next); err != nil {
t.Fatalf("release new caller: %v", err)
}
}

// TestUnmapFromGuest_RefCounting_VMRemoveOnLastRef verifies that with two
Expand Down Expand Up @@ -478,10 +589,21 @@ func TestUnmapFromGuest_WithoutMapToGuest_CleansUp(t *testing.T) {

// Reserve but never MapToGuest — no VM or guest calls expected.
id, _ := tc.c.Reserve(tc.ctx, share.Config{HostPath: "/host/path"}, mount.Config{})
// Keep a second guest configuration reserved while releasing the first.
other, err := tc.c.Reserve(tc.ctx, share.Config{HostPath: "/host/path"}, mount.Config{ReadOnly: true})
if err != nil {
t.Fatal(err)
}

if err := tc.c.UnmapFromGuest(tc.ctx, id); err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(tc.c.sharesByHostPath["/host/path"]) != 1 {
t.Fatal("share removed while another guest mount is still reserved")
}
if err := tc.c.UnmapFromGuest(tc.ctx, other); err != nil {
t.Fatal(err)
}
if len(tc.c.reservations) != 0 {
t.Errorf("expected 0 reservations, got %d", len(tc.c.reservations))
}
Expand Down
Loading
Loading