Openwhisk: Consider automatic server-side retry on concurrent modification of assets during CRUD operations

[houshengbo]

https://github.com/apache/incubator-openwhisk/commit/21d37a1af329544e245d376ad613d1a603fc104c

This commit makes openwhisk unstable, including unstable travis build, concurrency issue raised in wskdeploy integration tests.
I have no idea about the detailed reason, but this commit makes Travis vulnerable.

[rabbah]

Can you clarify and add some details to make this actionable?

[houshengbo]

wskdeploy travis build works fine before this commit.
When I pin the openwhisk version to this commit, the travis failed:
https://travis-ci.org/apache/incubator-openwhisk-wskdeploy/builds/308707393

[mrutkows]

@rabbah we are seeing "concurrency" errors when trying to "undeploy" or delete resources. We have a series of integration tests that repeatedly deploy then undeploy packages/actions/triggers/rules and every Travis build (as of yesterday fails) with a moving concurrency error... I have documented some of the errors as they move to different int. tests in this issue https://github.com/apache/incubator-openwhisk-wskdeploy/issues/651

[mrutkows]

@houshengbo thanks for working to "pin" commits in our Travis to isolate where this issue was introduced.

[rabbah]

Kafka is not part of the CRUD operations. So I suspect it’s a red herring.

[mrutkows]

@houshengbo @rabbah so I was informed by Dave Grove that we discovered that we were not publishing Docker images for perhaps 2 weeks and that was fixed yesterday thereby bringing in whatever caused this issue to suddenly appear for us yesterday.

[rabbah]

@mrutkows I'll speculate based on your comment the issue is due to the HA controller enablement. CRUD operations are sprayed round robin and cache invalidation messages for CRUD operations are likely happening slower than your deploy/undeploy operations.

[mrutkows]

@rabbah Jeremias indicated he saw this once after merge, but could not recreate it.

[rabbah]

I suggest we remove the bug label until you have confirmation this is a bug. I believe the system is working as intended and you'll need to add some retries for eventual consistency in the tests.

[mrutkows]

@rabbah It is a bug IMO since these tests have worked before these changes and is an exposure to users who should not be expected to write special client-side logic because resource locking at the server side is taking longer than previously (when all we are doing effectively are creates followed by deletes). I will defer to you chosen labels, but I consider wskdeploy broken/blocked until we fix or revert.

[rabbah]

As I explained - we now have multiple controllers and CRUD operations are eventually consistent as a result. So you have to adapt the client accordingly. The controller never offered transactional semantics or guarantees on CRUD operations.

[mrutkows]

@rabbah actually MS Azure adapts at the server (server-side API retry 3x) on all APIs... even with eventual consistency within the DB, it is also not surfaced as effectively a failure to the user but is "ok" as the server/db "work it out" over time using timestamps and not losing the requested change to the target datastore.

[rabbah]

Re-titled and re-labelled accordingly.

[mrutkows]

@rabbah enhancement to return behavior to its previous state?

[houshengbo]

@rabbah The failure of the wskdeploy build is from just the deploy and undeploy's perspective.
You can take a look at the recent Travis builds for PRs in openwhisk. There could be no error, but there could be raised randomly each time.

For example, I just add a longer timeout for the test cases, but this is the travis result:
https://travis-ci.org/apache/incubator-openwhisk/builds/308993351

Even some merged PR raised error: https://travis-ci.org/apache/incubator-openwhisk/builds/308952327. It used to build fine as a PR.

The Travis build of openwhisk has already been unstable.
It will become a severe issue as more contribution comes to openwhisk, because random errors will be raised in every pr. It could be lucky there is no error, but there must be unlucky ones. We cannot count on that the build depends on luck.
@csantanapr @jeremiaswerner @markusthoemmes ... All the brains need to come here.

[rabbah]

This is a new feature - the client didn't observe this before because OpenWhisk was deployed with one active controller at a time. So while there were no transactional semantics guaranteed, you got to observe them for most operations (see here as an example this was always the case: https://github.com/apache/incubator-openwhisk/issues/1815; in fact could close one of these as duplicate). So I'm speculation that the combination of multiple active controllers, plus a clustered kafka exacerbates the issue.

[mrutkows]

@houshengbo in the meantime we likely need to disable all undeploys in our integration tests and create a plan for adding retries on all concurrency errors in every test on perhaps every CRUD operation.

[mrutkows]

Also, if this is a client-side/user strategy for this failure that is now surfaced, we will need a plan to document that they need to expect these errors and retry accordingly.

[mrutkows]

this is causing deja vu for me, saw this in MS Azure ~6 years ago

MS they made users retry (documented this) and then they built the retry into .NET client libraries and then fixed it at the server (retry). They were always able to see the issue of concurrency surfaced in the same consistent manner by their underlying object store which maintained state for all resources.

[rabbah]

A point to reiterate here is that the CRUD operations are round robin distributed to controllers. If there was some session (read namespace) stickiness, it would alleviate this problem. cc @cbickel

[rabbah]

@houshengbo confirmed by reproducing and inspecting the logs that the conflict is caused by requests being sprayed to two controllers:

1. rule disable to controller 0
2. rule delete to controller 1

both of these operations update the associated trigger document.

Thinking further about server side retry on a conflict is wrong and the client should resolve the conflict. The session stickiness will alleviate this problem a bit and since we have two issues open related to that I'm going to consolidate these issues (hence closing this is a duplicate of #3032 for now since it contains a more accurate represenation of the issue).

[mrutkows]

@rabbah thanks Rodric for your analysis and 3032 reference. Also, will not Rules exist within the general user namespace so expect more concurrency failures on these and triggers...