test/e2e: Switch tests away from default namespace

[stevenhorsman]

• Currently we are run e2e-test cases under a namespace defined by

namespace := envconf.RandomName("default", 7)

The way that
envconf.RandomName works is to add random name after the prefix, up to the specified length. As default is 7 chars long then envconf.RandomName("default", 7) will just return default.

• This means that we have a lot of duplicated code in each test case that does nothing, so we should resolved this.

• It's also a good idea to have a single namespace for testing anyway, like we;ve done in kata-containers, as in case things fail and go wrong it's easier to clean up by deleting the namespace, though obviously default shouldn't be that namespace here.

• This PR adds a new e2e-test namespace to address these.

Fixes: #1652

[stevenhorsman]

Creating as draft PR as I've not tested yet as I think it will fail until #1651 is merged and this rebased on it.

[liudalibj]

We need make sure the created namespace is ready to use, eg:

...
const WAIT_NAMESPACE_AVAILABLE_TIMEOUT = time.Second * 120
...
		log.Infof("Wait for the %s namespace be ready", E2eNamespace)
		if err := wait.For(conditions.New(client.Resources()).ResourceMatch(nsObj, func(object k8s.Object) bool {
			ns, ok := object.(*v1.Namespace)
			if !ok {
				log.Printf("Not a namespace object: %v", object)
				return false
			}
			return ns.Status.Phase == corev1.NamespaceActive
		}), wait.WithTimeout(WAIT_NAMESPACE_AVAILABLE_TIMEOUT)); err != nil {
			return ctx, err
		}
		log.Infof("Namespace '%s' is ready for e2e-test", E2eNamespace)

otherwise we will got error looks like:

...
=== RUN   TestLibvirtCreateSimplePod/SimplePeerPod_test
    assessment_runner.go:223: pods "simple-test" is forbidden: error looking up service account e2e-test-8fb/default: serviceaccount "default" not found
--- FAIL: TestLibvirtCreateSimplePod (1.49s)
    --- FAIL: TestLibvirtCreateSimplePod/SimplePeerPod_test (1.49s)
...

[liudalibj]

And do we need delete the created namespace at L173?
eg:

		// Delete the namespace for all test cases
		if nsObj != nil {
			client, err := cfg.NewClient()
			if err != nil {
				return ctx, err
			}
			if err = client.Resources().Delete(ctx, nsObj); err != nil {
				return ctx, err
			}
			log.Infof("Deleting namespace %s...", nsObj.Name)
			if err = wait.For(conditions.New(
				client.Resources()).ResourceDeleted(nsObj),
				wait.WithInterval(5*time.Second),
				wait.WithTimeout(60*time.Second)); err != nil {
				return ctx, err
			}
			log.Infof("Namespace %s has been successfully deleted within 60s", nsObj.Name)
		}
		return ctx, nil
	})

	// Start the tests execution workflow.
	os.Exit(testEnv.Run(m))
}

[stevenhorsman]

Good point, we're still in a confusing (at least to me) place with respect to clean up. Based on the code it looks like there is a gap where we do teardown, but don't delete the cluster, so I think namespace deletion needs to be done in those circumstances, but they aren't especially common. It's still worth doing, so I'll look at adding that. Thanks for the suggestions!

[stevenhorsman]

Hey @liudalibj - something that occurs to me after reading your good suggestions about improving the namespace function - I think rather than adding the create and wait for namespace and delete namespace to main_tests.go, now it's grown we are better to move it elsewhere for easier readability. I know you've done some refactoring recently, so wondered if you had any good suggestions? I was thinking common.go, but that's more peer pods and less Kubernetes type functions. Maybe assessment_helpers.go makes sense (though the name might want improving in future)?

[stevenhorsman]

I decided on assessment_helpers.go in the short-term, but this can be changed before merging if required.

[liudalibj]

Check the k8s e2e test codes, it seems that they create/delete namespace for every test case:

• create
  https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/framework.go#L260
• delete
  https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/framework.go#L353

So maybe we can use same logical?

• add the e2e namespace object to TestCase struct
• create and waiting the namespace at the top of WithSetup
• delete the namespace at the end of Teardown

