diff --git a/core/src/main/java/google/registry/batch/RefreshDnsOnHostRenameAction.java b/core/src/main/java/google/registry/batch/RefreshDnsOnHostRenameAction.java index df3703a98..716e8e23e 100644 --- a/core/src/main/java/google/registry/batch/RefreshDnsOnHostRenameAction.java +++ b/core/src/main/java/google/registry/batch/RefreshDnsOnHostRenameAction.java @@ -23,6 +23,7 @@ import static google.registry.batch.AsyncTaskEnqueuer.PARAM_REQUESTED_TIME; import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_HOST_RENAME; import static google.registry.batch.AsyncTaskMetrics.OperationType.DNS_REFRESH; import static google.registry.mapreduce.inputs.EppResourceInputs.createEntityInput; +import static google.registry.model.EppResourceUtils.getLinkedDomainKeys; import static google.registry.model.EppResourceUtils.isActive; import static google.registry.model.EppResourceUtils.isDeleted; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -69,6 +70,7 @@ import java.util.logging.Level; import javax.annotation.Nullable; import javax.inject.Inject; import javax.inject.Named; +import org.apache.http.HttpStatus; import org.joda.time.DateTime; import org.joda.time.Duration; @@ -86,6 +88,8 @@ public class RefreshDnsOnHostRenameAction implements Runnable { @Inject Clock clock; @Inject MapreduceRunner mrRunner; @Inject @Named(QUEUE_ASYNC_HOST_RENAME) Queue pullQueue; + + @Inject DnsQueue dnsQueue; @Inject RequestStatusChecker requestStatusChecker; @Inject Response response; @Inject Retrier retrier; @@ -153,7 +157,39 @@ public class RefreshDnsOnHostRenameAction implements Runnable { } else { logger.atInfo().log( "Processing asynchronous DNS refresh for renamed hosts: %s", hostKeys.build()); - runMapreduce(refreshRequests, lock); + if (tm().isOfy()) { + runMapreduce(refreshRequests, lock); + } else { + try { + refreshRequests.stream() + .flatMap( + request -> + getLinkedDomainKeys(request.hostKey(), request.lastUpdateTime(), null) + .stream()) + .distinct() + .map(domainKey -> tm().transact(() -> tm().loadByKey(domainKey).getDomainName())) + .forEach( + domainName -> { + retrier.callWithRetry( + () -> dnsQueue.addDomainRefreshTask(domainName), + TransientFailureException.class); + logger.atInfo().log("Enqueued DNS refresh for domain %s.", domainName); + }); + deleteTasksWithRetry( + refreshRequests, + getQueue(QUEUE_ASYNC_HOST_RENAME), + asyncTaskMetrics, + retrier, + OperationResult.SUCCESS); + } catch (Throwable t) { + String message = "Error refreshing DNS on host rename."; + logger.atSevere().withCause(t).log(message); + response.setPayload(message); + response.setStatus(HttpStatus.SC_INTERNAL_SERVER_ERROR); + } finally { + lock.get().release(); + } + } } } diff --git a/core/src/main/java/google/registry/model/EppResourceUtils.java b/core/src/main/java/google/registry/model/EppResourceUtils.java index 9c5a09048..25ca49871 100644 --- a/core/src/main/java/google/registry/model/EppResourceUtils.java +++ b/core/src/main/java/google/registry/model/EppResourceUtils.java @@ -16,6 +16,7 @@ package google.registry.model; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static google.registry.model.ofy.ObjectifyService.auditedOfy; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -366,27 +367,26 @@ public final class EppResourceUtils { * * @param key the referent key * @param now the logical time of the check - * @param limit the maximum number of returned keys + * @param limit the maximum number of returned keys, unlimited if null */ public static ImmutableSet> getLinkedDomainKeys( - VKey key, DateTime now, int limit) { + VKey key, DateTime now, @Nullable Integer limit) { checkArgument( key.getKind().equals(ContactResource.class) || key.getKind().equals(HostResource.class), "key must be either VKey or VKey, but it is %s", key); boolean isContactKey = key.getKind().equals(ContactResource.class); if (tm().isOfy()) { - return ofy() - .load() - .type(DomainBase.class) - .filter(isContactKey ? "allContacts.contact" : "nsHosts", key.getOfyKey()) - .filter("deletionTime >", now) - .limit(limit) - .keys() - .list() - .stream() - .map(DomainBase::createVKey) - .collect(toImmutableSet()); + com.googlecode.objectify.cmd.Query query = + auditedOfy() + .load() + .type(DomainBase.class) + .filter(isContactKey ? "allContacts.contact" : "nsHosts", key.getOfyKey()) + .filter("deletionTime >", now); + if (limit != null) { + query.limit(limit); + } + return query.keys().list().stream().map(DomainBase::createVKey).collect(toImmutableSet()); } else { return tm().transact( () -> { @@ -405,11 +405,13 @@ public final class EppResourceUtils { .setParameter("fkRepoId", key.getSqlKey()) .setParameter("now", now.toDate()); } + if (limit != null) { + query.setMaxResults(limit); + } @SuppressWarnings("unchecked") ImmutableSet> domainBaseKeySet = (ImmutableSet>) query - .setMaxResults(limit) .getResultStream() .map( repoId -> diff --git a/core/src/test/java/google/registry/batch/RefreshDnsOnHostRenameActionTest.java b/core/src/test/java/google/registry/batch/RefreshDnsOnHostRenameActionTest.java index 33012a0fb..2970eb352 100644 --- a/core/src/test/java/google/registry/batch/RefreshDnsOnHostRenameActionTest.java +++ b/core/src/test/java/google/registry/batch/RefreshDnsOnHostRenameActionTest.java @@ -17,6 +17,8 @@ package google.registry.batch; import static com.google.appengine.api.taskqueue.QueueFactory.getQueue; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; +import static google.registry.batch.AsyncTaskEnqueuer.PARAM_HOST_KEY; +import static google.registry.batch.AsyncTaskEnqueuer.PARAM_REQUESTED_TIME; import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_HOST_RENAME; import static google.registry.batch.AsyncTaskMetrics.OperationType.DNS_REFRESH; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -45,6 +47,7 @@ import static org.mockito.Mockito.when; import com.googlecode.objectify.Key; import google.registry.batch.AsyncTaskMetrics.OperationResult; import google.registry.batch.RefreshDnsOnHostRenameAction.RefreshDnsOnHostRenameReducer; +import google.registry.dns.DnsQueue; import google.registry.model.host.HostResource; import google.registry.model.server.Lock; import google.registry.testing.DualDatabaseTest; @@ -55,6 +58,7 @@ import google.registry.testing.InjectExtension; import google.registry.testing.TaskQueueHelper.TaskMatcher; import google.registry.testing.TestOfyAndSql; import google.registry.testing.TestOfyOnly; +import google.registry.testing.TestSqlOnly; import google.registry.testing.mapreduce.MapreduceTestCase; import google.registry.util.AppEngineServiceUtils; import google.registry.util.RequestStatusChecker; @@ -62,6 +66,7 @@ import google.registry.util.Retrier; import google.registry.util.Sleeper; import google.registry.util.SystemSleeper; import java.util.Optional; +import org.apache.http.HttpStatus; import org.joda.time.DateTime; import org.joda.time.Duration; import org.junit.jupiter.api.BeforeEach; @@ -94,6 +99,7 @@ public class RefreshDnsOnHostRenameActionTest action.clock = clock; action.mrRunner = makeDefaultRunner(); action.pullQueue = getQueue(QUEUE_ASYNC_HOST_RENAME); + action.dnsQueue = DnsQueue.createForTesting(clock); action.requestStatusChecker = requestStatusChecker; action.response = fakeResponse; action.retrier = new Retrier(new FakeSleeper(clock), 1); @@ -102,7 +108,7 @@ public class RefreshDnsOnHostRenameActionTest .thenThrow(new AssertionError("Should not be called")); } - private void runMapreduce() throws Exception { + private void runAction() throws Exception { clock.advanceOneMilli(); // Use hard sleeps to ensure that the tasks are enqueued properly and will be leased. Sleeper sleeper = new SystemSleeper(); @@ -123,10 +129,32 @@ public class RefreshDnsOnHostRenameActionTest tm().clearSessionCache(); } - // TODO(b/181662306) None of the map reduce tests work with SQL since our map-reduce setup is - // inherently Datastore oriented, but this is a bigger task. + @TestSqlOnly + void testFailure_dnsUpdateEnqueueFailed() throws Exception { + HostResource host = persistActiveHost("ns1.example.tld"); + persistResource(newDomainBase("example.tld", host)); + persistResource(newDomainBase("otherexample.tld", host)); + persistResource(newDomainBase("untouched.tld", persistActiveHost("ns2.example.tld"))); + DateTime timeEnqueued = clock.nowUtc(); + enqueuer.enqueueAsyncDnsRefresh(host, timeEnqueued); + DnsQueue mockedQueue = mock(DnsQueue.class); + action.dnsQueue = mockedQueue; + when(mockedQueue.addDomainRefreshTask(anyString())) + .thenThrow(new RuntimeException("Cannot enqueue task.")); + runAction(); + assertNoDnsTasksEnqueued(); + assertTasksEnqueued( + QUEUE_ASYNC_HOST_RENAME, + new TaskMatcher() + .param(PARAM_HOST_KEY, host.createVKey().getOfyKey().getString()) + .param(PARAM_REQUESTED_TIME, timeEnqueued.toString())); + verify(action.asyncTaskMetrics).recordDnsRefreshBatchSize(1L); + verifyNoMoreInteractions(action.asyncTaskMetrics); + assertThat(fakeResponse.getStatus()).isEqualTo(HttpStatus.SC_INTERNAL_SERVER_ERROR); + assertThat(acquireLock()).isPresent(); + } - @TestOfyOnly + @TestOfyAndSql void testSuccess_dnsUpdateEnqueued() throws Exception { HostResource host = persistActiveHost("ns1.example.tld"); persistResource(newDomainBase("example.tld", host)); @@ -134,21 +162,22 @@ public class RefreshDnsOnHostRenameActionTest persistResource(newDomainBase("untouched.tld", persistActiveHost("ns2.example.tld"))); DateTime timeEnqueued = clock.nowUtc(); enqueuer.enqueueAsyncDnsRefresh(host, timeEnqueued); - runMapreduce(); + runAction(); assertDnsTasksEnqueued("example.tld", "otherexample.tld"); assertNoTasksEnqueued(QUEUE_ASYNC_HOST_RENAME); verify(action.asyncTaskMetrics).recordDnsRefreshBatchSize(1L); verify(action.asyncTaskMetrics) .recordAsyncFlowResult(DNS_REFRESH, OperationResult.SUCCESS, timeEnqueued); verifyNoMoreInteractions(action.asyncTaskMetrics); + assertThat(acquireLock()).isPresent(); } - @TestOfyOnly + @TestOfyAndSql void testSuccess_multipleHostsProcessedInBatch() throws Exception { HostResource host1 = persistActiveHost("ns1.example.tld"); HostResource host2 = persistActiveHost("ns2.example.tld"); HostResource host3 = persistActiveHost("ns3.example.tld"); - persistResource(newDomainBase("example1.tld", host1)); + persistResource(newDomainBase("example1.tld", host1, host2)); persistResource(newDomainBase("example2.tld", host2)); persistResource(newDomainBase("example3.tld", host3)); DateTime timeEnqueued = clock.nowUtc(); @@ -156,7 +185,7 @@ public class RefreshDnsOnHostRenameActionTest enqueuer.enqueueAsyncDnsRefresh(host1, timeEnqueued); enqueuer.enqueueAsyncDnsRefresh(host2, timeEnqueued); enqueuer.enqueueAsyncDnsRefresh(host3, laterTimeEnqueued); - runMapreduce(); + runAction(); assertDnsTasksEnqueued("example1.tld", "example2.tld", "example3.tld"); assertNoTasksEnqueued(QUEUE_ASYNC_HOST_RENAME); verify(action.asyncTaskMetrics).recordDnsRefreshBatchSize(3L); @@ -165,21 +194,23 @@ public class RefreshDnsOnHostRenameActionTest verify(action.asyncTaskMetrics) .recordAsyncFlowResult(DNS_REFRESH, OperationResult.SUCCESS, laterTimeEnqueued); verifyNoMoreInteractions(action.asyncTaskMetrics); + assertThat(acquireLock()).isPresent(); } - @TestOfyOnly + @TestOfyAndSql void testSuccess_deletedHost_doesntTriggerDnsRefresh() throws Exception { HostResource host = persistDeletedHost("ns11.fakesss.tld", clock.nowUtc().minusDays(4)); persistResource(newDomainBase("example1.tld", host)); DateTime timeEnqueued = clock.nowUtc(); enqueuer.enqueueAsyncDnsRefresh(host, timeEnqueued); - runMapreduce(); + runAction(); assertNoDnsTasksEnqueued(); assertNoTasksEnqueued(QUEUE_ASYNC_HOST_RENAME); verify(action.asyncTaskMetrics).recordDnsRefreshBatchSize(1L); verify(action.asyncTaskMetrics) .recordAsyncFlowResult(DNS_REFRESH, OperationResult.STALE, timeEnqueued); verifyNoMoreInteractions(action.asyncTaskMetrics); + assertThat(acquireLock()).isPresent(); } @TestOfyAndSql @@ -191,9 +222,10 @@ public class RefreshDnsOnHostRenameActionTest .setDeletionTime(START_OF_TIME) .build()); enqueuer.enqueueAsyncDnsRefresh(renamedHost, clock.nowUtc()); - runMapreduce(); + runAction(); assertNoDnsTasksEnqueued(); assertNoTasksEnqueued(QUEUE_ASYNC_HOST_RENAME); + assertThat(acquireLock()).isPresent(); } @TestOfyAndSql @@ -217,9 +249,10 @@ public class RefreshDnsOnHostRenameActionTest enqueueMapreduceOnly(); assertThat(fakeResponse.getPayload()).isEqualTo("Can't acquire lock; aborting."); assertNoDnsTasksEnqueued(); + assertThat(acquireLock()).isEmpty(); } - @TestOfyAndSql + @TestOfyOnly void test_mapreduceHasWorkToDo_lockIsAcquired() { HostResource host = persistActiveHost("ns1.example.tld"); enqueuer.enqueueAsyncDnsRefresh(host, clock.nowUtc()); diff --git a/core/src/test/java/google/registry/testing/DatabaseHelper.java b/core/src/test/java/google/registry/testing/DatabaseHelper.java index 6d0e64fc8..6307838cb 100644 --- a/core/src/test/java/google/registry/testing/DatabaseHelper.java +++ b/core/src/test/java/google/registry/testing/DatabaseHelper.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Suppliers.memoize; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Iterables.toArray; import static com.google.common.collect.MoreCollectors.onlyElement; import static com.google.common.truth.Truth.assertThat; @@ -110,6 +111,7 @@ import google.registry.model.transfer.TransferStatus; import google.registry.persistence.VKey; import google.registry.schema.tld.PremiumListDao; import google.registry.tmch.LordnTaskUtils; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; @@ -182,11 +184,10 @@ public class DatabaseHelper { domainName, generateNewDomainRoid(getTldFromDomainName(domainName)), contact); } - public static DomainBase newDomainBase(String domainName, HostResource host) { - return newDomainBase(domainName) - .asBuilder() - .setNameservers(ImmutableSet.of(host.createVKey())) - .build(); + public static DomainBase newDomainBase(String domainName, HostResource... hosts) { + ImmutableSet> hostKeys = + Arrays.stream(hosts).map(HostResource::createVKey).collect(toImmutableSet()); + return newDomainBase(domainName).asBuilder().setNameservers(hostKeys).build(); } public static DomainBase newDomainBase(