Reintroduce worker race condition fix from #98 that was lost during the migration #187

rtannenbaum · 2018-03-07T23:38:26Z

If we go with this approach, I put a placeholder in the Cosmos Store to implement this

…ation

holachuy

Good catch. Unlike the legacy JobManager, I think we may be able to keep the JobStore interface as is and free of biz logic per my comments below.

holachuy · 2018-03-08T04:04:44Z

...ility-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleJobStore.java

@@ -142,6 +118,11 @@ public void updateJob(UUID jobId, PortabilityJob job) throws IOException {
        throw new IOException("Could not find record for jobId: " + jobId);
      }

+      if (previousState != null) {
+        PortabilityJob previousJob = PortabilityJob.fromMap(getProperties(previousEntity));


For a future PR: Maybe if we json serialize the job, we won't have to go from entity -> map -> job object.

agreed, @jimmarino brought up the same thing in Slack while working on the Azure impl

added issue #191

holachuy · 2018-03-08T04:06:58Z

portability-worker/src/main/java/org/dataportabilityproject/worker/JobPollingService.java

+    // instance polled the same job, and already claimed it, it will have updated the job's state
+    // to CREDS_ENCRYPTION_KEY_GENERATED.
+    try {
+      store.updateJob(jobId, updatedJob, JobAuthorization.State.CREDS_AVAILABLE);


I think we can ensure here that the existingJob state is CREDS_AVAILABLE rather than pass it into the JobStore

Updated to use Validator

holachuy · 2018-03-08T04:07:51Z

portability-api/src/main/java/org/dataportabilityproject/webapp/StartCopyHandler.java

@@ -124,13 +124,13 @@ private void handleWorkerAssignmentFlow(HttpExchange exchange, UUID jobId) throw
    Preconditions.checkArgument(
        !Strings.isNullOrEmpty(importAuthCookieValue), "Import auth cookie required");

-    // We have the data, now update to 'pending worker assignment' so a worker may be assigned
+    // We have the data, now update to 'CREDS_AVAILABLE' so a worker may poll the job.


We also have the existing job here and can validate it was INITIAL before setting the new state.

Added TODO since this is the LegacyPortabilityJob impl

holachuy · 2018-03-08T04:09:08Z

portability-spi-cloud/src/main/java/org/dataportabilityproject/spi/cloud/storage/JobStore.java

+   * @throws IllegalStateException if state validation was requested ({@code previousState} was
+   * non-null) and the job's state was not the expected {@code previousState}
+   */
+  void updateJob(UUID jobId, PortabilityJob job, JobAuthorization.State previousState)


I'm weary of adding the third param simply for validation since this class mostly just cares about storage. Since we usually have the existing job object before updateJob is called, can we validate before the updateJob method is called rather than having it leak into the JobStore?

Agreed we should minimize business logic in here, but if we do the validation before, I think we'd still get the race condition:
Worker 1 polls job, validates state CREDS_AVAILABLE
Worker 2 polls job, validates state CREDS_AVAILABLE
Worker 1 updates with its public key
Worker 2 updates with its public key too
both workers start processing the same job

With the current approach, the race condition is handled b/c:
Worker 1 polls job
Worker 2 polls job
Worker 1 starts transaction
Worker 2 starts transaction - this will wait to look up job until previous transaction is closed
Worker 1 transaction: validate state CREDS_AVAILABLE, update with new state & public key - succeeds
Worker 2 transaction: validate state CREDS_AVAILABLE - fails, polls a new job

One way to avoid adding the new param, we could add a check for if you are setting public key, validate it was previously empty. That is still business logic inside this class but I think that's OK since it's a "Job Store" so can have some job-related logic.

The benefit to passing in the Authorization State is we can use that to validate other state changes like the comment you added about INITIAL -> CREDS_AVAILABLE. It would be slightly cleaner to pass a flag 'validateStateChange' that would validate the previous auth state. I don't know of any other race conditions now with state changes but would be good to prevent them in general.

The other approach I had a while ago was to expose the db transaction outside the jobstore and have the worker manage the transaction, but I think that would be messier.

WDYT?

Good description of the issue. Ok, let's move forward with this.

It's up to you if you want to do this now or later, but maybe we can pass in an JobUpdateValidator interface that the client can provide to wrap this logic.

void updateJob(UUID jobId, PortabilityJob job, JobUpdateValidator validator)

Something like
inteface JobUpdateValidator {
/** Validates before update. */
void validate(PortabilityJob previous, PortabilityJob updated);

Nice, I like that solution, I went with that

holachuy · 2018-03-08T04:09:32Z

portability-worker/src/main/java/org/dataportabilityproject/worker/JobPollingService.java

+    try {
+      store.updateJob(jobId, updatedJob, JobAuthorization.State.CREDS_AVAILABLE);
+    } catch (IllegalStateException e) {
+      throw new IOException("Could not 'claim' job " + jobId + ". It was probably already "


Good catch.

rtannenbaum

Addressed your comment about business logic in the job stores. Let me know what you think, thanks!

rtannenbaum · 2018-03-08T14:35:33Z

...ility-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleJobStore.java

@@ -142,6 +118,11 @@ public void updateJob(UUID jobId, PortabilityJob job) throws IOException {
        throw new IOException("Could not find record for jobId: " + jobId);
      }

+      if (previousState != null) {
+        PortabilityJob previousJob = PortabilityJob.fromMap(getProperties(previousEntity));


agreed, @jimmarino brought up the same thing in Slack while working on the Azure impl

rtannenbaum · 2018-03-08T14:54:05Z

portability-spi-cloud/src/main/java/org/dataportabilityproject/spi/cloud/storage/JobStore.java

+   * @throws IllegalStateException if state validation was requested ({@code previousState} was
+   * non-null) and the job's state was not the expected {@code previousState}
+   */
+  void updateJob(UUID jobId, PortabilityJob job, JobAuthorization.State previousState)


Agreed we should minimize business logic in here, but if we do the validation before, I think we'd still get the race condition:
Worker 1 polls job, validates state CREDS_AVAILABLE
Worker 2 polls job, validates state CREDS_AVAILABLE
Worker 1 updates with its public key
Worker 2 updates with its public key too
both workers start processing the same job

With the current approach, the race condition is handled b/c:
Worker 1 polls job
Worker 2 polls job
Worker 1 starts transaction
Worker 2 starts transaction - this will wait to look up job until previous transaction is closed
Worker 1 transaction: validate state CREDS_AVAILABLE, update with new state & public key - succeeds
Worker 2 transaction: validate state CREDS_AVAILABLE - fails, polls a new job

One way to avoid adding the new param, we could add a check for if you are setting public key, validate it was previously empty. That is still business logic inside this class but I think that's OK since it's a "Job Store" so can have some job-related logic.

The benefit to passing in the Authorization State is we can use that to validate other state changes like the comment you added about INITIAL -> CREDS_AVAILABLE. It would be slightly cleaner to pass a flag 'validateStateChange' that would validate the previous auth state. I don't know of any other race conditions now with state changes but would be good to prevent them in general.

The other approach I had a while ago was to expose the db transaction outside the jobstore and have the worker manage the transaction, but I think that would be messier.

WDYT?

jimmarino · 2018-03-08T17:37:27Z

I'll take a look at how to handle this in Cosmos DB. It claims to be transactional so it shold be OK.

holachuy · 2018-03-08T17:38:22Z

portability-spi-cloud/src/main/java/org/dataportabilityproject/spi/cloud/storage/JobStore.java

+     * Validation to do as part of an atomic update. Implementers should throw an
+     * {@code IllegalStateException} if the validation fails.
+     */
+    void validate(PortabilityJob previous, PortabilityJob updated);


So clients can see it in the contract of the method signature, let's explicitly declare throws IllegalStateException or a custom ValidationException

IllegalStateException is unchecked, and this is the same convention as Preconditions.checkState (i.e. that also documents @throws IllegalStateException but does not explicitly declare in method signature)

I think it's probably unrecoverable if validate fails, so should stick with an unchecked exception, depending on what we decide for #176. Can we revisit this with #176?

Sure, I'm thinking more about defining explicitly how a client handles exceptions to the validation to avoid a mishmash of inconsistent exception handling.

Reintroduce race condition fix from #98 that was lost during the migr…

ac5b796

…ation

rtannenbaum requested review from jimmarino and holachuy March 7, 2018 23:38

holachuy requested changes Mar 8, 2018

View reviewed changes

rtannenbaum commented Mar 8, 2018

View reviewed changes

holachuy approved these changes Mar 8, 2018

View reviewed changes

rtannenbaum mentioned this pull request Mar 8, 2018

Remove PortabilityJob.to/fromMap in favor of JSON serialization #191

Open

Pass in a JobUpdateValidator to JobStore.update

2fa7458

rtannenbaum force-pushed the worker-authstate branch from 5cb73c4 to 2fa7458 Compare March 8, 2018 17:22

Merge branch 'master' into worker-authstate

5d77c8c

rtannenbaum merged commit e6b1f15 into master Mar 8, 2018

rtannenbaum deleted the worker-authstate branch March 8, 2018 17:34

holachuy reviewed Mar 8, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce worker race condition fix from #98 that was lost during the migration #187

Reintroduce worker race condition fix from #98 that was lost during the migration #187

rtannenbaum commented Mar 7, 2018

holachuy left a comment

holachuy Mar 8, 2018

rtannenbaum Mar 8, 2018

rtannenbaum Mar 8, 2018

holachuy Mar 8, 2018

rtannenbaum Mar 8, 2018

holachuy Mar 8, 2018

rtannenbaum Mar 8, 2018

holachuy Mar 8, 2018

rtannenbaum Mar 8, 2018

holachuy Mar 8, 2018

rtannenbaum Mar 8, 2018

holachuy Mar 8, 2018

rtannenbaum left a comment

rtannenbaum Mar 8, 2018

rtannenbaum Mar 8, 2018

jimmarino commented Mar 8, 2018

holachuy Mar 8, 2018

rtannenbaum Mar 8, 2018

holachuy Mar 8, 2018

Reintroduce worker race condition fix from #98 that was lost during the migration #187

Reintroduce worker race condition fix from #98 that was lost during the migration #187

Conversation

rtannenbaum commented Mar 7, 2018

holachuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtannenbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmarino commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment