Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(performance): setup vms #40

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

universal-itengineer
Copy link
Member

Description

Part of tests

Why do we need it, and what problem does it solve?

What is the expected result?

Checklist

  • The code is covered by unit tests.
  • e2e tests passed.
  • Documentation updated according to the changes.
  • Changes were tested in the Kubernetes cluster manually.

tests/e2e/vm_creation_performance_test.go Outdated Show resolved Hide resolved
tests/e2e/vm_creation_performance_test.go Outdated Show resolved Hide resolved
tests/e2e/vm_creation_performance_test.go Outdated Show resolved Hide resolved
tests/e2e/vm_creation_performance_test.go Outdated Show resolved Hide resolved
@universal-itengineer universal-itengineer changed the title test(performance): setup 20 plus vms test(performance): setup 26 vms Apr 8, 2024
Comment on lines 198 to 225
vmList, err := client.VirtualMachines(conf.Namespace).List(context.TODO(), metav1.ListOptions{})
Expect(err).NotTo(HaveOccurred())

start := time.Now()
fmt.Println("Starting at", start)

for len(vmMap) != vmCount {
vmList, err = client.VirtualMachines(conf.Namespace).List(context.TODO(), metav1.ListOptions{})
Expect(err).NotTo(HaveOccurred())

now := time.Now()
elapsed := now.Sub(start)

for _, vm := range vmList.Items {
if vmMap[vm.Name] != vm.Name && string(vm.Status.Phase) == "Running" {
vmMap[vm.Name] = string(vm.Status.Phase)
}
}

if elapsed > overallTimeout {
break
}

time.Sleep(1 * time.Second)
}
fmt.Println("Finished at", time.Now())
fmt.Println("Duration", time.Now().Sub(start))
Expect(len(vmList.Items)).To(Equal(vmCount))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be simpler with Eventually that has overall timeout and an error checking.

https://onsi.github.io/ginkgo/#patterns-for-asynchronous-testing
https://onsi.github.io/gomega/#eventually

Quick draft:

Eventually(func() (int, error) {
  vmList, err := client.VirtualMachines(conf.Namespace).List(context.TODO(), metav1.ListOptions{})
  if err != nil {
    return 0, err
  }

  runningCount := 0
  for _, vm := range vmList.Items {
    if string(vm.Status.Phase) == "Running" {
      runningCount++
    }
  }
  return runningCount, nil
}).WithTimeout(overallTimeout).Should(Equal(vmCount))

Overall duration better to calculate in AfterAll block: it will report duration even for failed test.

deleteGracePeriodSeconds int64 = 30
)

var cvmi *v1alpha2.ClusterVirtualMachineImage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either cvmi can be a local variable, or you forgot to delete CVMI in AfterAll


Context("VM", func() {
cvmi, err = CVMI(client, cvmiName, "create")
Expect(err).NotTo(HaveOccurred(), "%s", err)
Copy link
Member

@diafour diafour Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Expect(err).NotTo(HaveOccurred(), "%s", err)
Expect(err).NotTo(HaveOccurred(), "should create CVMI %s", cvmiName)

%s, err is redundant: ginkgo will print err, as an argument from the Expect. Better add something not in Expect but helpful for debugging, some arguments from previous call, e.g. cvmiName. See https://github.com/deckhouse/virtualization/pull/40/files#diff-d7d5b8095ea72fb8452a176dd86499806e1dd1e384473db6d4853ff3e2023df0R148

Comment on lines 167 to 171
clientConfig := kubeclient.DefaultClientConfig(&pflag.FlagSet{})
client, err := kubeclient.GetClientFromClientConfig(clientConfig)
if err != nil {
log.Fatalf("Cannot obtain Virtualization client: %v\n", err)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make clientConfig and client a variables and initialize them in BeforeAll, so Expect can be used to check for error.


res, err = client.ClusterVirtualMachineImages().Create(context.TODO(), &cvmi, metav1.CreateOptions{})
if err != nil {
log.Fatal(err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return err from here, so ginkgo runs Expect. log.Fatal exits immediately and ginkgo will not print helpful report.

if err != nil {
log.Fatal(err)
}
fmt.Println(res.Name, "was deleted")
Copy link
Member

@diafour diafour Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend to use GinkgoWriter instead all fmt.Print* in ginkgo tests, so these debug messages can be enabled with -v flag or disabled if not needed.
See https://onsi.github.io/ginkgo/#logging-output

Copy link
Member

@diafour diafour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some ginkgo best practices.

@universal-itengineer universal-itengineer force-pushed the test/e2e/perf-20-plus-vm branch 2 times, most recently from bfbcae2 to 9356aa3 Compare April 16, 2024 18:02
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need ansible-facts here?

Comment on lines +167 to +168
clientConfig := kubeclient.DefaultClientConfig(&pflag.FlagSet{})
client, err := kubeclient.GetClientFromClientConfig(clientConfig)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use the same context in all e2e tests.

@fl64 fl64 marked this pull request as draft June 24, 2024 12:16
Signed-off-by: Nikita korolev <[email protected]>
@universal-itengineer universal-itengineer force-pushed the test/e2e/perf-20-plus-vm branch 2 times, most recently from 061cad4 to 9a585c4 Compare July 26, 2024 08:48
Signed-off-by: Nikita korolev <[email protected]>
@universal-itengineer universal-itengineer changed the title test(performance): setup 26 vms test(performance): setup vms Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants