Skip to content

Commit

Permalink
GCS tests and features/bugfixes to support them
Browse files Browse the repository at this point in the history
Exposed data and functionality for testing GCS:
* `internal\guest\runtime\hcsv2.Container.InitProcess()`
* `internal\guest\runtime\hcsv2.GetOrAddNetworkNamespace()`
* `internal\guest\runtime\hcsv2.RemoveNetworkNamespace()`
* `internal\guest\runtime\hcsv2.Host.SecurityPolicyEnforcer()`
* `internal\guest\runtime\hcsv2.Host.Transport()`

Fixed bug where `host.RemoveContainer` did not remove the network
namespace for standalone and pod containers.

Updated go-winio version to include bugfixes for closing hvsockets,
specifically to close a socket for writing (needed by internal\cmd
to signal that the stdin stream has finished).

Added doc.go files to guest packages to prevent linter/compiler errors
under windows.

The tests themselves are broken out here:
#1352
#1351

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
  • Loading branch information
helsaawy committed May 3, 2022
1 parent dbb347e commit 132cb11
Show file tree
Hide file tree
Showing 50 changed files with 1,213 additions and 858 deletions.
7 changes: 6 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ service/pkg/
*.img
*.vhd
*.tar.gz
*.tar

# Make stuff
.rootfs-done
Expand All @@ -36,5 +37,9 @@ rootfs-conv/*
deps/*
out/*

# test results
test/results

# go workspace files
go.work
go.work.sum
go.work.sum
12 changes: 0 additions & 12 deletions functional_tests.ps1

This file was deleted.

2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ go 1.17

require (
github.com/BurntSushi/toml v0.3.1
github.com/Microsoft/go-winio v0.4.17
github.com/Microsoft/go-winio v0.5.2
github.com/cenkalti/backoff/v4 v4.1.1
github.com/containerd/cgroups v1.0.1
github.com/containerd/console v1.0.2
Expand Down
3 changes: 2 additions & 1 deletion go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ github.com/Microsoft/go-winio v0.4.16-0.20201130162521-d1ffc52c7331/go.mod h1:XB
github.com/Microsoft/go-winio v0.4.16/go.mod h1:XB6nPKklQyQ7GC9LdcBEcBl8PF76WugXOPRXwdLnMv0=
github.com/Microsoft/go-winio v0.4.17-0.20210211115548-6eac466e5fa3/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84=
github.com/Microsoft/go-winio v0.4.17-0.20210324224401-5516f17a5958/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84=
github.com/Microsoft/go-winio v0.4.17 h1:iT12IBVClFevaf8PuVyi3UmZOVh4OqnaLxDTW2O6j3w=
github.com/Microsoft/go-winio v0.4.17/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84=
github.com/Microsoft/go-winio v0.5.2 h1:a9IhgEQBCUEk6QCdml9CiJGhAws+YwffDHEMp1VMrpA=
github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY=
github.com/Microsoft/hcsshim v0.8.6/go.mod h1:Op3hHsoHPAvb6lceZHDtd9OkTew38wNoXnJs8iY7rUg=
github.com/Microsoft/hcsshim v0.8.7-0.20190325164909-8abdbb8205e4/go.mod h1:Op3hHsoHPAvb6lceZHDtd9OkTew38wNoXnJs8iY7rUg=
github.com/Microsoft/hcsshim v0.8.7/go.mod h1:OHd7sQqRFrYd3RmSgbgji+ctCwkbq2wbEYNSzOYtcBQ=
Expand Down
2 changes: 2 additions & 0 deletions internal/guest/linux/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Package linux contains definitions required for making a linux ioctl.
package linux
1 change: 0 additions & 1 deletion internal/guest/linux/ioctl.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//go:build linux
// +build linux

// Package linux contains definitions required for making a linux ioctl.
package linux

import (
Expand Down
1 change: 1 addition & 0 deletions internal/guest/policy/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package policy
11 changes: 10 additions & 1 deletion internal/guest/runtime/hcsv2/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,19 @@ func (c *Container) ExecProcess(ctx context.Context, process *oci.Process, conSe
return pid, nil
}

// InitProcess returns the container's init process
func (c *Container) InitProcess() Process {
return c.initProcess
}

// GetProcess returns the Process with the matching 'pid'. If the 'pid' does
// not exit returns error.
func (c *Container) GetProcess(pid uint32) (Process, error) {
//todo: thread a context to this function call
logrus.WithFields(logrus.Fields{
logfields.ContainerID: c.id,
logfields.ProcessID: pid,
}).Info("opengcs::Container::GetProcesss")
}).Info("opengcs::Container::GetProcess")
if c.initProcess.pid == pid {
return c.initProcess, nil
}
Expand Down Expand Up @@ -245,3 +250,7 @@ func (c *Container) getStatus() containerStatus {
func (c *Container) setStatus(st containerStatus) {
atomic.StoreUint32((*uint32)(&c.status), uint32(st))
}

func (c *Container) ID() string {
return c.id
}
10 changes: 5 additions & 5 deletions internal/guest/runtime/hcsv2/network.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ func getNetworkNamespace(id string) (*namespace, error) {
return ns, nil
}

// getOrAddNetworkNamespace returns the namespace found by `id` or creates a new
// GetOrAddNetworkNamespace returns the namespace found by `id` or creates a new
// one and assigns `id.
func getOrAddNetworkNamespace(id string) *namespace {
func GetOrAddNetworkNamespace(id string) *namespace {
id = strings.ToLower(id)

namespaceSync.Lock()
Expand All @@ -69,8 +69,8 @@ func getOrAddNetworkNamespace(id string) *namespace {
return ns
}

// removeNetworkNamespace removes the in-memory `namespace` found by `id`.
func removeNetworkNamespace(ctx context.Context, id string) (err error) {
// RemoveNetworkNamespace removes the in-memory `namespace` found by `id`.
func RemoveNetworkNamespace(ctx context.Context, id string) (err error) {
_, span := trace.StartSpan(ctx, "hcsv2::removeNetworkNamespace")
defer span.End()
defer func() { oc.SetSpanStatus(span, err) }()
Expand Down Expand Up @@ -123,7 +123,7 @@ func (n *namespace) AssignContainerPid(ctx context.Context, pid int) (err error)
defer n.m.Unlock()

if n.pid != 0 {
return errors.Errorf("previously assigned container pid: %d", n.pid)
return errors.Errorf("previously assigned container pid %d to network namespace %q", n.pid, n.id)
}

n.pid = pid
Expand Down
26 changes: 13 additions & 13 deletions internal/guest/runtime/hcsv2/network_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

func Test_getNetworkNamespace_NotExist(t *testing.T) {
defer func() {
err := removeNetworkNamespace(context.Background(), t.Name())
err := RemoveNetworkNamespace(context.Background(), t.Name())
if err != nil {
t.Errorf("failed to remove ns with error: %v", err)
}
Expand All @@ -29,13 +29,13 @@ func Test_getNetworkNamespace_NotExist(t *testing.T) {

func Test_getNetworkNamespace_PreviousExist(t *testing.T) {
defer func() {
err := removeNetworkNamespace(context.Background(), t.Name())
err := RemoveNetworkNamespace(context.Background(), t.Name())
if err != nil {
t.Errorf("failed to remove ns with error: %v", err)
}
}()

ns1 := getOrAddNetworkNamespace(t.Name())
ns1 := GetOrAddNetworkNamespace(t.Name())
if ns1 == nil {
t.Fatal("namespace ns1 should not be nil")
}
Expand All @@ -50,43 +50,43 @@ func Test_getNetworkNamespace_PreviousExist(t *testing.T) {

func Test_getOrAddNetworkNamespace_NotExist(t *testing.T) {
defer func() {
err := removeNetworkNamespace(context.Background(), t.Name())
err := RemoveNetworkNamespace(context.Background(), t.Name())
if err != nil {
t.Errorf("failed to remove ns with error: %v", err)
}
}()

ns := getOrAddNetworkNamespace(t.Name())
ns := GetOrAddNetworkNamespace(t.Name())
if ns == nil {
t.Fatalf("namespace should not be nil")
}
}

func Test_getOrAddNetworkNamespace_PreviousExist(t *testing.T) {
defer func() {
err := removeNetworkNamespace(context.Background(), t.Name())
err := RemoveNetworkNamespace(context.Background(), t.Name())
if err != nil {
t.Errorf("failed to remove ns with error: %v", err)
}
}()

ns1 := getOrAddNetworkNamespace(t.Name())
ns2 := getOrAddNetworkNamespace(t.Name())
ns1 := GetOrAddNetworkNamespace(t.Name())
ns2 := GetOrAddNetworkNamespace(t.Name())
if ns1 != ns2 {
t.Fatalf("ns1 %+v != ns2 %+v", ns1, ns2)
}
}

func Test_removeNetworkNamespace_NotExist(t *testing.T) {
err := removeNetworkNamespace(context.Background(), t.Name())
err := RemoveNetworkNamespace(context.Background(), t.Name())
if err != nil {
t.Fatalf("failed to remove non-existing ns with error: %v", err)
}
}

func Test_removeNetworkNamespace_HasAdapters(t *testing.T) {
defer func() {
err := removeNetworkNamespace(context.Background(), t.Name())
err := RemoveNetworkNamespace(context.Background(), t.Name())
if err != nil {
t.Errorf("failed to remove ns with error: %v", err)
}
Expand All @@ -96,7 +96,7 @@ func Test_removeNetworkNamespace_HasAdapters(t *testing.T) {
networkInstanceIDToName = nsOld
}()

ns := getOrAddNetworkNamespace(t.Name())
ns := GetOrAddNetworkNamespace(t.Name())

networkInstanceIDToName = func(ctx context.Context, id string, _ bool) (string, error) {
return "/dev/sdz", nil
Expand All @@ -105,15 +105,15 @@ func Test_removeNetworkNamespace_HasAdapters(t *testing.T) {
if err != nil {
t.Fatalf("failed to add adapter: %v", err)
}
err = removeNetworkNamespace(context.Background(), t.Name())
err = RemoveNetworkNamespace(context.Background(), t.Name())
if err == nil {
t.Fatal("should have failed to delete namespace with adapters")
}
err = ns.RemoveAdapter(context.Background(), "test")
if err != nil {
t.Fatalf("failed to remove adapter: %v", err)
}
err = removeNetworkNamespace(context.Background(), t.Name())
err = RemoveNetworkNamespace(context.Background(), t.Name())
if err != nil {
t.Fatalf("should not have failed to delete empty namepace got: %v", err)
}
Expand Down
4 changes: 4 additions & 0 deletions internal/guest/runtime/hcsv2/process.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ type containerProcess struct {
writersCalled bool
}

var _ Process = &containerProcess{}

// newProcess returns a containerProcess struct that has been initialized with
// an outstanding wait for process exit, and post exit an outstanding wait for
// process cleanup to release all resources once at least 1 waiter has
Expand Down Expand Up @@ -262,6 +264,8 @@ type externalProcess struct {
remove func(pid int)
}

var _ Process = &externalProcess{}

func (ep *externalProcess) Kill(ctx context.Context, signal syscall.Signal) error {
if err := syscall.Kill(int(ep.cmd.Process.Pid), signal); err != nil {
if err == syscall.ESRCH {
Expand Down
2 changes: 1 addition & 1 deletion internal/guest/runtime/hcsv2/standalone_container.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func setupStandaloneContainerSpec(ctx context.Context, id string, spec *oci.Spec

// Write resolv.conf
if !specInternal.MountPresent("/etc/resolv.conf", spec.Mounts) {
ns := getOrAddNetworkNamespace(getNetworkNamespaceID(spec))
ns := GetOrAddNetworkNamespace(getNetworkNamespaceID(spec))
var searches, servers []string
for _, n := range ns.Adapters() {
if len(n.DNSSuffix) > 0 {
Expand Down
23 changes: 21 additions & 2 deletions internal/guest/runtime/hcsv2/uvm.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,29 @@ func (h *Host) SetSecurityPolicy(base64Policy string) error {
return nil
}

func (h *Host) SecurityPolicyEnforcer() securitypolicy.SecurityPolicyEnforcer {
return h.securityPolicyEnforcer
}

func (h *Host) Transport() transport.Transport {
return h.vsock
}

func (h *Host) RemoveContainer(id string) {
h.containersMutex.Lock()
defer h.containersMutex.Unlock()

c, ok := h.containers[id]
if !ok {
return
}

// delete the network namespace for standalone and sandbox containers
criType, isCRI := c.spec.Annotations[annotations.KubernetesContainerType]
if !isCRI || criType == "sandbox" {
RemoveNetworkNamespace(context.Background(), id)
}

delete(h.containers, id)
}

Expand Down Expand Up @@ -580,15 +599,15 @@ func modifyCombinedLayers(ctx context.Context, rt guestrequest.RequestType, cl *
func modifyNetwork(ctx context.Context, rt guestrequest.RequestType, na *guestresource.LCOWNetworkAdapter) (err error) {
switch rt {
case guestrequest.RequestTypeAdd:
ns := getOrAddNetworkNamespace(na.NamespaceID)
ns := GetOrAddNetworkNamespace(na.NamespaceID)
if err := ns.AddAdapter(ctx, na); err != nil {
return err
}
// This code doesnt know if the namespace was already added to the
// container or not so it must always call `Sync`.
return ns.Sync(ctx)
case guestrequest.RequestTypeRemove:
ns := getOrAddNetworkNamespace(na.ID)
ns := GetOrAddNetworkNamespace(na.ID)
if err := ns.RemoveAdapter(ctx, na.ID); err != nil {
return err
}
Expand Down
Loading

0 comments on commit 132cb11

Please sign in to comment.