From dff591f0eb81959e27337d743505532545fcc297 Mon Sep 17 00:00:00 2001 From: Simon Pichugin Date: Thu, 26 Nov 2015 16:55:52 +0100 Subject: [PATCH] Ticket 48360 - Refactor the delete agreement function Description: The delete agreement function of agreement.py module doesn't cover all usage varieties and should be rewritten. Now it requires suffix and replica(DirSrv object of the server that the agreement points to). By some reasons, this replica object may be missing, then we couldn't delete the agreement. We don't have test coverage for that function. It should be added to tests/agreement_test.py. Also test_changes fails, fix is required. Fix description: Change arguments of the delete agreement function to: - suffix - consumer_host - of the server that the agreement points to - consumer_port - of the server that the agreement points to With that, we can clearly define the required agreement. Add the delete test to tests/agreement_test.py Also fix the changes test by adding time.sleep(2) before change number checking, because there was not enough time for updating change number on master. https://fedorahosted.org/389/ticket/48360 Reviewed by: ? --- lib389/agreement.py | 38 +++++++++++++++++--------------------- tests/agreement_test.py | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 23 deletions(-) diff --git a/lib389/agreement.py b/lib389/agreement.py index 700f591..8cf009e 100644 --- a/lib389/agreement.py +++ b/lib389/agreement.py @@ -546,40 +546,36 @@ class Agreement(object): return dn_agreement - def delete(self, suffix, replica): - ''' - Delete a replication agreement + def delete(self, suffix, consumer_host, consumer_port): + """Delete a replication agreement @param suffix - the suffix that the agreement is configured for - @replica - DirSrv object of the server that the agreement points to + @param consumer_host - of the server that the agreement points to + @param consumer_port - of the server that the agreement points to + @raise ldap.LDAPError - for ldap operation failures - ''' + @raise TypeError - if too many agreements were found + @raise NoSuchEntryError - if no agreements were found + """ - if replica.sslport: - # We assume that if you are using SSL, you are using it with - # replication - port = replica.sslport - else: - port = replica.port + agmts = self.list(suffix, consumer_host, consumer_port) - agmts = self.list(suffix=suffix, consumer_host=replica.host, - consumer_port=port) if agmts: if len(agmts) > 1: - # Found too many agreements - self.log.error('Too many agreements found') - raise + raise TypeError('Too many agreements were found') else: - # delete the agreement + # Delete the agreement try: - self.conn.delete_s(agmts[0].dn) + agmt_dn = agmts[0].dn + self.conn.delete_s(agmt_dn) + self.log.info('Agreement (%s) was successfully removed' % + agmt_dn) except ldap.LDAPError as e: self.log.error('Failed to delete agreement (%s), ' + - 'error: %s' % (agmts[0].dn, str(e))) + 'error: %s' % (agmt_dn, str(e))) raise else: - # No agreements, error? - self.log.error('No agreements found') + raise NoSuchEntryError('No agreements were found') def init(self, suffix=None, consumer_host=None, consumer_port=None): """Trigger a total update of the consumer replica diff --git a/tests/agreement_test.py b/tests/agreement_test.py index 5b0af12..59e8f58 100644 --- a/tests/agreement_test.py +++ b/tests/agreement_test.py @@ -29,6 +29,7 @@ SERVERID_CONSUMER = 'consumer' SUFFIX = DEFAULT_SUFFIX ENTRY_DN = "cn=test_entry, %s" % SUFFIX +SECOND_AGMT_TEST_PORT = 12345 class TopologyReplication(object): @@ -152,14 +153,15 @@ def test_list(topology): str(topology.consumer.port) # Create a second RA to check .list returns 2 RA - properties = {RA_NAME: r'meTo_%s:%d' % (topology.consumer.host, 12345), + properties = {RA_NAME: r'meTo_%s:%d' % (topology.consumer.host, + SECOND_AGMT_TEST_PORT), RA_BINDDN: defaultProperties[REPLICATION_BIND_DN], RA_BINDPW: defaultProperties[REPLICATION_BIND_PW], RA_METHOD: defaultProperties[REPLICATION_BIND_METHOD], RA_TRANSPORT_PROT: defaultProperties[REPLICATION_TRANSPORT]} topology.master.agreement.create(suffix=SUFFIX, host=topology.consumer.host, - port=12345, + port=SECOND_AGMT_TEST_PORT, properties=properties) ents = topology.master.agreement.list(suffix=SUFFIX) assert len(ents) == 2 @@ -175,6 +177,29 @@ def test_list(topology): str(topology.consumer.port) +def test_delete(topology): + """Delete previously created the replica agreement + + PREREQUISITE: it exists a replica for SUFFIX and a replica agreement + with a SECOND_AGMT_TEST_PORT specified + """ + + topology.master.log.info("\n\n##############\n### DELETE\n##############\n") + ents = topology.master.agreement.list(suffix=SUFFIX, + consumer_host=topology.consumer.host, + consumer_port=SECOND_AGMT_TEST_PORT) + assert len(ents) == 1 + + topology.master.agreement.delete(suffix=SUFFIX, + consumer_host=topology.consumer.host, + consumer_port=SECOND_AGMT_TEST_PORT) + + ents = topology.master.agreement.list(suffix=SUFFIX, + consumer_host=topology.consumer.host, + consumer_port=SECOND_AGMT_TEST_PORT) + assert not ents + + def test_status(topology): """Test that status is returned from agreement""" @@ -311,6 +336,9 @@ def test_changes(topology): loop += 1 assert loop <= 10 + # Give a little time to update a change number on master + time.sleep(2) + # Check change number newvalue = topology.master.agreement.changes(agmnt_dn=ents[0].dn) topology.master.log.info("\ntest_changes: %d changes\n" % newvalue) -- 2.5.0