Dynamic Port Cfg - VS test #12
Conversation
853a422 to
3490918
Compare
Signed-off-by: tomeri <tomeri@nvidia.com>
3490918 to
2282fea
Compare
tests/test_port_add_remove.py
Outdated
| # get the number of ports before removal | ||
| num_of_ports = len(asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_PORT")) | ||
|
|
||
| for i in range(10): |
There was a problem hiding this comment.
10 times for adding and removing ports.
I wanted the test to remove and add ports in a loop, since i dont wat the test execution runtime will take too much, 10 times is enough.
There was a problem hiding this comment.
I suggest to create a named constant value
There was a problem hiding this comment.
will add a constant value
tests/test_port_add_remove.py
Outdated
| # verify that the port wasn't removed since we still have buffer cfg | ||
| assert len(num) == num_of_ports - 1 | ||
|
|
||
| time.sleep((i+1)%3) |
There was a problem hiding this comment.
What is the logic behind (i+1)%3 ? Same comment for L104
There was a problem hiding this comment.
I wanted the sleep time between removing and adding ports will be different in each iteration so I used this calculation
tests/test_port_add_remove.py
Outdated
| # verify that the port wasn't removed since we still have buffer cfg | ||
| assert len(num) == num_of_ports - 1 | ||
|
|
||
| time.sleep((i+1)%3) |
There was a problem hiding this comment.
Can we avoid sleep ? Same comment for L104
There was a problem hiding this comment.
we need sleep between removing and adding ports - it takes few seconds until all ports are removed/created
since we are using (i+1)%3 we have different timeout for each iteration.
different sleep timeout between add and remove ports can detect bugs, for example we found a bug on LLDP that was reproduced only on specific timeout values
There was a problem hiding this comment.
Is this test case designed to cover some real bug? VS does not have LLDP. How are you detecting those bugs? The only assert I see after sleep is the expectation on the number of SAI_OBJECT_TYPE_PORT objects. It will capture a situation when creation of the port fails in case there is specific sleep time interval after port deletion. Is it even a possible scenario? Can't we just assume that if we successfully deleted the port and the re-creation of the port succeded with 0 sec sleep in between it will work with any sleep interval? What is happening in the system during that sleep that can affect the test case result? The asic.db.wait_for_n_keys will sleep till the entry will appear - will it affect somehow your sleep interval logic?
There was a problem hiding this comment.
this test case is not covering a real bug, the lldp isue that I found was on real switch and it is just an example for race conditions that related to timeouts between delete and create.
I wanted to add several different timeouts between delete and create in order to cover such cases of race conditions that may occur between orchagent and syncd.
I guess that VS switch is not like the real switch and such cases of race conditions will probably be more relevant to real switch.
There was a problem hiding this comment.
From what I can tell from your comment is that these sleep's are redundant here as they are relevant for real switch and not for VS.
There was a problem hiding this comment.
I didn't say that, maybe I wasn't clear, I think that race condition can also be on VS switch (maybe on real switch it's more common), that's why the different sleep time in each iteration is relevant also on VS.
There was a problem hiding this comment.
But how are you catching this race condition, where the test fails when you hit it?
Signed-off-by: tomeri <tomeri@nvidia.com>
tests/test_port_add_remove.py
Outdated
|
|
||
| time.sleep(i%3) | ||
|
|
||
| def test_add_remove_all_the_ports(self, dvs, testlog): |
There was a problem hiding this comment.
This test case duplicated the code in test_add_remove_a_port - can we have a single test case that can be parametrized with the number of ports and then parametrize it with 1 port and N ports where N is the total number of ports in the system?
There was a problem hiding this comment.
you right, it's a kind of duplicate code, we can change it but I prefer to leave it as is, in different test cases, one of them is testing only one port and the second one is testing all the ports, I think it will be more readable.
tests/test_port_add_remove.py
Outdated
| port_info = config_db.get_entry("PORT", PORT) | ||
|
|
||
| # remove buffer pg cfg for the port | ||
| pgs = config_db.get_keys('BUFFER_PG') | ||
| for key in pgs: | ||
| if PORT in key: | ||
| config_db.delete_entry('BUFFER_PG', key) | ||
|
|
||
| # remove buffer queue cfg for the port | ||
| pgs = config_db.get_keys('BUFFER_QUEUE') | ||
| for key in pgs: | ||
| if PORT in key: | ||
| config_db.delete_entry('BUFFER_QUEUE', key) | ||
|
|
There was a problem hiding this comment.
Can we reuse part of the DVSPort remove_port ?
There was a problem hiding this comment.
I want to remove the buffer cfg dependencies without removing the port, the removal of the port will be inside the loop
There was a problem hiding this comment.
Can you create a function in DVSPort - remove_port_dependencies that will be used here and in remove_port function?
There was a problem hiding this comment.
The reason is that in case new dependencies added we need to modify test cases that do that instaed of modifying one function
There was a problem hiding this comment.
they are both using the same remove function:
self.dvs_port.remove_port(key) - for removing port dependencies and the port itself
between remove and add port I decided to add different sleep timeouts so a function that will remove and add ports needs to use the I variable inside and it will make the function a bit complex
So I prefer to leave it as is
Signed-off-by: tomeri <tomeri@nvidia.com>
Signed-off-by: tomeri <tomeri@nvidia.com>
Signed-off-by: tomeri tomeri@nvidia.com
What I did
added a vs test to check functionality of add/remove ports dynamically on run time
Why I did it
in order to test add/remove ports functionality
How I verified it
test passed
Details if related