[liudalibj]

Create namespace once for all test cases and create namespace for test case everytime, both one is fine to me.

[stevenhorsman]

Yeah, I saw those examples, but I don't want to create a unique namespace for each test case, but rather want them all shared for each run, so that we can easily delete all pods/services etc if we get a failure happening and want to clean things up. This is based on the approach that kata-containers moved to a while ago: kata-containers/kata-containers#7238

[stevenhorsman]

I am happy to discuss if people think that unique namespace per test, rather than test run is a better approach though. Sorry if I made it sound black and white. I was just implementing the issue description, but I'm also aware that I ghost-wrote that, so it's good too much of my thinking in already!

[stevenhorsman]

I did abort a previous run in the middle of execution, but it seems like the error that you saw might not be related to the namespace not being in Active state:

time="2024-01-09T10:30:07Z" level=info msg="Creating namespace coco-pp-e2e-test-6afd3aac"
time="2024-01-09T10:30:07Z" level=info msg="Wait for namespace 'coco-pp-e2e-test-6afd3aac' be ready"
time="2024-01-09T10:30:12Z" level=info msg="Namespace 'coco-pp-e2e-test-6afd3aac' is ready"
=== RUN   TestLibvirtCreateSimplePod
=== RUN   TestLibvirtCreateSimplePod/SimplePeerPod_test
    assessment_runner.go:223: pods "simple-test" is forbidden: error looking up service account coco-pp-e2e-test-6afd3aac/default: serviceaccount "default" not found
--- FAIL: TestLibvirtCreateSimplePod (1.81s)
    --- FAIL: TestLibvirtCreateSimplePod/SimplePeerPod_test (1.81s)
=== RUN   TestLibvirtCreateSimplePodWithNydusAnnotation
=== RUN   TestLibvirtCreateSimplePodWithNydusAnnotation/SimplePeerPod_test
    assessment_runner.go:223: pods "alpine" is forbidden: error looking up service account coco-pp-e2e-test-6afd3aac/default: serviceaccount "default" not found
...

When I checked the namespace logic wasn't broken it seems to be okay:

# kubectl get namespaces
NAME                             STATUS   AGE
autorules                        Active   20h
coco-pp-e2e-test-6afd3aac        Active   68s
confidential-containers-system   Active   20h
default                          Active   20h
ingress-nginx                    Active   20h
kube-flannel                     Active   20h
kube-node-lease                  Active   20h
kube-public                      Active   20h
kube-system                      Active   20h

so there might be something else going on related to us creating the service account in another namespace?

I didn't see it on my early test runs though, so I'll try and uninstall the CAA and re-try in...

[liudalibj]

All changes looks good to me,
added comments for how to

1. make the E2eNamespace be random to coco-pp-e2e-test-xxx
2. createNamespace and deleteNamespace

[stevenhorsman]

Thanks for the suggestions @liudalibj - I've updated them and made you a co-author on the wait and delete namespace code as it's heavily based on your suggestions. Please take a look when you get a chance and let me know if there is anything else I've missed.

[wainersm]

LGTM. Thanks @stevenhorsman @liudalibj!

Ah, one namespace per test increases their isolation to avoid messing with each other, but is more complex to implement and, afaik, we don't have conflicting tests. So creating a single-to-all namespace seems the better approach to solve the actual problem which is to clean up everything at execution end.

[stevenhorsman]

The e2e ci shows in the logs that the namespace creation is working properly there:

Wait for the kata-remote runtimeclass be created
time="2024-01-09T13:58:24Z" level=info msg="Creating namespace 'coco-pp-e2e-test-36d55f67'..."
time="2024-01-09T13:58:24Z" level=info msg="Wait for namespace 'coco-pp-e2e-test-36d55f67' be ready"
time="2024-01-09T13:58:29Z" level=info msg="Namespace 'coco-pp-e2e-test-36d55f67' is ready"

It doesn't do TEARDOWN (as it just bins the VM), so we don't test/log the deletion of the namespace

[liudalibj]

LGTM
thanks @stevenhorsman