From cd895b4d114093e858d48139ed8dc214727cac7d Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Thu, 27 May 2021 11:55:51 -0700 Subject: [PATCH] Remove internal GCS connection functionality HCS maintains an internal guest connection to the GCS normally if you request it. However, there are certain features that require us to maintain an external connection (external in this sense meaning not in HCS) instead like late cloning. We had swapped to always managing the connection to the GCS ourselves some time ago and afaik there's been no fallout from it, so I propose let's get rid of the internal branches altogether. This greatly simplifies the work for going through a different virtstack for hypervisor isolated containers as well. Ran go mod vendor in /test to bring in the changes as well. Signed-off-by: Daniel Canter --- internal/oci/uvm.go | 6 ----- internal/tools/uvmboot/main.go | 8 ------- internal/uvm/create.go | 22 ++++++------------- internal/uvm/create_lcow.go | 9 +------- internal/uvm/create_wcow.go | 11 ++-------- test/cri-containerd/runpodsandbox_test.go | 15 ------------- .../hcsshim/internal/cpugroup/cpugroup.go | 5 ++--- .../internal/hcs/schema2/cpu_group_config.go | 2 +- .../Microsoft/hcsshim/internal/oci/uvm.go | 6 ----- .../Microsoft/hcsshim/internal/uvm/create.go | 22 ++++++------------- .../hcsshim/internal/uvm/create_lcow.go | 9 +------- .../hcsshim/internal/uvm/create_wcow.go | 11 ++-------- 12 files changed, 23 insertions(+), 103 deletions(-) diff --git a/internal/oci/uvm.go b/internal/oci/uvm.go index 00a10e7081..db3f429253 100644 --- a/internal/oci/uvm.go +++ b/internal/oci/uvm.go @@ -139,10 +139,6 @@ const ( annotationFullyPhysicallyBacked = "io.microsoft.virtualmachine.fullyphysicallybacked" annotationDisableCompartmentNamespace = "io.microsoft.virtualmachine.disablecompartmentnamespace" annotationVSMBNoDirectMap = "io.microsoft.virtualmachine.wcow.virtualSMB.nodirectmap" - // A boolean annotation to control whether to use an external bridge or the - // HCS-GCS bridge. Default value is true which means external bridge will be used - // by default. - annotationUseExternalGCSBridge = "io.microsoft.virtualmachine.useexternalgcsbridge" // annotation used to specify the cpugroup ID that a UVM should be assigned to annotationCPUGroupID = "io.microsoft.virtualmachine.cpugroup.id" @@ -464,7 +460,6 @@ func SpecToUVMCreateOpts(ctx context.Context, s *specs.Spec, id, owner string) ( lopts.StorageQoSIopsMaximum = ParseAnnotationsStorageIops(ctx, s, annotationStorageQoSIopsMaximum, lopts.StorageQoSIopsMaximum) lopts.VPCIEnabled = parseAnnotationsBool(ctx, s.Annotations, annotationVPCIEnabled, lopts.VPCIEnabled) lopts.BootFilesPath = parseAnnotationsString(s.Annotations, annotationBootFilesRootPath, lopts.BootFilesPath) - lopts.ExternalGuestConnection = parseAnnotationsBool(ctx, s.Annotations, annotationUseExternalGCSBridge, lopts.ExternalGuestConnection) lopts.CPUGroupID = parseAnnotationsString(s.Annotations, annotationCPUGroupID, lopts.CPUGroupID) lopts.NetworkConfigProxy = parseAnnotationsString(s.Annotations, annotationNetworkConfigProxy, lopts.NetworkConfigProxy) handleAnnotationPreferredRootFSType(ctx, s.Annotations, lopts) @@ -487,7 +482,6 @@ func SpecToUVMCreateOpts(ctx context.Context, s *specs.Spec, id, owner string) ( wopts.ProcessorWeight = ParseAnnotationsCPUWeight(ctx, s, annotationProcessorWeight, wopts.ProcessorWeight) wopts.StorageQoSBandwidthMaximum = ParseAnnotationsStorageBps(ctx, s, annotationStorageQoSBandwidthMaximum, wopts.StorageQoSBandwidthMaximum) wopts.StorageQoSIopsMaximum = ParseAnnotationsStorageIops(ctx, s, annotationStorageQoSIopsMaximum, wopts.StorageQoSIopsMaximum) - wopts.ExternalGuestConnection = parseAnnotationsBool(ctx, s.Annotations, annotationUseExternalGCSBridge, wopts.ExternalGuestConnection) wopts.DisableCompartmentNamespace = parseAnnotationsBool(ctx, s.Annotations, annotationDisableCompartmentNamespace, wopts.DisableCompartmentNamespace) wopts.CPUGroupID = parseAnnotationsString(s.Annotations, annotationCPUGroupID, wopts.CPUGroupID) wopts.NetworkConfigProxy = parseAnnotationsString(s.Annotations, annotationNetworkConfigProxy, wopts.NetworkConfigProxy) diff --git a/internal/tools/uvmboot/main.go b/internal/tools/uvmboot/main.go index 09973cdf61..8f0a1b6293 100644 --- a/internal/tools/uvmboot/main.go +++ b/internal/tools/uvmboot/main.go @@ -21,7 +21,6 @@ const ( countArgName = "count" debugArgName = "debug" gcsArgName = "gcs" - externalBridgeArgName = "external-bridge" execCommandLineArgName = "exec" ) @@ -70,10 +69,6 @@ func main() { Name: gcsArgName, Usage: "Launch the GCS and perform requested operations via its RPC interface", }, - cli.BoolFlag{ - Name: externalBridgeArgName, - Usage: "Use the external implementation of the guest connection", - }, } app.Commands = []cli.Command{ @@ -109,9 +104,6 @@ func setGlobalOptions(c *cli.Context, options *uvm.Options) { if c.GlobalIsSet(enableDeferredCommitArgName) { options.EnableDeferredCommit = c.GlobalBool(enableDeferredCommitArgName) } - if c.GlobalIsSet(externalBridgeArgName) { - options.ExternalGuestConnection = c.GlobalBool(externalBridgeArgName) - } } func runMany(c *cli.Context, runFunc func(id string) error) { diff --git a/internal/uvm/create.go b/internal/uvm/create.go index 2ec528f0d3..6f7f844462 100644 --- a/internal/uvm/create.go +++ b/internal/uvm/create.go @@ -66,10 +66,6 @@ type Options struct { // will default to the platform default. StorageQoSBandwidthMaximum int32 - // ExternalGuestConnection sets whether the guest RPC connection is performed - // internally by the OS platform or externally by this package. - ExternalGuestConnection bool - // DisableCompartmentNamespace sets whether to disable namespacing the network compartment in the UVM // for WCOW. Namespacing makes it so the compartment created for a container is essentially no longer // aware or able to see any of the other compartments on the host (in this case the UVM). @@ -161,9 +157,6 @@ func verifyOptions(ctx context.Context, options interface{}) error { if opts.IsClone && opts.TemplateConfig == nil { return errors.New("template config can not be nil when creating clone") } - if opts.IsClone && !opts.ExternalGuestConnection { - return errors.New("External gcs connection can not be disabled for clones") - } if opts.IsTemplate && opts.FullyPhysicallyBacked { return errors.New("Template can not be created from a full physically backed UVM") } @@ -178,14 +171,13 @@ func verifyOptions(ctx context.Context, options interface{}) error { // If `owner` is empty it will be set to the calling executables name. func newDefaultOptions(id, owner string) *Options { opts := &Options{ - ID: id, - Owner: owner, - MemorySizeInMB: 1024, - AllowOvercommit: true, - EnableDeferredCommit: false, - ProcessorCount: defaultProcessorCount(), - ExternalGuestConnection: true, - FullyPhysicallyBacked: false, + ID: id, + Owner: owner, + MemorySizeInMB: 1024, + AllowOvercommit: true, + EnableDeferredCommit: false, + ProcessorCount: defaultProcessorCount(), + FullyPhysicallyBacked: false, } if opts.Owner == "" { diff --git a/internal/uvm/create_lcow.go b/internal/uvm/create_lcow.go index da26278c1a..92c7e8d478 100644 --- a/internal/uvm/create_lcow.go +++ b/internal/uvm/create_lcow.go @@ -256,13 +256,6 @@ func CreateLCOW(ctx context.Context, opts *OptionsLCOW) (_ *UtilityVM, err error } } - if opts.UseGuestConnection && !opts.ExternalGuestConnection { - doc.VirtualMachine.GuestConnection = &hcsschema.GuestConnection{ - UseVsock: true, - UseConnectedSuspend: true, - } - } - if uvm.scsiControllerCount > 0 { // TODO: JTERRY75 - this should enumerate scsicount and add an entry per value. doc.VirtualMachine.Devices.Scsi = map[string]hcsschema.Scsi{ @@ -406,7 +399,7 @@ func CreateLCOW(ctx context.Context, opts *OptionsLCOW) (_ *UtilityVM, err error } } - if opts.UseGuestConnection && opts.ExternalGuestConnection { + if opts.UseGuestConnection { log.G(ctx).WithField("vmID", uvm.runtimeID).Debug("Using external GCS bridge") l, err := uvm.listenVsock(gcs.LinuxGcsVsockPort) if err != nil { diff --git a/internal/uvm/create_wcow.go b/internal/uvm/create_wcow.go index 04eb1dd9d1..439b290ee7 100644 --- a/internal/uvm/create_wcow.go +++ b/internal/uvm/create_wcow.go @@ -180,10 +180,6 @@ func prepareConfigDoc(ctx context.Context, uvm *UtilityVM, opts *OptionsWCOW, uv }, } - if !opts.ExternalGuestConnection { - doc.VirtualMachine.GuestConnection = &hcsschema.GuestConnection{} - } - // Handle StorageQoS if set if opts.StorageQoSBandwidthMaximum > 0 || opts.StorageQoSIopsMaximum > 0 { doc.VirtualMachine.StorageQoS = &hcsschema.StorageQoS{ @@ -332,11 +328,8 @@ func CreateWCOW(ctx context.Context, opts *OptionsWCOW) (_ *UtilityVM, err error return nil, fmt.Errorf("error while creating the compute system: %s", err) } - // All clones MUST use external gcs connection - if opts.ExternalGuestConnection { - if err = uvm.startExternalGcsListener(ctx); err != nil { - return nil, err - } + if err = uvm.startExternalGcsListener(ctx); err != nil { + return nil, err } // If network config proxy address passed in, construct a client. diff --git a/test/cri-containerd/runpodsandbox_test.go b/test/cri-containerd/runpodsandbox_test.go index f2c441bc70..9ab32ce711 100644 --- a/test/cri-containerd/runpodsandbox_test.go +++ b/test/cri-containerd/runpodsandbox_test.go @@ -51,21 +51,6 @@ func Test_RunPodSandbox_WCOW_Hypervisor(t *testing.T) { runPodSandboxTest(t, request) } -func Test_RunPodSandbox_WCOW_Hypervisor_WithoutExternalBridge(t *testing.T) { - requireFeatures(t, featureWCOWHypervisor) - - pullRequiredImages(t, []string{imageWindowsNanoserver}) - - request := getRunPodSandboxRequest( - t, - wcowHypervisorRuntimeHandler, - map[string]string{ - "io.microsoft.virtualmachine.useexternalgcsbridge": "false", - }, - ) - runPodSandboxTest(t, request) -} - func Test_RunPodSandbox_LCOW(t *testing.T) { requireFeatures(t, featureLCOW) diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/cpugroup/cpugroup.go b/test/vendor/github.com/Microsoft/hcsshim/internal/cpugroup/cpugroup.go index ab3b757efb..61cd703586 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/cpugroup/cpugroup.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/cpugroup/cpugroup.go @@ -53,9 +53,8 @@ func Create(ctx context.Context, id string, logicalProcessors []uint32) error { return nil } -// getCPUGroupConfig finds the cpugroup config information for group with `id` -//nolint:unused -func getCPUGroupConfig(ctx context.Context, id string) (*hcsschema.CpuGroupConfig, error) { +// GetCPUGroupConfig finds the cpugroup config information for group with `id` +func GetCPUGroupConfig(ctx context.Context, id string) (*hcsschema.CpuGroupConfig, error) { query := hcsschema.PropertyQuery{ PropertyTypes: []hcsschema.PropertyType{hcsschema.PTCPUGroup}, } diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/hcs/schema2/cpu_group_config.go b/test/vendor/github.com/Microsoft/hcsshim/internal/hcs/schema2/cpu_group_config.go index f1a28cd389..0be0475d41 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/hcs/schema2/cpu_group_config.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/hcs/schema2/cpu_group_config.go @@ -14,5 +14,5 @@ type CpuGroupConfig struct { Affinity *CpuGroupAffinity `json:"Affinity,omitempty"` GroupProperties []CpuGroupProperty `json:"GroupProperties,omitempty"` // Hypervisor CPU group IDs exposed to clients - HypervisorGroupId int32 `json:"HypervisorGroupId,omitempty"` + HypervisorGroupId uint64 `json:"HypervisorGroupId,omitempty"` } diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/oci/uvm.go b/test/vendor/github.com/Microsoft/hcsshim/internal/oci/uvm.go index 00a10e7081..db3f429253 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/oci/uvm.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/oci/uvm.go @@ -139,10 +139,6 @@ const ( annotationFullyPhysicallyBacked = "io.microsoft.virtualmachine.fullyphysicallybacked" annotationDisableCompartmentNamespace = "io.microsoft.virtualmachine.disablecompartmentnamespace" annotationVSMBNoDirectMap = "io.microsoft.virtualmachine.wcow.virtualSMB.nodirectmap" - // A boolean annotation to control whether to use an external bridge or the - // HCS-GCS bridge. Default value is true which means external bridge will be used - // by default. - annotationUseExternalGCSBridge = "io.microsoft.virtualmachine.useexternalgcsbridge" // annotation used to specify the cpugroup ID that a UVM should be assigned to annotationCPUGroupID = "io.microsoft.virtualmachine.cpugroup.id" @@ -464,7 +460,6 @@ func SpecToUVMCreateOpts(ctx context.Context, s *specs.Spec, id, owner string) ( lopts.StorageQoSIopsMaximum = ParseAnnotationsStorageIops(ctx, s, annotationStorageQoSIopsMaximum, lopts.StorageQoSIopsMaximum) lopts.VPCIEnabled = parseAnnotationsBool(ctx, s.Annotations, annotationVPCIEnabled, lopts.VPCIEnabled) lopts.BootFilesPath = parseAnnotationsString(s.Annotations, annotationBootFilesRootPath, lopts.BootFilesPath) - lopts.ExternalGuestConnection = parseAnnotationsBool(ctx, s.Annotations, annotationUseExternalGCSBridge, lopts.ExternalGuestConnection) lopts.CPUGroupID = parseAnnotationsString(s.Annotations, annotationCPUGroupID, lopts.CPUGroupID) lopts.NetworkConfigProxy = parseAnnotationsString(s.Annotations, annotationNetworkConfigProxy, lopts.NetworkConfigProxy) handleAnnotationPreferredRootFSType(ctx, s.Annotations, lopts) @@ -487,7 +482,6 @@ func SpecToUVMCreateOpts(ctx context.Context, s *specs.Spec, id, owner string) ( wopts.ProcessorWeight = ParseAnnotationsCPUWeight(ctx, s, annotationProcessorWeight, wopts.ProcessorWeight) wopts.StorageQoSBandwidthMaximum = ParseAnnotationsStorageBps(ctx, s, annotationStorageQoSBandwidthMaximum, wopts.StorageQoSBandwidthMaximum) wopts.StorageQoSIopsMaximum = ParseAnnotationsStorageIops(ctx, s, annotationStorageQoSIopsMaximum, wopts.StorageQoSIopsMaximum) - wopts.ExternalGuestConnection = parseAnnotationsBool(ctx, s.Annotations, annotationUseExternalGCSBridge, wopts.ExternalGuestConnection) wopts.DisableCompartmentNamespace = parseAnnotationsBool(ctx, s.Annotations, annotationDisableCompartmentNamespace, wopts.DisableCompartmentNamespace) wopts.CPUGroupID = parseAnnotationsString(s.Annotations, annotationCPUGroupID, wopts.CPUGroupID) wopts.NetworkConfigProxy = parseAnnotationsString(s.Annotations, annotationNetworkConfigProxy, wopts.NetworkConfigProxy) diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create.go b/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create.go index 2ec528f0d3..6f7f844462 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create.go @@ -66,10 +66,6 @@ type Options struct { // will default to the platform default. StorageQoSBandwidthMaximum int32 - // ExternalGuestConnection sets whether the guest RPC connection is performed - // internally by the OS platform or externally by this package. - ExternalGuestConnection bool - // DisableCompartmentNamespace sets whether to disable namespacing the network compartment in the UVM // for WCOW. Namespacing makes it so the compartment created for a container is essentially no longer // aware or able to see any of the other compartments on the host (in this case the UVM). @@ -161,9 +157,6 @@ func verifyOptions(ctx context.Context, options interface{}) error { if opts.IsClone && opts.TemplateConfig == nil { return errors.New("template config can not be nil when creating clone") } - if opts.IsClone && !opts.ExternalGuestConnection { - return errors.New("External gcs connection can not be disabled for clones") - } if opts.IsTemplate && opts.FullyPhysicallyBacked { return errors.New("Template can not be created from a full physically backed UVM") } @@ -178,14 +171,13 @@ func verifyOptions(ctx context.Context, options interface{}) error { // If `owner` is empty it will be set to the calling executables name. func newDefaultOptions(id, owner string) *Options { opts := &Options{ - ID: id, - Owner: owner, - MemorySizeInMB: 1024, - AllowOvercommit: true, - EnableDeferredCommit: false, - ProcessorCount: defaultProcessorCount(), - ExternalGuestConnection: true, - FullyPhysicallyBacked: false, + ID: id, + Owner: owner, + MemorySizeInMB: 1024, + AllowOvercommit: true, + EnableDeferredCommit: false, + ProcessorCount: defaultProcessorCount(), + FullyPhysicallyBacked: false, } if opts.Owner == "" { diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create_lcow.go b/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create_lcow.go index da26278c1a..92c7e8d478 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create_lcow.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create_lcow.go @@ -256,13 +256,6 @@ func CreateLCOW(ctx context.Context, opts *OptionsLCOW) (_ *UtilityVM, err error } } - if opts.UseGuestConnection && !opts.ExternalGuestConnection { - doc.VirtualMachine.GuestConnection = &hcsschema.GuestConnection{ - UseVsock: true, - UseConnectedSuspend: true, - } - } - if uvm.scsiControllerCount > 0 { // TODO: JTERRY75 - this should enumerate scsicount and add an entry per value. doc.VirtualMachine.Devices.Scsi = map[string]hcsschema.Scsi{ @@ -406,7 +399,7 @@ func CreateLCOW(ctx context.Context, opts *OptionsLCOW) (_ *UtilityVM, err error } } - if opts.UseGuestConnection && opts.ExternalGuestConnection { + if opts.UseGuestConnection { log.G(ctx).WithField("vmID", uvm.runtimeID).Debug("Using external GCS bridge") l, err := uvm.listenVsock(gcs.LinuxGcsVsockPort) if err != nil { diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create_wcow.go b/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create_wcow.go index 04eb1dd9d1..439b290ee7 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create_wcow.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/create_wcow.go @@ -180,10 +180,6 @@ func prepareConfigDoc(ctx context.Context, uvm *UtilityVM, opts *OptionsWCOW, uv }, } - if !opts.ExternalGuestConnection { - doc.VirtualMachine.GuestConnection = &hcsschema.GuestConnection{} - } - // Handle StorageQoS if set if opts.StorageQoSBandwidthMaximum > 0 || opts.StorageQoSIopsMaximum > 0 { doc.VirtualMachine.StorageQoS = &hcsschema.StorageQoS{ @@ -332,11 +328,8 @@ func CreateWCOW(ctx context.Context, opts *OptionsWCOW) (_ *UtilityVM, err error return nil, fmt.Errorf("error while creating the compute system: %s", err) } - // All clones MUST use external gcs connection - if opts.ExternalGuestConnection { - if err = uvm.startExternalGcsListener(ctx); err != nil { - return nil, err - } + if err = uvm.startExternalGcsListener(ctx); err != nil { + return nil, err } // If network config proxy address passed in, construct a client.