From 85d6e6596c730e14c7b6409e07ed49b4a6e63272 Mon Sep 17 00:00:00 2001 From: Hamza El-Saawy Date: Tue, 1 Jul 2025 14:12:44 -0400 Subject: [PATCH] Warn on incomplete vNUMA setting, clarify field names Warn if vNUMA is not completely specified in uVM creation options, as this is likely a user error. Rename `"uvm".Opts.MaxSizePerNode` to `MaxMemorySizePerNumaNode` and clarify that it is measured in MiB. Similarly, rename `"annotations".NumaMaximumSizePerNode` to `NumaMaximumMemorySizePerNode`. Format `prepareVNumaTopology` doc comment to display appropriately. Related: switch to using `"logrus".IsLevelEnabled` rather than explicit logging level comparison, and fix bug where `--debug` flag was not added to runc if logging level is greater than `Debug` (i.e., `Trace`). Signed-off-by: Hamza El-Saawy --- cmd/runhcs/container.go | 2 +- internal/gcs/bridge.go | 2 +- internal/guest/bridge/bridge.go | 2 +- internal/guest/network/netns.go | 2 +- internal/oci/uvm.go | 5 ++- internal/uvm/create.go | 4 +- internal/uvm/create_lcow.go | 2 +- internal/uvm/create_wcow.go | 2 +- internal/uvm/vnuma.go | 66 ++++++++++++++++++++++++--------- pkg/annotations/annotations.go | 6 ++- 10 files changed, 65 insertions(+), 28 deletions(-) diff --git a/cmd/runhcs/container.go b/cmd/runhcs/container.go index 2a5f5b7669..0a8786368f 100644 --- a/cmd/runhcs/container.go +++ b/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") } } diff --git a/internal/gcs/bridge.go b/internal/gcs/bridge.go index c1e026778d..3f09b3d1ff 100644 --- a/internal/gcs/bridge.go +++ b/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 diff --git a/internal/guest/bridge/bridge.go b/internal/guest/bridge/bridge.go index 024c7108b9..4ea03ed104 100644 --- a/internal/guest/bridge/bridge.go +++ b/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 { diff --git a/internal/guest/network/netns.go b/internal/guest/network/netns.go index fde15911c6..e809a6ea04 100644 --- a/internal/guest/network/netns.go +++ b/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) diff --git a/internal/oci/uvm.go b/internal/oci/uvm.go index cf8de1227d..f0d9402e1d 100644 --- a/internal/oci/uvm.go +++ b/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)) } diff --git a/internal/uvm/create.go b/internal/uvm/create.go index 4c113ff251..c736133494 100644 --- a/internal/uvm/create.go +++ b/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. diff --git a/internal/uvm/create_lcow.go b/internal/uvm/create_lcow.go index 1bb9461a14..3b3c364734 100644 --- a/internal/uvm/create_lcow.go +++ b/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 } diff --git a/internal/uvm/create_wcow.go b/internal/uvm/create_wcow.go index 2b6f8c8b2d..b1dbfdd138 100644 --- a/internal/uvm/create_wcow.go +++ b/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 } diff --git a/internal/uvm/vnuma.go b/internal/uvm/vnuma.go index f944e8c8de..f5442edac9 100644 --- a/internal/uvm/vnuma.go +++ b/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) } diff --git a/pkg/annotations/annotations.go b/pkg/annotations/annotations.go index 86c088a1cc..31406fa1b9 100644 --- a/pkg/annotations/annotations.go +++ b/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.