On 11/03/2016 04:56 PM, Oleg Fayans wrote:
Hi Martin,The commit message was updated with the correct ticket link Thanks for review! On 11/03/2016 04:22 PM, Martin Basti wrote:almost ACK, but the ticket in commit message is closed as invalid. So I'm quite puzzled now what to do. On 03.11.2016 13:28, Oleg Fayans wrote:ping for review On 10/19/2016 04:54 PM, Oleg Fayans wrote:Hi Martin, Thanks for the review. Fixed both issues.$ ipa-run-tests test_integration/test_topology.py -k TestCASpecificRUVsWARNING: Couldn't write lextab module 'pycparser.lextab'. [Errno 13] Permission denied: 'lextab.py' WARNING: yacc table file version is out of date WARNING: Couldn't create 'pycparser.yacctab'. [Errno 13] Permission denied: 'yacctab.py'====================================================================================test session starts=====================================================================================platform linux2 -- Python 2.7.11, pytest-2.9.2, py-1.4.31, pluggy-0.3.1 rootdir: /usr/lib/python2.7/site-packages/ipatests, inifile: pytest.iniplugins: sourceorder-0.5, multihost-1.0 collected 5 items test_integration/test_topology.py ..================================================================================2 passed in 2444.84 seconds=================================================================================On 10/17/2016 07:05 PM, Martin Basti wrote:1)you don't need to disable/enable dirsrv, just stop/start. Please removedisable/enable parts 2)tracebackself = <ipatests.test_integration.test_topology.TestCASpecificRUVs object at 0x7f6a502eec90> def test_delete_ruvs(self): """ http://www.freeipa.org/page/V4/Manage_replication_topology_4_4/ Test_Plan#Test_case:_clean-ruv_subcommand """ replica = self.replicas[0] master = self.master res1 = master.run_command(['ipa-replica-manage', 'list-ruv', '-p', master.config.dirman_password])"Certificate Server Replica Update Vectors" in res1), (assert(res1.stdout_text.count(replica.hostname) == 2 and"CA-specific RUVs are not displayed") E TypeError: argument of type 'SSHCommand' is not iterable test_integration/test_topology.py:215: TypeErrorentering PDB/usr/lib/python2.7/site-packages/ipatests/test_integration/test_topology.py(215)test_delete_ruvs()-> assert(res1.stdout_text.count(replica.hostname) == 2 and On 14.10.2016 11:36, Oleg Fayans wrote:Right you are! I am sorry. On 10/13/2016 06:10 PM, Martin Basti wrote:I think that you forgot to squash commits. Patch 47 doesn't apply On 13.10.2016 14:01, Oleg Fayans wrote:Hi Martin, Thanks for the review. With disabling directory server it works as well, thanks for the hint. Also I moved the cleanup logic to the test itself for the sake of simplicity. Patch-0048 was not changed On 10/12/2016 02:35 PM, Martin Basti wrote:1) Can you just turn off dirsrv on replica instead of doing iptables magic? 2) NACK No more eval() ever in code, use 'getattr', 'get' or whatever in the object that can be used. + evalhost = eval("args[0].%s" % host) Martin^2 On 12.10.2016 14:03, Oleg Fayans wrote:Hi Martin,After extensive discussion with Ludwig, I finally got the clue onhowdoes this feature work. When we uninstall the replica, the mastercleans the replication agreements with this replica and automatically cleans all replica's RUVs. If we clean replica's RUVs on master without uninstalling the replica, the replica's RUVs get recreated on master (replication works!). So, the only way to test the clean-ruv subcommand is to turn off the replica, or block the traffic on it so it gets inaccessible to updates from master. The testcases were updated, see [1] and [2] The updated versions of the patches are attached [1]http://www.freeipa.org/page/V4/Manage_replication_topology_4_4/Test_Plan#Test_case:_.2A-ruv_subcommands_of_ipa-replica-manage_are_extended_to_handle_CA-specific_RUVs[2]http://www.freeipa.org/page/V4/Manage_replication_topology_4_4/Test_Plan#Test_case:_clean-ruv_subcommandOn 08/05/2016 06:36 PM, Martin Basti wrote:On 03.08.2016 14:45, Oleg Fayans wrote:Hi Martin, Thanks for the review! Both patches were updated. On 07/28/2016 04:11 PM, Martin Basti wrote:On 08.07.2016 15:41, Oleg Fayans wrote:Hi Martin, Thanks for the review! On 07/08/2016 02:18 PM, Martin Basti wrote:On 27.06.2016 13:53, Oleg Fayans wrote:Hi guys, Is there a chance the patches NN 0047.1 and 0048.1 get reviewed before4.4 release? They cover a good part of the Managed Topology4.4 feature. On 06/17/2016 11:18 AM, Oleg Fayans wrote:One more test was added to the patch-0048 On 06/17/2016 09:43 AM, Oleg Fayans wrote:Fixed a bug in the previous patch, automated 2 more testcases fromhttp://www.freeipa.org/page/V4/Manage_replication_topology_4_4/Test_PlanOn 06/16/2016 04:46 PM, Oleg Fayans wrote:IIUC, this will turn off the machine completely, how is cleanup done then. AFAIK our tests cannot turn on machine again and run cleanup, so you will not be able to run more tests on the same topology without manual cleanup and manual start. + replica = self.replicas[0] + replica.run_command(['poweroff']) IMO would be better to just call 'ipactl stop' instead of 'poweroff'Agreed! Fixed.Martin^2*Automated ipa-replica-manage del tests* 1) + replica.run_command(['ipactl', 'stop']) + time.sleep(3) Why do you need sleep here?Removed, it was left from the old "poweroff" approach2) + ruvid_re = re.compile(".*%s:389: (\d+).*" % replica.hostname) + replica_ruvs = ruvid_re.findall(result.stdout_text) + master.run_command(['ipa-replica-manage', 'clean-ruv', 'f', + '-p', master.config.dirman_password, + replica_ruvs[0]])Because you are using re.findall(), without any match you willreceiveIndexError here replica_ruvs[0]. IMO it deserves assert beforeImplemented the assert which checks that the output contains enough replica RUVs3) assert(replica.hostname in result1.stdout_text) I think that this is error prone. What if there is just error 'could not connect to replica <replica hostname>', or something similar. instead of listing/cleaning/whatever operation was executed. I think that it should be more specific regexp than just finding a replica name substring (Yes In IPA we dont always print error so stderr) I'm not sure, but probably there might be cases when non critical error happen and exist status is still 0Agree. Implemented a regex-based search4) + replica.run_command(['poweroff']) + time.sleep(3) There should not be poweroff, probably sleep could be removed too.Gone* Automated clean-ruv subcommand test* 1) PEP8, 2 new lines expected ./ipatests/test_integration/test_topology.py:163:1: E302 expected 2 blank lines, found 0./ipatests/test_integration/test_topology.py:182:80: E501 linetoo long (85 > 79 characters)Fixed2) I dont like doing assert just with count of occurences of substring in STDOUT, would be possible to improve this somehow?Maybe, but frankly, I don't see how. In this case we are makingsure that both simple and CA-specific RUVs of a replica are displayed. The format of the output is strict: Replica Update Vectors: replica1_hostname:389: RUV_id replica2_hostname:389: RUV_id Certificate Server Replica Update Vectors: replica1_hostname:389: RUV_id replica2_hostname:389: RUV_id If we do not see 2 occurrences of the replica hostname than definitely something went wrong3) I'm not sure if clean-ruv is instant operations or there is some magic happening in background (we have abort-clean-ruv). Maybe some sleep should be there, but this needs investigation. + assert(replica.hostname in result2.stdout_text), ( + "The wrong RUV was deleted") + result3 = master.run_command(['ipa-replica-manage', 'list-ruv', + '-p', master.config.dirman_password]) + assert(result3.stdout_text.count(replica.hostname) == 1), ( + "CA RUV of the replica is still displayed")Based on my discussion with Stanislav Laznicka, I understood that by default clean-ruv does not return the shell until the operation is finished. You can force dropping into the shell by pressing CTRL+C, inwhich case the background job will still be running, but this isnot the default behaviorTest failed: result4 = master.run_command(['ipa-replica-manage', 'list-ruv', '-p', master.config.dirman_password])assert(replica.hostname not in result4.stdout_text), ("replica's RUV is still displayed") E AssertionError: replica's RUV is still displayed E assert 'replica3.ipa.test' not in 'Replica Update V...ipa.test:389: 8\n' E 'replica3.ipa.test' is contained here: E Replica Update Vectors: E \tmaster.ipa.test:389: 4 E \treplica3.ipa.test:389: 3 E \treplica2.ipa.test:389: 7 E Certificate Server Replica Update Vectors: E \tmaster.ipa.test:389: 6 E \treplica2.ipa.test:389: 8 [root@master ~]# ipa topologysegment-find Suffix name: domain ------------------ 2 segments matched ------------------ Segment name: master.ipa.test-to-replica2.ipa.test Left node: master.ipa.test Right node: replica2.ipa.test Connectivity: both Segment name: master.ipa.test-to-replica3.ipa.test Left node: master.ipa.test Right node: replica3.ipa.test Connectivity: both ---------------------------- Number of entries returned 2 ---------------------------- [root@master ~]# ipa-replica-manage list-ruv Directory Manager password: Replica Update Vectors: master.ipa.test:389: 4 replica2.ipa.test:389: 7 replica3.ipa.test:389: 3 Certificate Server Replica Update Vectors: master.ipa.test:389: 6 replica2.ipa.test:389: 8 [root@master ~]#Then I tried manually to clean RUV 3, and it behaves somehow odd[root@master ~]# 'ipa-replica-manage' 'clean-ruv' '3' '-p' 'Secret123' '-f' Clean the Replication Update Vector for replica3.ipa.test:389 Background task created to clean replication data. This may take a while. This may be safely interrupted with Ctrl+C Cleanup task created [root@master ~]# less /var/log/dirsrv/slapd-IPA-TEST/errors [root@master ~]# ipa-replica-manage list-ruv Directory Manager password: Replica Update Vectors: master.ipa.test:389: 4 replica2.ipa.test:389: 7 replica3.ipa.test:389: 3 Certificate Server Replica Update Vectors: master.ipa.test:389: 6 replica2.ipa.test:389: 8 [root@master ~]# 'ipa-replica-manage' 'clean-ruv' '3' '-p' 'Secret123' '-f' Clean the Replication Update Vector for replica3.ipa.test:389 CLEANALLRUV task for replica id 3 already exists. This may be safely interrupted with Ctrl+C Cleanup task created [root@master ~]# ipa-replica-manage list-clean-ruv -p Secret123 No CLEANALLRUV tasks running No abort CLEANALLRUV tasks running [root@master ~]# 'ipa-replica-manage' 'clean-ruv' '3' '-p' 'Secret123' '-f' Clean the Replication Update Vector for replica3.ipa.test:389 Background task created to clean replication data. This may take a while. This may be safely interrupted with Ctrl+C Cleanup task created [root@master ~]# ipa-replica-manage list-clean-ruv -p Secret123 CLEANALLRUV tasks RID 3: Successfully cleaned rid(3). No abort CLEANALLRUV tasks running [root@master ~]# ipa-replica-manage list-ruv -p Secret123 Replica Update Vectors: master.ipa.test:389: 4 replica2.ipa.test:389: 7 Certificate Server Replica Update Vectors: master.ipa.test:389: 6 replica2.ipa.test:389: 8 I'm not sure if this behavior is right, Ludwig may know.
ACK -- Milan Kubik
-- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code