Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-12-13
16:15:25 opendevreview yuval proposed openstack/nova master: Lightbits LightOS driver https://review.opendev.org/c/openstack/nova/+/821606
16:36:53 jhartkopf Hi there, I need another +2 on my patch: https://review.opendev.org/c/openstack/nova/+/813672 It's just a small fix. I'd appreciate if someone could take time for review.
16:46:40 gmann gibi: I got the problem, its tempest test also need to modify as per your change.
16:47:29 gibi gmann: what would be the good way to separate the AZ tests from the rest?
16:47:32 gmann gibi: test is trying to remove the host which has servers (bug you are fixing). you can see remove host request - https://zuul.opendev.org/t/openstack/build/5a9aad45498d45cda1eabd2b87b743d2/log/controller/logs/screen-n-api.txt#2268
16:47:41 gmann and delete server request is after that https://zuul.opendev.org/t/openstack/build/5a9aad45498d45cda1eabd2b87b743d2/log/controller/logs/screen-n-api.txt#2568
16:48:00 gmann gibi: I am fixing test to wait for server delete before it does remove host request
16:48:51 gmann there is no race condition here its just tests did not wait for server to be deleted before remove_host is called in cleanup
16:49:01 gibi gmann: OK, if that is in a single test case I can fix that up. But I think there are cases when we have two parallel tempest tests 1) does some AZ testing by moving host between aggregates 2) boot instance on host. If this two happens in parallel then the 1) will fail after the bugfix
16:50:10 gmann gibi: I can check those if there is any such case. I think that is only test we have to add/remove host to agg with server
16:50:39 gibi OK, I will gather the full picutre and fix up the cleanup issue
16:50:46 gmann gibi: let me update test and add your change as depends-on to see if all pass. and later we can merge the temest fix before nova change as per bug fix procedure
16:51:22 gibi gmann: sure, if you have time then I can appreciate the tempest fix
16:51:35 gmann those tempest test play on same host but we have only single tests doing server boot but I will check if we have any case like race condition you explained
16:51:37 gmann sure
16:53:32 gibi gmann: I think test_aggregate_basic_ops failure in https://zuul.opendev.org/t/openstack/build/83aad9cdd5b849d48f4ffb463880d259/logs is a race
16:53:41 gibi as that test case does not boot a server
16:53:51 gibi but it fails as probably other parallel test booted one
16:54:36 gmann gibi: ohk, did not see that in latest run. let me check
16:55:42 gmann we should not need lock here instead test should check host with AZ etc carefully
16:58:09 gibi gmann: so mean skip the AZ test if the host has instance?
16:58:21 gibi or wait in the test until the host has no instnace?
16:59:04 gmann gibi: skip like we do here https://github.com/openstack/tempest/blob/7e96c8e854386f43604ad098a6ec7606ee676145/tempest/api/compute/admin/test_aggregates.py#L228
16:59:48 gibi gmann: but would not that mean we sometimes run the test sometimes does not depending on which other parallel test runs at the same time
16:59:51 gibi ?
17:00:03 gibi so in theory we might lose test coverage by that
17:00:39 gmann gibi: i mean skip if there is no such host in env. test creating instance has to be cleaned up so we can add wait or so
17:02:23 gmann gibi: ah, we do have lock there but not sure why race is happening https://github.com/openstack/tempest/blob/master/tempest/scenario/test_aggregates_basic_ops.py#L119
17:02:58 gmann gibi: ohk but any other non agg test might be picking that host and creating server
17:03:09 gibi gmann: exactly ^^
17:03:25 gibi so the lock only avoid two AZ test to race
17:03:58 gmann yeah
17:04:34 gibi can we somehow make the AZ tests always run _after_ all the other tests?
17:04:42 gmann and skip if host has server also does not solve the issue as server can be created in between
17:05:37 gmann gibi: we can configure that way in our CI but as tempest run it is still bug that this test canot be run in parallal
17:06:59 sean-k-mooney we do run the senario test after all the others in serial
17:07:07 sean-k-mooney so we could put the az test in that group
17:07:13 sean-k-mooney at least for tempest-full
17:07:38 sean-k-mooney https://github.com/openstack/tempest/blob/master/tox.ini#L112-L113
17:07:54 sean-k-mooney we could exclude the az test the same way and run them with the senario and slow tests
17:08:22 sean-k-mooney its not the ideal solution but its an option
17:08:40 gmann gibi: one best we can do in tests 'check for host has no servers and then only add/remove tests' but that does not solve race completly
17:09:18 sean-k-mooney gmann: we donbt really want to make the dest depend on thing that are dynamic like that really
17:09:45 gmann gibi: or we can add try except and if conflict error due to server present then skip the test otherwise pass
17:09:59 gibi sean-k-mooney: you mean AZ test would be a 3rd group?
17:10:08 gibi to separate them from all the others
17:10:17 sean-k-mooney gibi: not entirly just put them with the senario tests
17:10:24 sean-k-mooney we could have them be a third group
17:10:27 gmann otherwise we have only choice of remvoe the test
17:10:35 sean-k-mooney but only if we could run them in paralle in that group
17:10:42 gmann sean-k-mooney: that does not solve the issue, it is just our CI fix
17:10:59 gmann anyone running tempest will face the same issue
17:11:01 sean-k-mooney gmann: right but the only way to solve the issue it so use a lock really
17:11:18 gibi sean-k-mooney: does the scenario / show runs in a single thread? if so we can add the AZ test there but if those also run in parallel then the same race can happen there too
17:11:26 gmann sean-k-mooney: we cannot use lock as it end up making all tests running serially
17:11:42 sean-k-mooney gibi: for tempest full its serical so one thread yes
17:11:43 gmann gibi: yeah that is happening now.
17:12:13 gmann scenario test also create server.
17:12:44 gibi yeah, locking means we basically need to lock agains any test that boots an instance, that is pretty restrictive.
17:12:51 gmann yeah
17:12:54 sean-k-mooney gibi: not quite
17:13:10 gmann it end up running tempest in serial
17:13:10 sean-k-mooney yes we do but the locking is slightly different
17:13:25 sean-k-mooney basiclly we need somihing like a reader writer lock
17:13:49 sean-k-mooney the aggreate tests need to grab the writer lock but the boot test need only a reader lock
17:14:18 gmann I see these are good option if we want to keep tests - 1. test will 'check for host has no servers and then only add/remove tests' 2. add try except and if conflict error due to server present then skip the test otherwise pass
17:14:34 sean-k-mooney so the build test can run in parallel with the read lock until a aggreate test try to modify thing in which case it grabes the writer lock
17:16:00 gmann we do have same kind of try except for image tests too for some cases, that is best tempest can test these APi race operations
17:16:27 gmann and which matches to the real operation scenario
17:16:54 sean-k-mooney perhaps but i still think a reader/writere lock would be a good fit here
17:17:43 gmann sean-k-mooney: but boot tests having reader lock will still create server on host as aggregate test having write lock. correct?
17:18:28 gmann if we want to isolate them the it has to be same lock
17:18:31 sean-k-mooney it depend on the sematics of the lcok. you can have it perfer reads so that you cant get the write lock until all readers relese
17:18:44 sean-k-mooney https://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock
17:19:16 gmann yeah which is same thing right. end up running in serial as per many current boot tests
17:19:40 sean-k-mooney well we have to run in serial
17:19:47 gmann let me try the above check and then we can see how it looks like
17:19:56 sean-k-mooney we can run in parallel only if aggreates are not being changed
17:20:15 sean-k-mooney if they are then only that test can happen as its global state
17:21:19 sean-k-mooney we actully need "Write-preferring RW lock" semantics if we were to use this correctly
17:22:13 gibi as we have a small amount of AZ test I still think separating them out to the end would be easier than addin a reader lock to each test that boots an instance
17:22:27 gibi of course if such separation is possible
17:22:38 sean-k-mooney gibi: yep it would be but it need to be done by teh caller of tempest
17:22:54 sean-k-mooney we can do it via tox with the regex support
17:23:01 sean-k-mooney but every client would have to do that
17:23:02 gibi sean-k-mooney: then we might need to invent someting inside tempest to order cases?
17:23:14 sean-k-mooney unless we add a new feature to tempest to tag things as "must be serial"
17:23:18 gibi yeah
17:23:20 gibi like that
17:23:34 sean-k-mooney if we could do that as a decorator that would be nice
17:23:50 gibi OK, so we have couple of options a) RW lock b) @serial decorator
17:23:51 sean-k-mooney i just dont know if we can. i suspect its posible
17:24:05 sean-k-mooney c) tox.ini for now
17:24:19 gibi yeah temporarily tox.ini but I don't really like that
17:24:39 gmann yeah we should not do that as other tempest user still will face the error
17:24:39 sean-k-mooney its what we have done for senario tests for years
17:24:45 sean-k-mooney it works but you have to know to do it
17:25:18 gibi I will sleep on this. I don't need to rush to land the nova bug fix so we have time
17:25:21 gmann sean-k-mooney: we did that only for env/timeout restriction they can be run in parallel like we do in tempest-parallel job
17:25:38 gibi thanks for the input

Earlier   Later