Added LCOW functional tests and benchmarks for uVMs and containers. #1351

helsaawy · 2022-04-13T21:54:06Z

This PR is broken into two commits: the first has the core changes, and the second is vendoring and switching other tests to use the new testing utilities.

Added functional tests and benchmarks for LCOW uVMs and containers and updated other LCOW tests to work.
Non-working tests are skipped.

Split out test utilities from test/functional to an internal package, and updated it o use containerd instead of docker.
Updated cri-containerd and runhcs tests to use moved code in test\internal.

Updated k8s.io/cri-api to v0.22 to useWindowsPodSandboxConfig struct

Relies on PRs:

Signed-off-by: Hamza El-Saawy hamzaelsaawy@microsoft.com

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). The tests themselves are broken out here: microsoft#1352 microsoft#1351 Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

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: microsoft#1352 microsoft#1351 Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

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>

helsaawy · 2022-05-10T16:18:27Z

rebased and fixed test-vendor CI issue

helsaawy · 2022-06-14T16:56:40Z

rebased to fix merge conflicts, removed branch .\test\vendor

helsaawy · 2022-07-21T22:21:01Z

rebased, updated tests

ambarve

Thanks a lot for working on this Hamza! This is a ton of great work!

test/internal/timeout/timeout.go

test/functional/lcow_container_test.go

test/cri-containerd/runpodsandbox_test.go

test/functional/lcow_container_bench_test.go

scripts/Test-Functional.ps1

test/internal/constants/constants.go

test/internal/containerd/containerd.go

test/internal/layers/layerfolders.go

katiewasnothere · 2022-08-01T22:17:39Z

test/functional/lcow_bench_test.go

+	b.StopTimer()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		opts := defaultLCOWOptions(b, b.Name())


Does each iteration count as a "subtest" for the benchmark? If no, wouldn't b.Name() not be unique?

So its a little weird, but this is based on how the docs tell you to do it, a benchmark run of BenchmarkLCOW_UVM_Create will include as many iterations (b.N) the target -benchtime criteria is met (either a set number, eg. -benchtime 1000x or duration, eg. -benchtime 3s)
All those iterations are accumulated under one timer, which can then be repeated via -count

So if we with
'-test.count=2' '-test.run=^#' '-test.benchmem' '-test.bench=.' '-test.benchtime=2x'
The output would look something like (I dont know why go append the 12 here):

BenchmarkLCOW_UVM_Create-12 2 84758700 ns/op 47800 B/op 363 allocs/op BenchmarkLCOW_UVM_Create-12 2 81733600 ns/op 42584 B/op 348 allocs/op BenchmarkLCOW_UVM_Create-12 2 83118950 ns/op 43904 B/op 385 allocs/op

benchstat then aggregates benchmarks with the same name, so we want the non-uniqueness:

LCOW_UVM_Create-12 83.2ms ± 2%

how does creating a new LCOW work if the names are the same?

the cleanup() call on line 33 deletes the uvm, so the name can be reused

katiewasnothere · 2022-08-01T22:22:05Z

test/functional/lcow_bench_test.go

