Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 1 addition & 3 deletions internal/controller/vm/save_lcow.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,6 @@ func (c *Controller) Import(ctx context.Context, env *anypb.Any) error {
// the controller migrating so only migration APIs are permitted.
c.vmID = state.GetVmID()
c.sandboxOptions = sandboxOptionsFromProto(state.GetSandboxOptions())
if c.sandboxOptions != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need sandboxOptions.FullyPhysicallyBacked option?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we still need and retain sandboxOptions.FullyPhysicallyBacked. This change only stops using it as the stats predicate. Effective memory backing is !Memory.AllowOvercommit, while FullyPhysicallyBacked additionally controls device backing (for example, disabling VPMEM in favor of SCSI) and is still round-tripped in the migration payload. A caller can set AllowOvercommit=false without setting FullyPhysicallyBacked; that was the failing case. This mirrors V1's separate physicallyBacked and devicesPhysicallyBacked fields.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key is whether the option remains required for device/VPMEM behavior even though it is no longer the right stats predicate.

c.isPhysicallyBacked = c.sandboxOptions.FullyPhysicallyBacked
}
c.nextGuestPort = state.GetGcsNextPort()
c.nextBridgeID = state.GetBridgeNextID()
c.compatInfo = state.GetCompatInfo()
Expand All @@ -155,6 +152,7 @@ func (c *Controller) Import(ctx context.Context, env *anypb.Any) error {
}

c.hcsDocument = doc
c.isPhysicallyBacked = isMemoryPhysicallyBacked(doc)
}

// Import the SCSI sub-controller.
Expand Down
16 changes: 7 additions & 9 deletions internal/controller/vm/vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -530,15 +530,6 @@ func (c *Controller) Stats(ctx context.Context) (*stats.VirtualMachineStatistics
return nil, fmt.Errorf("cannot get stats: VM is in incorrect state %s", c.vmState)
}

// Initialization of vmmemProcess to calculate stats properly for VA-backed UVMs.
if c.vmmemProcess == 0 {
vmmemHandle, err := vmutils.LookupVMMEM(ctx, c.uvm.RuntimeID(), &iwin.WinAPI{})
if err != nil {
return nil, fmt.Errorf("cannot get stats: %w", err)
}
c.vmmemProcess = vmmemHandle
}

s := &stats.VirtualMachineStatistics{}
props, err := c.uvm.PropertiesV2(ctx, hcsschema.PTStatistics, hcsschema.PTMemory)
if err != nil {
Expand All @@ -553,6 +544,13 @@ func (c *Controller) Stats(ctx context.Context) (*stats.VirtualMachineStatistics
// working set size for a VA-backed UVM. To work around this, we instead
// locate the vmmem process for the VM, and query that process's working set
// instead, which will be the working set for the VM.
if c.vmmemProcess == 0 {
vmmemHandle, err := vmutils.LookupVMMEM(ctx, c.uvm.RuntimeID(), &iwin.WinAPI{})
if err != nil {
return nil, fmt.Errorf("cannot get stats: %w", err)
}
c.vmmemProcess = vmmemHandle
}
memCounters, err := process.GetProcessMemoryInfo(c.vmmemProcess)
if err != nil {
return nil, err
Expand Down
10 changes: 9 additions & 1 deletion internal/controller/vm/vm_lcow.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,19 @@ func (c *Controller) buildHCSConfig(ctx context.Context, opts *CreateOptions) (*
}

c.sandboxOptions = sandboxOptions
c.isPhysicallyBacked = sandboxOptions.FullyPhysicallyBacked
c.isPhysicallyBacked = isMemoryPhysicallyBacked(hcsDocument)

return hcsDocument, nil
}

func isMemoryPhysicallyBacked(doc *hcsschema.ComputeSystem) bool {
return doc != nil &&
doc.VirtualMachine != nil &&
doc.VirtualMachine.ComputeTopology != nil &&
doc.VirtualMachine.ComputeTopology.Memory != nil &&
!doc.VirtualMachine.ComputeTopology.Memory.AllowOvercommit
}

// buildConfidentialOptions builds confidential options from the stored sandbox options.
func (c *Controller) buildConfidentialOptions(ctx context.Context) (*guestresource.ConfidentialOptions, error) {
if c.sandboxOptions == nil || c.sandboxOptions.ConfidentialConfig == nil {
Expand Down
53 changes: 53 additions & 0 deletions internal/controller/vm/vm_lcow_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
//go:build windows && lcow

package vm

import (
"testing"

hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
)

func TestIsMemoryPhysicallyBacked(t *testing.T) {
tests := []struct {
name string
doc *hcsschema.ComputeSystem
want bool
}{
{
name: "missing memory topology",
doc: &hcsschema.ComputeSystem{},
},
{
name: "overcommitted memory",
doc: computeSystemWithMemory(&hcsschema.VirtualMachineMemory{
AllowOvercommit: true,
}),
},
{
name: "non-overcommitted memory",
doc: computeSystemWithMemory(&hcsschema.VirtualMachineMemory{
AllowOvercommit: false,
}),
want: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
if got := isMemoryPhysicallyBacked(test.doc); got != test.want {
t.Fatalf("isMemoryPhysicallyBacked() = %t, want %t", got, test.want)
}
})
}
}

func computeSystemWithMemory(memory *hcsschema.VirtualMachineMemory) *hcsschema.ComputeSystem {
return &hcsschema.ComputeSystem{
VirtualMachine: &hcsschema.VirtualMachine{
ComputeTopology: &hcsschema.Topology{
Memory: memory,
},
},
}
}
Loading