Warn on incomplete vNUMA setting, clarify field names

cmd/runhcs/container.go

@@ -153,7 +153,7 @@ func launchShim(cmd, pidFile, logFile string, args []string, data interface{}) (
 		}
 
 		fullargs = append(fullargs, "--log-format", logFormat)
-		if logrus.GetLevel() == logrus.DebugLevel {
+		if logrus.IsLevelEnabled(logrus.DebugLevel) {
 			fullargs = append(fullargs, "--debug")
 		}
 	}

internal/gcs/bridge.go

@@ -391,7 +391,7 @@ func (brdg *bridge) writeMessage(buf *bytes.Buffer, enc *json.Encoder, typ prot.
 	// Update the message header with the size.
 	binary.LittleEndian.PutUint32(buf.Bytes()[prot.HdrOffSize:], uint32(buf.Len()))
 
-	if brdg.log.Logger.GetLevel() > logrus.DebugLevel {
+	if brdg.log.Logger.IsLevelEnabled(logrus.TraceLevel) {
 		b := buf.Bytes()[prot.HdrSize:]
 		switch typ {
 		// container environment vars are in rpCreate for linux; rpcExecuteProcess for windows

internal/guest/bridge/bridge.go

@@ -309,7 +309,7 @@ func (b *Bridge) ListenAndServe(bridgeIn io.ReadCloser, bridgeOut io.WriteCloser
 					trace.StringAttribute("cid", base.ContainerID))
 
 				entry := log.G(ctx)
-				if entry.Logger.GetLevel() > logrus.DebugLevel {
+				if entry.Logger.IsLevelEnabled(logrus.TraceLevel) {
 					var err error
 					var msgBytes []byte
 					switch header.Type {

internal/guest/network/netns.go

@@ -170,7 +170,7 @@ func NetNSConfig(ctx context.Context, ifStr string, nsPid int, adapter *guestres
 	}
 
 	// Add some debug logging
-	if entry.Logger.GetLevel() >= logrus.DebugLevel {
+	if entry.Logger.IsLevelEnabled(logrus.DebugLevel) {
 		curNS, _ := netns.Get()
 		// Refresh link attributes/state
 		link, _ = netlink.LinkByIndex(link.Attrs().Index)

internal/oci/uvm.go

@@ -268,8 +268,10 @@ func specToUVMCreateOptionsCommon(ctx context.Context, opts *uvm.Options, s *spe
 	opts.ProcessDumpLocation = ParseAnnotationsString(s.Annotations, annotations.ContainerProcessDumpLocation, opts.ProcessDumpLocation)
 	opts.NoWritableFileShares = ParseAnnotationsBool(ctx, s.Annotations, annotations.DisableWritableFileShares, opts.NoWritableFileShares)
 	opts.DumpDirectoryPath = ParseAnnotationsString(s.Annotations, annotations.DumpDirectoryPath, opts.DumpDirectoryPath)
+
+	// NUMA settings
 	opts.MaxProcessorsPerNumaNode = ParseAnnotationsUint32(ctx, s.Annotations, annotations.NumaMaximumProcessorsPerNode, opts.MaxProcessorsPerNumaNode)
-	opts.MaxSizePerNode = ParseAnnotationsUint64(ctx, s.Annotations, annotations.NumaMaximumSizePerNode, opts.MaxSizePerNode)
+	opts.MaxMemorySizePerNumaNode = ParseAnnotationsUint64(ctx, s.Annotations, annotations.NumaMaximumMemorySizePerNode, opts.MaxMemorySizePerNumaNode)
 	opts.PreferredPhysicalNumaNodes = ParseAnnotationCommaSeparatedUint32(ctx, s.Annotations, annotations.NumaPreferredPhysicalNodes,
 		opts.PreferredPhysicalNumaNodes)
 	opts.NumaMappedPhysicalNodes = ParseAnnotationCommaSeparatedUint32(ctx, s.Annotations, annotations.NumaMappedPhysicalNodes,
@@ -278,6 +280,7 @@ func specToUVMCreateOptionsCommon(ctx context.Context, opts *uvm.Options, s *spe
 		opts.NumaProcessorCounts)
 	opts.NumaMemoryBlocksCounts = ParseAnnotationCommaSeparatedUint64(ctx, s.Annotations, annotations.NumaCountOfMemoryBlocks,
 		opts.NumaMemoryBlocksCounts)
+
 	maps.Copy(opts.AdditionalHyperVConfig, parseHVSocketServiceTable(ctx, s.Annotations))
 }
 

internal/uvm/create.go

@@ -108,8 +108,8 @@ type Options struct {
 	AdditionalHyperVConfig map[string]hcsschema.HvSocketServiceConfig
 
 	// The following options are for implicit vNUMA topology settings.
-	// MaxSizePerNode is the maximum size of memory per vNUMA node.
-	MaxSizePerNode uint64
+	// MaxMemorySizePerNumaNode is the maximum size of memory (in MiB) per vNUMA node.
+	MaxMemorySizePerNumaNode uint64
 	// MaxProcessorsPerNumaNode is the maximum number of processors per vNUMA node.
 	MaxProcessorsPerNumaNode uint32
 	// PhysicalNumaNodes are the preferred physical NUMA nodes to map to vNUMA nodes.

internal/uvm/create_lcow.go

@@ -596,7 +596,7 @@ func makeLCOWDoc(ctx context.Context, opts *OptionsLCOW, uvm *UtilityVM) (_ *hcs
 		return nil, err
 	}
 
-	numa, numaProcessors, err := prepareVNumaTopology(opts.Options)
+	numa, numaProcessors, err := prepareVNumaTopology(ctx, opts.Options)
 	if err != nil {
 		return nil, err
 	}

internal/uvm/create_wcow.go

@@ -165,7 +165,7 @@ func prepareCommonConfigDoc(ctx context.Context, uvm *UtilityVM, opts *OptionsWC
 		Weight: uint64(opts.ProcessorWeight),
 	}
 
-	numa, numaProcessors, err := prepareVNumaTopology(opts.Options)
+	numa, numaProcessors, err := prepareVNumaTopology(ctx, opts.Options)
 	if err != nil {
 		return nil, err
 	}

internal/uvm/vnuma.go

@@ -3,45 +3,68 @@
 package uvm
 
 import (
+	"context"
 	"fmt"
 
+	"github.com/sirupsen/logrus"
+
 	hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
+	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/osversion"
 )
 
 // prepareVNumaTopology creates vNUMA settings for implicit (platform) or explicit (user-defined) topology.
 //
-// For implicit topology we look for `MaxProcessorsPerNumaNode`, `MaxSizePerNode` and `preferredNumaNodes` create options. Setting them
-// in HCS doc, will trigger platform to create vNUMA topology based on the given values. Based on experiments, the
-// platform will create an evenly distributed topology based on requested memory and processor count for the HCS VM.
+// For implicit topology we look for `MaxProcessorsPerNumaNode`, `MaxMemorySizePerNumaNode` and
+// `PreferredPhysicalNumaNodes` create options.
+// Setting them in HCS doc will trigger platform to create vNUMA topology based on the given values.
+// Based on experiments, the platform will create an evenly distributed topology based on
+// requested memory and processor count for the HCS VM.
 //
-// For explicit topology we look for `NumaMappedPhysicalNodes`, `NumaProcessorCounts` and `NumaMemoryBlocksCounts` create
-// options. The above options are number slices, where a value at index `i` in each slice represents the corresponding
+// For explicit topology we look for `NumaMappedPhysicalNodes`, `NumaProcessorCounts` and
+// `NumaMemoryBlocksCounts` create options.
+// The above options are number slices, where a value at index `i` in each slice represents the corresponding
 // value for the `i`th vNUMA node.
+//
 // Limitations:
-// - only hcsschema.MemoryBackingType_PHYSICAL is supported
-// - `PhysicalNumaNodes` values at index `i` will be mapped to virtual node number `i`
-// - client is responsible for setting wildcard physical node numbers
-// TODO: Add exact OS build version for vNUMA support.
-func prepareVNumaTopology(opts *Options) (*hcsschema.Numa, *hcsschema.NumaProcessors, error) {
+//
+//   - only `hcsschema.MemoryBackingType_PHYSICAL` is supported
+//   - `PhysicalNumaNodes` values at index `i` will be mapped to virtual node number `i`
+//   - client is responsible for setting wildcard physical node numbers
+func prepareVNumaTopology(ctx context.Context, opts *Options) (*hcsschema.Numa, *hcsschema.NumaProcessors, error) {
 	if opts.MaxProcessorsPerNumaNode == 0 && len(opts.NumaMappedPhysicalNodes) == 0 {
+		// warn if vNUMA settings are partially specified, since its likely an error on the client's side
+		if opts.MaxMemorySizePerNumaNode > 0 || len(opts.PreferredPhysicalNumaNodes) > 0 {
+			log.G(ctx).WithFields(logrus.Fields{
+				"max-memory-size-per-numa-node": opts.MaxMemorySizePerNumaNode,
+				"max-processors-per-numa-node":  opts.MaxProcessorsPerNumaNode,
+				"preferred-physical-numa-nodes": log.Format(ctx, opts.PreferredPhysicalNumaNodes),
+			}).Warn("potentially incomplete implicit vNUMA topology")
+		}
+		if len(opts.NumaProcessorCounts) > 0 || len(opts.NumaMemoryBlocksCounts) > 0 {
+			log.G(ctx).WithFields(logrus.Fields{
+				"numa-mapped-physical-nodes": log.Format(ctx, opts.NumaMappedPhysicalNodes),
+				"numa-processor-counts":      log.Format(ctx, opts.NumaProcessorCounts),
+				"numa-memory-block-counts":   log.Format(ctx, opts.NumaMemoryBlocksCounts),
+			}).Warn("potentially incomplete explicit vNUMA topology")
+		}
 		// vNUMA settings are missing, return empty topology
 		return nil, nil, nil
 	}
 
+	// TODO: Add exact OS build version for vNUMA support, or check for dedicated NUMA feature.
+	if build := osversion.Build(); build < osversion.V25H1Server {
+		return nil, nil, fmt.Errorf("vNUMA topology is not supported on %d version of Windows", build)
+	}
+
 	var preferredNumaNodes []int64
 	for _, pn := range opts.PreferredPhysicalNumaNodes {
 		preferredNumaNodes = append(preferredNumaNodes, int64(pn))
 	}
 
-	build := osversion.Get().Build
-	if build < osversion.V25H1Server {
-		return nil, nil, fmt.Errorf("vNUMA topology is not supported on %d version of Windows", build)
-	}
-
 	// Implicit vNUMA topology.
 	if opts.MaxProcessorsPerNumaNode > 0 {
-		if opts.MaxSizePerNode == 0 {
+		if opts.MaxMemorySizePerNumaNode == 0 {
 			return nil, nil, fmt.Errorf("max size per node must be set when max processors per numa node is set")
 		}
 		numaProcessors := &hcsschema.NumaProcessors{
@@ -50,9 +73,15 @@ func prepareVNumaTopology(opts *Options) (*hcsschema.Numa, *hcsschema.NumaProces
 			},
 		}
 		numa := &hcsschema.Numa{
-			MaxSizePerNode:         opts.MaxSizePerNode,
+			MaxSizePerNode:         opts.MaxMemorySizePerNumaNode,
 			PreferredPhysicalNodes: preferredNumaNodes,
 		}
+		if entry := log.G(ctx); entry.Logger.IsLevelEnabled(logrus.DebugLevel) {
+			entry.WithFields(logrus.Fields{
+				"numa":            log.Format(ctx, numa),
+				"numa-processors": log.Format(ctx, numaProcessors),
+			}).Debug("created implicit NUMA topology")
+		}
 		return numa, numaProcessors, nil
 	}
 
@@ -79,6 +108,9 @@ func prepareVNumaTopology(opts *Options) (*hcsschema.Numa, *hcsschema.NumaProces
 		}
 		numa.Settings = append(numa.Settings, nodeTopology)
 	}
+	if entry := log.G(ctx); entry.Logger.IsLevelEnabled(logrus.DebugLevel) {
+		entry.WithField("numa", log.Format(ctx, numa)).Debug("created explicit NUMA topology")
+	}
 	return numa, nil, validate(numa)
 }
 

pkg/annotations/annotations.go

@@ -286,9 +286,11 @@ const (
 	// This should be used for implicit vNUMA topology.
 	NumaMaximumProcessorsPerNode = "io.microsoft.virtualmachine.computetopology.processor.numa.max-processors-per-node"
 
-	// NumaMaximumSizePerNode is the maximum size per vNUMA node.
+	// NumaMaximumMemorySizePerNode is the maximum memory size (in MB) per vNUMA node.
 	// This should be used for implicit vNUMA topology.
-	NumaMaximumSizePerNode = "io.microsoft.virtualmachine.computetopology.processor.numa.max-size-per-node"
+	NumaMaximumMemorySizePerNode = "io.microsoft.virtualmachine.computetopology.processor.numa.max-size-per-node"
+	// Deprecated: Use [NumaMaximumMemorySizePerNode] instead.
+	NumaMaximumSizePerNode = NumaMaximumMemorySizePerNode
 
 	// NumaPreferredPhysicalNodes is an integer slice representing the preferred physical NUMA nodes.
 	// This should be used for implicit vNUMA topology.