[Freeipa-devel] [Test][Patch-0047] Added a test for Ticket N 5964
Oleg Fayans
ofayans at redhat.com
Fri Oct 14 09:36:49 UTC 2016
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 on how
>>>> does this feature work. When we uninstall the replica, the master
>>>> cleans 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_subcommand
>>>>
>>>>
>>>>
>>>> On 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
>>>>>>>>>> before
>>>>>>>>>> 4.4 release? They cover a good part of the Managed Topology 4.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
>>>>>>>>>>>> from
>>>>>>>>>>>> http://www.freeipa.org/page/V4/Manage_replication_topology_4_4/Test_Plan
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 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" approach
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 2)
>>>>>>> + 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 will
>>>>>>> receive
>>>>>>> IndexError here replica_ruvs[0]. IMO it deserves assert before
>>>>>>
>>>>>> Implemented the assert which checks that the output contains enough
>>>>>> replica RUVs
>>>>>>
>>>>>>>
>>>>>>> 3)
>>>>>>> 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 0
>>>>>>
>>>>>> Agree. Implemented a regex-based search
>>>>>>
>>>>>>>
>>>>>>> 4)
>>>>>>>
>>>>>>> + 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 line too
>>>>>>> long
>>>>>>> (85 > 79 characters)
>>>>>>
>>>>>> Fixed
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 2)
>>>>>>> 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 making sure
>>>>>> 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 wrong
>>>>>>
>>>>>>>
>>>>>>> 3)
>>>>>>> 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, in
>>>>>> which case the background job will still be running, but this is not
>>>>>> the default behavior
>>>>>>
>>>>> Test 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 at 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 at 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 at master ~]#
>>>>>
>>>>> Then I tried manually to clean RUV 3, and it behaves somehow odd
>>>>>
>>>>> [root at 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 at master ~]# less /var/log/dirsrv/slapd-IPA-TEST/errors
>>>>> [root at 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 at 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 at master ~]# ipa-replica-manage list-clean-ruv -p Secret123
>>>>> No CLEANALLRUV tasks running
>>>>>
>>>>> No abort CLEANALLRUV tasks running
>>>>> [root at 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 at master ~]# ipa-replica-manage list-clean-ruv -p Secret123
>>>>> CLEANALLRUV tasks
>>>>> RID 3: Successfully cleaned rid(3).
>>>>>
>>>>> No abort CLEANALLRUV tasks running
>>>>> [root at 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.
>>>>
>>>
>>
>
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0047.5-Automated-clean-ruv-subcommand-tests.patch
Type: text/x-patch
Size: 4522 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20161014/ea13d1c3/attachment.bin>
More information about the Freeipa-devel
mailing list