add test cases of volume hot-plug for VM #857

lanfon72 · 2023-06-11T17:07:33Z

Changes

ADD [apiclient] vm.add_volume to hot plug volumes
ADD [apiclient] vm.remove_volume to hot unplug volumes
ADD TestHotPlugVolume for volume hot plug/unplug
- new test cases: test_add, test_remove
- new fixtures: small_volume
DEL unused fixtures: virtctl, kubeconfig_file

lanfon72 · 2023-06-12T13:08:21Z

Tested on harvester-runtests#320

albinsun · 2023-06-13T10:01:38Z

harvester_e2e_tests/integration/test_vm_functions.py

+
+    yield vol_name, size
+
+    code, data = api_client.volumes.delete(vol_name)


Do we need to check delete done to avoid affect TC later?

I think it should be fine, the volume name is unique and no other test cases will use it.
And we are expecting functionality of volume deletion been tested in other test cases.

khushboo-rancher

In general looks good. Some improvements.

khushboo-rancher · 2023-06-14T20:08:08Z

harvester_e2e_tests/integration/test_vm_functions.py

+        code, data = api_client.vms.start(unique_vm_name)
+        endtime = datetime.now() + timedelta(seconds=wait_timeout)
+        while endtime > datetime.now():
+            code, data = api_client.vms.get_status(unique_vm_name)
+            if 200 == code:
+                phase = data.get('status', {}).get('phase')
+                conds = data.get('status', {}).get('conditions', [{}])
+                if ("Running" == phase
+                   and "AgentConnected" == conds[-1].get('type')
+                   and data['status'].get('interfaces')):
+                    break
+            sleep(3)
+        else:
+            raise AssertionError(
+                f"Failed to Start VM({unique_vm_name}) with errors:\n"
+                f"Status: {data.get('status')}\n"
+                f"API Status({code}): {data}"
+            )


I feel, we should have a fixture running_vm instead of writing starting vm code in the test. As in future, many test cases will require running vm to start with.
Could you help to implement that?

we need to start the VM because we leverage existing stopped_vm which already defined, stopped_vm would be more useful when we are going to update the VM or especially cloud-config.

Of course we can add another running_vm fixture, but as you mentioned in next comment, it will add more lines rather than check the VM is started.

It is a trade off, in here, I would prefer reuse existing fixture and start the VM to make sure the VM just started and accessible.

Can you elaborate on how the running_vm will add more lines? I'm assuming a running_vm fixture will have the checks that the vm is started successfully.

Running_vm could be useful, quick and convenient to use for other test cases.

2 lines for function naming and decorator, 1 lines for return, and lines same as the codes, is it the minimal lines to create the fixture and depends on stopped_vm

khushboo-rancher · 2023-06-14T20:10:48Z

harvester_e2e_tests/integration/test_vm_functions.py

+        with host_shell.login(host_ip, jumphost=True) as h:
+            vm_sh = vm_shell(ssh_user, pkey=pri_key)
+            endtime = datetime.now() + timedelta(seconds=wait_timeout)
+            while endtime > datetime.now():
+                try:
+                    vm_sh.connect(vm_ip, jumphost=h.client)
+                except ChannelException as e:
+                    login_ex = e
+                    sleep(3)
+                else:
+                    break
+            else:
+                raise AssertionError(f"Unable to login to VM {unique_vm_name}") from login_ex


Same with checking the login to VM, we should have a common method. So that we can use is at multiple places and keep the test simple.

I remember we were discussed it before, to make those thing into common functions will lead us to the way to the structure of legacy code, then test cases will looks like:

def tets_something(fixture_a, fix_b): utils.create_something() # ... etc

When we add new tests and use utils.create_something, it make those test cases depends on the common function, when one of tests failed, we will need fix codes in utils.create_something, then it will affect other test cases because they really depends on it. (It is what legacy code did.)

Of course I totally agree that repeat code is not good for automation, but currently It make test cases independent with each others and we can view the whole step in a test case, not refer to other functions, and I still thinking whether there have a better way to organize those repeated code in the future, to keep readability and clarity.

Or you can demonstrate a show case for me that how to organize them in a common function to prevent the results like legacy code or longhorn's bunch imports from common import .... ?

It's not necessary that you create a common file and put the common methods there but you can create common methods required within a module at least.

For example, In the test test_add I see there are two attempts to login to a VM, we can avoid writing same lines of code again and have a method login_to_vm. Writing repeated code is hard to maintain in future, if something changes you need to modify same things at multiple places.

