[Freeipa-devel] [Test][Patch-0047] Added a test for Ticket N 5964
Martin Basti
mbasti at redhat.com
Thu Oct 13 16:10:22 UTC 2016
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.
>>>
>>
>
More information about the Freeipa-devel
mailing list