Refactor Testworkflow structure... (#800)

* Refactor TestWorkflow structure to provide a compute client instead of an image name

Retained the partial URL as ImageURL because this it's not readily available on the image object and daisy needs it

Add a new TestWorkflowForUnitTest helper to instantiate an empty testworkflow without worrying about the compute API

Add a testclient in TestNewTestWorkflow and verify that it's setting the properties receieved from the API

Use the compute API to check image metadata rather than image naming conventions for OS type and image architecture.

Use the compute API to check for GuestOsFeature flags to skip tests rather than curated lists

* gofmt

* Check number of guest CPUs directly in networkperf setup

* Compare arch directly in hotattach

* Handle shapevalidation shape exceptions

* gofmt/lint/vet

* Document usage of the compute.Image property

* merge GoogleCloudPlatform/master into acrate/computeapirefactor

* rebase

* gofmt/lint/vet

imagetest/README.md

@@ -114,6 +114,21 @@ checking if a Linux binary exists.
 It is suggested to start by copying an existing test package. Do not forget to add
 your test to the relevant `setup.go` file in order to add the test to the test suite.
 
+### Modifying test behavior based on image properties ###
+
+For tests that need to behave different based on whether an image is arm or x86, or linux or windows, it is preferred to use compute API properties rather than relying on image naming conventions. These properties can be found on the testworkflow Image value. The list of values can be found in the compute API documentation [here](https://pkg.go.dev/google.golang.org/api/compute/v1#Image). Some examples are in the following code snippet.
+
+```go
+func Setup(t *imagetest.Testworkflow) {
+	if t.Image.Architecture == "ARM64" {
+	//...
+	} else if utils.HasFeature(t.Image, "GVNIC") {
+	//...
+	}
+}
+```
+
+
 ## Building the container image ##
 
 From the `imagetest` directory of this repository:

imagetest/cmd/manager/main.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 
 	"cloud.google.com/go/storage"
+	"github.com/GoogleCloudPlatform/compute-daisy/compute"
 	"github.com/GoogleCloudPlatform/guest-test-infra/imagetest"
 	"github.com/GoogleCloudPlatform/guest-test-infra/imagetest/test_suites/cvm"
 	"github.com/GoogleCloudPlatform/guest-test-infra/imagetest/test_suites/disk"
@@ -199,6 +200,12 @@ func main() {
 		},
 	}
 
+	ctx := context.Background()
+	computeclient, err := compute.NewClient(ctx)
+	if err != nil {
+		log.Fatalf("Could not create compute client:%v", err)
+	}
+
 	var testWorkflows []*imagetest.TestWorkflow
 	for _, testPackage := range testPackages {
 		if filterRegex != nil && !filterRegex.MatchString(testPackage.name) {
@@ -242,7 +249,7 @@ func main() {
 			}
 
 			log.Printf("Add test workflow for test %s on image %s", testPackage.name, image)
-			test, err := imagetest.NewTestWorkflow(testPackage.name, image, *timeout, *x86Shape, *arm64Shape)
+			test, err := imagetest.NewTestWorkflow(computeclient, testPackage.name, image, *timeout, *project, *zone, *x86Shape, *arm64Shape)
 			if err != nil {
 				log.Fatalf("Failed to create test workflow: %v", err)
 			}
@@ -259,26 +266,24 @@ func main() {
 
 	log.Println("Done with setup")
 
-	ctx := context.Background()
-
-	client, err := storage.NewClient(ctx)
+	storageclient, err := storage.NewClient(ctx)
 	if err != nil {
 		log.Fatalf("failed to set up storage client: %v", err)
 	}
 
 	if *printwf {
-		imagetest.PrintTests(ctx, client, testWorkflows, *project, *zone, *gcsPath)
+		imagetest.PrintTests(ctx, storageclient, testWorkflows, *project, *zone, *gcsPath)
 		return
 	}
 
 	if *validate {
-		if err := imagetest.ValidateTests(ctx, client, testWorkflows, *project, *zone, *gcsPath); err != nil {
+		if err := imagetest.ValidateTests(ctx, storageclient, testWorkflows, *project, *zone, *gcsPath); err != nil {
 			log.Printf("Validate failed: %v\n", err)
 		}
 		return
 	}
 
-	suites, err := imagetest.RunTests(ctx, client, testWorkflows, *project, *zone, *gcsPath, *parallelCount, *parallelStagger, testProjectsReal)
+	suites, err := imagetest.RunTests(ctx, storageclient, testWorkflows, *project, *zone, *gcsPath, *parallelCount, *parallelStagger, testProjectsReal)
 	if err != nil {
 		log.Fatalf("Failed to run tests: %v", err)
 	}

imagetest/fixtures_test.go

@@ -11,10 +11,7 @@ import (
 // TestAddMetadata tests that *TestVM.AddMetadata succeeds and that it
 // populates the instance.Metadata map.
 func TestAddMetadata(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -35,10 +32,7 @@ func TestAddMetadata(t *testing.T) {
 // TestReboot tests that *TestVM.Reboot succeeds and that the appropriate stop
 // and new final wait steps are created in the workflow.
 func TestReboot(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -70,10 +64,7 @@ func TestReboot(t *testing.T) {
 // TestCreateVMMultipleDisks tests that after creating a VM with multiple disks,
 // the correct step dependencies are in place.
 func TestCreateVMMultipleDisks(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	disks := []*compute.Disk{{Name: "vm"}, {Name: "mountdisk", Type: PdSsd, SizeGb: 100}}
 	tvm, err := twf.CreateTestVMMultipleDisks(disks, map[string]string{})
 	if err != nil {
@@ -154,10 +145,7 @@ func TestCreateVMMultipleDisks(t *testing.T) {
 // TestCreateVMRebootGA tests that after creating a VM with multiple disks, if the vm
 // is expected to reboot during the test, a special guest attribute is used as the wait signal.
 func TestCreateVMRebootGA(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	disks := []*compute.Disk{{Name: "vm"}, {Name: "mountdisk", Type: PdSsd, SizeGb: 100}}
 	rebootGAParams := map[string]string{ShouldRebootDuringTest: "true"}
 	tvm, err := twf.CreateTestVMMultipleDisks(disks, rebootGAParams)
@@ -260,10 +248,7 @@ func TestCreateVMRebootGA(t *testing.T) {
 // TestRebootMultipleDisks creates a VM using multiple disks, and then runs
 // the same tests as TestReboot.
 func TestRebootMultipleDisks(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	disks := []*compute.Disk{{Name: "vm"}, {Name: "mountdisk", Type: PdBalanced, SizeGb: 100}}
 	testMachineType := "c3-standard-4"
 	pdBalancedParams := map[string]string{"machineType": testMachineType}
@@ -302,10 +287,7 @@ func TestRebootMultipleDisks(t *testing.T) {
 // that the appropriate resize and new final wait steps are created in the
 // workflow.
 func TestResizeDiskAndReboot(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -331,10 +313,7 @@ func TestResizeDiskAndReboot(t *testing.T) {
 // TestEnableSecureBoot tests that *TestVM.EnableSecureBoot succeeds and
 // populates the ShieldedInstanceConfig struct.
 func TestEnableSecureBoot(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -384,12 +363,9 @@ func TestWaitForQuotaStep(t *testing.T) {
 	}
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-			if err != nil {
-				t.Errorf("failed to create test workflow: %v", err)
-			}
+			twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 			for _, quota := range tc.input {
-				err = twf.waitForQuotaStep(quota, tc.name)
+				err := twf.waitForQuotaStep(quota, tc.name)
 				if err != nil {
 					t.Errorf("failed to append quota: %v", err)
 				}
@@ -414,10 +390,7 @@ func TestWaitForQuotaStep(t *testing.T) {
 // TestUseGVNIC tests that *TestVM.UseGVNIC succeeds and
 // populates the Network Interface with a NIC type of GVNIC.
 func TestUseGVNIC(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -434,10 +407,7 @@ func TestUseGVNIC(t *testing.T) {
 // TestAddAliasIPRanges tests that *TestVM.AddAliasIPRanges succeeds and that
 // it fails if *TestVM.AddCustomNetwork hasn't been called first.
 func TestAddAliasIPRanges(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -463,10 +433,7 @@ func TestAddAliasIPRanges(t *testing.T) {
 // TestSetCustomNetwork tests that *TestVM.AddCustomNetwork succeeds and that
 // it fails if testworkflow.CreateNetwork has not been called first.
 func TestSetCustomNetwork(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -484,10 +451,7 @@ func TestSetCustomNetwork(t *testing.T) {
 // succeeds with a subnet argument and that it fails if
 // *Network.CreateSubnetwork has not been called first.
 func TestSetCustomNetworkAndSubnetwork(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -511,10 +475,7 @@ func TestSetCustomNetworkAndSubnetwork(t *testing.T) {
 // TestAddSecondaryRange tests that AddSecondaryRange populates the
 // subnet.SecondaryIpRanges struct.
 func TestAddSecondaryRange(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	network, err := twf.CreateNetwork("network", false)
 	if err != nil {
 		t.Errorf("failed to create network: %v", err)
@@ -535,14 +496,11 @@ func TestAddSecondaryRange(t *testing.T) {
 // TestCreateNetworkDependenciesReverse tests that the create-vms step depends
 // on the create-networks step if they are created in order.
 func TestCreateNetworkDependencies(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
-	if _, err = twf.CreateNetwork("network", false); err != nil {
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
+	if _, err := twf.CreateNetwork("network", false); err != nil {
 		t.Errorf("failed to create network: %v", err)
 	}
-	if _, err = twf.CreateTestVM("vm"); err != nil {
+	if _, err := twf.CreateTestVM("vm"); err != nil {
 		t.Errorf("failed to create network: %v", err)
 	}
 	if _, ok := twf.wf.Dependencies[createNetworkStepName]; ok {
@@ -567,14 +525,11 @@ func TestCreateNetworkDependencies(t *testing.T) {
 // TestCreateNetworkDependenciesReverse tests that the create-vms step depends
 // on the create-networks step if they are created in reverse.
 func TestCreateNetworkDependenciesReverse(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
-	if _, err = twf.CreateTestVM("vm"); err != nil {
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
+	if _, err := twf.CreateTestVM("vm"); err != nil {
 		t.Errorf("failed to create network: %v", err)
 	}
-	if _, err = twf.CreateNetwork("network", false); err != nil {
+	if _, err := twf.CreateNetwork("network", false); err != nil {
 		t.Errorf("failed to create network: %v", err)
 	}
 	if _, ok := twf.wf.Dependencies[createNetworkStepName]; ok {
@@ -597,10 +552,7 @@ func TestCreateNetworkDependenciesReverse(t *testing.T) {
 }
 
 func TestAddUser(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create network: %v", err)
@@ -623,10 +575,7 @@ func TestAddUser(t *testing.T) {
 }
 
 func TestForceMachineType(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)
@@ -641,10 +590,7 @@ func TestForceMachineType(t *testing.T) {
 }
 
 func TestForceZone(t *testing.T) {
-	twf, err := NewTestWorkflow("name", "image", "30m", "x86", "arm64")
-	if err != nil {
-		t.Errorf("failed to create test workflow: %v", err)
-	}
+	twf := NewTestWorkflowForUnitTest("name", "image", "30m")
 	tvm, err := twf.CreateTestVM("vm")
 	if err != nil {
 		t.Errorf("failed to create test vm: %v", err)

imagetest/test_suites/cvm/setup.go

@@ -2,9 +2,9 @@ package cvm
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/GoogleCloudPlatform/guest-test-infra/imagetest"
+	"github.com/GoogleCloudPlatform/guest-test-infra/imagetest/utils"
 )
 
 // Name is the name of the test package. It must match the directory name.
@@ -14,21 +14,19 @@ const vmName = "vm"
 
 // TestSetup sets up test workflow.
 func TestSetup(t *imagetest.TestWorkflow) error {
-	if strings.Contains(t.Image, "arm64") || strings.Contains(t.Image, "aarch64") {
+	if t.Image.Architecture == "ARM64" {
 		t.Skip("CVM is not supported on arm")
 	}
-	if strings.Contains(t.Image, "windows") || strings.Contains(t.Image, "rhel-7") || strings.Contains(t.Image, "centos-7") || strings.Contains(t.Image, "debian-10") || strings.Contains(t.Image, "rhel-8-1-sap") || strings.Contains(t.Image, "rhel-8-2-sap") {
-		t.Skip(fmt.Sprintf("%v does not support CVM", t.Image))
+	if !utils.HasFeature(t.Image, "SEV_CAPABLE") {
+		t.Skip(fmt.Sprintf("%s does not support CVM", t.Image.Name))
 	}
-
 	vm, err := t.CreateTestVM(vmName)
 	if err != nil {
 		return err
 	}
 	vm.EnableConfidentialInstance()
 	vm.SetMinCPUPlatform("AMD Milan")
 	vm.ForceMachineType("n2d-standard-2")
-
 	vm.RunTests("TestCVMEnabled")
 	return nil
 }

imagetest/test_suites/hotattach/setup.go

@@ -2,7 +2,6 @@ package hotattach
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/GoogleCloudPlatform/guest-test-infra/imagetest"
 	"google.golang.org/api/compute/v1"
@@ -31,7 +30,7 @@ func TestSetup(t *imagetest.TestWorkflow) error {
 	}
 	// The extra scope is required to call detachDisk and attachDisk.
 	hotattachParams := map[string]string{"extraScopes": "https://www.googleapis.com/auth/cloud-platform"}
-	if strings.Contains(t.Image, "arm64") {
+	if t.Image.Architecture == "ARM64" {
 		hotattachParams["machineType"] = "t2a-standard-8"
 	} else {
 		hotattachParams["machineType"] = "n2-standard-8"

imagetest/test_suites/imageboot/setup.go

@@ -47,7 +47,7 @@ func TestSetup(t *imagetest.TestWorkflow) error {
 	vm3.RunTests("TestStartTime|TestBootTime")
 
 	for _, r := range sbUnsupported {
-		if r.MatchString(t.Image) {
+		if r.MatchString(t.Image.Name) {
 			return nil
 		}
 	}

imagetest/test_suites/metadata/setup.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	"github.com/GoogleCloudPlatform/guest-test-infra/imagetest"
+	"github.com/GoogleCloudPlatform/guest-test-infra/imagetest/utils"
 	"google.golang.org/api/compute/v1"
 )
 
@@ -99,7 +100,7 @@ func TestSetup(t *imagetest.TestWorkflow) error {
 	var timeByteArr []byte
 
 	// Determine if the OS is Windows or Linux and set the appropriate script metadata.
-	if strings.Contains(t.Image, "windows") {
+	if utils.HasFeature(t.Image, "WINDOWS") {
 		startupByteArr, err = scripts.ReadFile(startupScriptWindowsURL)
 		if err != nil {
 			return err