I think it is not necessary to emphasize that a test case will frequently read to addressing the fail is a real bug or need a implement improvement, otherwise it will not be modified, so not couple with others function should be the highest priority than code lines.

but yeah, it is just my opinion like #840 , so let’s follow your design.

@lanfon72 It's not about your or my opinion but I recommend let's follow what is best for Harvester test. The goal of these tests is to have good coverage and nicely written cases which are easy to read and maintain.
I think the discussion here is not working. Let's discuss this offline.

I think we have different concept about automation testing. what is care about the test case is:

Test isolation

readability, including test reports

accurate and stability

I had explained when I migrating legacy code to current implementation, one of problems is, legacy code calling sharing functions which make tests not isolated. And I said I don't have another better way to reduce repeated code for now.

In my opinion, in automation testing, repeated code is not harmful, sharing functions does.
I will not say something like good, easy, or nicely, because it will not explain anything.

I think the discussion here is not working. Let's discuss this offline.

If I can't persuade to not do that, I will just follow your instructions.
I don't think discuss anything offline will solve the problem, same as #840 is not closed, in my experienced, it is not necessary and should be closed, and we discussed offline; same as #363, I had explained in the issue, but yeah, let's follow your instructions.

khushboo-rancher · 2023-06-14T20:13:56Z

harvester_e2e_tests/integration/test_vm_functions.py

+        # Login to VM to verify volume hot pluged
+        with host_shell.login(host_ip, jumphost=True) as h:
+            vm_sh = vm_shell(ssh_user, pkey=pri_key)
+            endtime = datetime.now() + timedelta(seconds=wait_timeout)
+            while endtime > datetime.now():
+                try:
+                    vm_sh.connect(vm_ip, jumphost=h.client)
+                except ChannelException as e:
+                    login_ex = e
+                    sleep(3)
+                else:
+                    break
+            else:
+                raise AssertionError(f"Unable to login to VM {unique_vm_name}") from login_ex


As mentioned above, repeated code for Login, we can replace this with common method.

khushboo-rancher

LGTM

lanfon72 added 2 commits June 12, 2023 01:05

+ [apiclient] implemnt volume hot plug for VM

25ef9b6

+ [e2e] add test cases for volume hot pluging

832af6d

lanfon72 force-pushed the e2e branch from 94d1c10 to 832af6d Compare June 12, 2023 12:39

lanfon72 marked this pull request as ready for review June 12, 2023 12:42

lanfon72 requested a review from a team June 12, 2023 12:43

albinsun reviewed Jun 13, 2023

View reviewed changes

albinsun requested a review from a team June 13, 2023 10:05

lanfon72 mentioned this pull request Jun 14, 2023

[BUG] Volume could be attach to multiple VMs / times by API calls harvester/harvester#4085

Open

khushboo-rancher requested changes Jun 14, 2023

View reviewed changes

lanfon72 force-pushed the e2e branch 2 times, most recently from d4c2a4f to 832af6d Compare July 3, 2023 12:07

* [e2e] restruct shell login

5402879

khushboo-rancher approved these changes Jul 18, 2023

View reviewed changes

khushboo-rancher merged commit eb68dcf into harvester:main Jul 18, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add test cases of volume hot-plug for VM #857

add test cases of volume hot-plug for VM #857

lanfon72 commented Jun 11, 2023 •

edited

Loading

lanfon72 commented Jun 12, 2023

albinsun Jun 13, 2023

lanfon72 Jun 13, 2023 •

edited

Loading

khushboo-rancher left a comment

khushboo-rancher Jun 14, 2023

lanfon72 Jun 15, 2023

khushboo-rancher Jun 21, 2023

lanfon72 Jun 21, 2023

khushboo-rancher Jun 14, 2023

lanfon72 Jun 15, 2023

khushboo-rancher Jun 21, 2023

lanfon72 Jun 21, 2023

khushboo-rancher Jun 22, 2023

lanfon72 Jun 22, 2023

khushboo-rancher Jun 14, 2023

khushboo-rancher left a comment


		yield vol_name, size

		code, data = api_client.volumes.delete(vol_name)

add test cases of volume hot-plug for VM #857

add test cases of volume hot-plug for VM #857

Conversation

lanfon72 commented Jun 11, 2023 • edited Loading

Changes

lanfon72 commented Jun 12, 2023

Choose a reason for hiding this comment

lanfon72 Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

khushboo-rancher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khushboo-rancher left a comment

Choose a reason for hiding this comment

lanfon72 commented Jun 11, 2023 •

edited

Loading

lanfon72 Jun 13, 2023 •

edited

Loading