+		vm := uvm.CreateLCOW(ctx, b, defaultLCOWOptions(b, b.Name()))
+
+		b.StartTimer()
+		if err := vm.Start(ctx); err != nil {


why aren't we using the same 'uvm.Start' function here?

it has a bunch of test-related overhead I dont want to profile

hcsshim/test/internal/uvm/uvm.go

Lines 17 to 28 in bf3434f

f := func() {

if err := vm.Close(); err != nil {

t.Logf("could not close vm %q: %v", vm.ID(), err)

}

}

if err != nil {

t.Helper()

t.Fatalf("could not start UVM: %v", err)

}

return f

That's pretty negligible code though, I wouldn't expect any of it to cause a marked slowdown

Thats true, I can change it back if you think it distracts unnecessarily?

test/functional/lcow_container_bench_test.go

katiewasnothere · 2022-08-03T22:51:52Z

test/internal/cmd/cmd.go

+}
+
+func Wait(_ context.Context, t testing.TB, c *cmd.Cmd) int {
+	// todo, wait on context.Done


why did we not do this in this PR?

I didnt need it yet, and I didnt want to include a whole new batch of tests to see if canceling contexts work as expected
It was more of a note for when we expand the functional tests

katiewasnothere · 2022-08-03T22:59:58Z

test/functional/lcow_container_bench_test.go

+			// been waited on (usually via a WaitForProcess request) and had its exit code read
+			// (hcsv2/process.go:(*containerProcess).Wait).
+			//
+			// So ... to test container kill and wait times, we need to first start and wait on the init process


So this just tests the time for the kill and wait command to return when the container init is already exited? I would think the thing we really care about is how long it takes to stop a simple program that's running.

katiewasnothere · 2022-08-03T23:05:45Z

test/functional/lcow_container_test.go

+			id := strings.ReplaceAll(t.Name(), "/", "")
+			scratch, _ := layers.ScratchSpace(ctx, t, vm, "", "", cache)
+			spec := oci.CreateLinuxSpec(ctx, t, id,
+				oci.DefaultLinuxSpecOpts(id,
+					ctrdoci.WithProcessArgs(tt.args...),
+					oci.WithWindowsLayerFolders(append(ls, scratch)))...)


This could probably all be refactored out as well

test/internal/cmd/io.go

test/functional/lcow_test.go

test/functional/main_test.go

test/internal/constants/images.go

test/internal/container/container.go

katiewasnothere

Rebase and then lgtm

Split out utility functions from `test/functional` into an internal package, separate from functional tests. Updated code to use containerd instead of docker. Added new functional tests and benchmarks for LCOW uVM containers, and updated other LCOW tests as well. Not all (LCOW) functional tests were updated, and most others are now explicitly skipped. Updated `k8s.io/cri-api` to v0.22 to include `WindowsPodSandboxConfig` struct Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

Added deprecated warning to `layers.LayerFolders`, which relies on docker. Added doc comment to functional tests to clarify overlap with other tests. Removed unnecessary parameter in `WaitForError`. Updated snapshotter logic Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

helsaawy mentioned this pull request Apr 13, 2022

Support for LCOW tests and benchmarks #1349

Closed

helsaawy mentioned this pull request Apr 19, 2022

Features/bugfixes to support LCOW GCS tests #1360

Merged

helsaawy force-pushed the he/lcowfunc branch from 680f42f to e34112c Compare April 19, 2022 21:30

helsaawy mentioned this pull request May 4, 2022

Linux GCS tests and benchmarks #1352

Merged

3 tasks

helsaawy force-pushed the he/lcowfunc branch 3 times, most recently from b358cef to 5bc615a Compare May 4, 2022 21:21

helsaawy marked this pull request as ready for review May 4, 2022 21:21

helsaawy requested a review from a team as a code owner May 4, 2022 21:21

helsaawy force-pushed the he/lcowfunc branch 3 times, most recently from 40400b1 to f621e11 Compare May 5, 2022 00:26

katiewasnothere mentioned this pull request May 10, 2022

Unset text attribute for vendored files in gitattributes #1393

Merged

helsaawy force-pushed the he/lcowfunc branch 2 times, most recently from bf77250 to 531d89d Compare May 10, 2022 15:52

helsaawy force-pushed the he/lcowfunc branch 3 times, most recently from 24b1ac0 to 7381576 Compare May 11, 2022 15:14

msscotb assigned katiewasnothere and ambarve May 11, 2022

helsaawy force-pushed the he/lcowfunc branch 3 times, most recently from 2f110e9 to 6831726 Compare June 14, 2022 16:56

helsaawy force-pushed the he/lcowfunc branch from 6831726 to 8aae1ff Compare July 21, 2022 22:20

ambarve reviewed Jul 28, 2022

View reviewed changes

helsaawy force-pushed the he/lcowfunc branch from c805fb3 to bf3434f Compare July 28, 2022 19:43

ambarve approved these changes Jul 29, 2022

View reviewed changes

katiewasnothere reviewed Aug 1, 2022

View reviewed changes