Overall framework and first buggify entries #1417

mpilman · 2019-04-04T00:41:58Z

This PR will add a special client side buggify. The overall idea is that a user can enable this client-side buggify to test client libraries and layers.

If enabled, the client will start throwing errors with a configurable probability. This way an application or layer implementer can test that her exception handling works correctly.

dongxinEric · 2019-04-04T21:02:00Z

fdbclient/NativeAPI.actor.cpp

 				throw invalid_option_value();
 			}
 			break;
+		case FDBNetworkOptions::CLIENTBUGGIFY_ENABLE:


Can we remove the CLIENTBUGGIFY_DISABLE option and simply default the CLIENTBUGGIFY_ENABLE to false? This is totally non-blocking though.

Good question - I don't know what makes more sense and I don't have strong opinions. My line of thinking was that you might want to be able to enable and disable client buggify multiple times during a single run. But that might be a rare enough use-case so that we might not want to have more options for this.

I think I'm personally ok with having both options, but if anybody disagrees we should have that discussion now rather than try to remove the option from our API later.

Anybody else want to weigh in? @alecgrieser @dongxinEric @alexmiller-apple etc.

apkar · 2019-04-05T16:21:13Z

@mpilman Thanks for working on it. I was thinking of doing something similar in Doc Layer transaction abstraction. But, this is much better and generic. I didn't do code review, but the concept looks good to me. Couple of comments

I see that you are trying to simulate errors in getRange() and commit(). Is there a reason we don't want to buggify clear() and get()?
Does it make sense to add artificial delays in addition to errors, so we can trigger transaction_timeout errors as well?

mpilman · 2019-04-05T16:25:06Z

Thanks, I'll continue with this then. To answer your question:

Yes there is a reason: I didn't finish yet ;) I just wanted to provide a draft PR to showcase the overall idea. I will add more failure cases.
That's a good idea, I will add some of those.

ajbeamon

Another thing we could buggify (though it needn't be part of this PR necessarily) is cluster upgrades via the multi-version client. The idea here would be to cause the multi-version client to "switch" from the current client to the same one while triggering all the usual behavior of failing outstanding requests, etc.

fdbclient/NativeAPI.actor.cpp

atn34 · 2019-04-05T17:02:27Z

I see that you are trying to simulate errors in getRange() and commit(). Is there a reason we don't want to buggify clear() and get()?

What kind of errors might clear throw? I think it's just adding a mutation to an in-memory buffer and adding a write conflict range.

apkar · 2019-04-05T17:05:40Z

I see that you are trying to simulate errors in getRange() and commit(). Is there a reason we don't want to buggify clear() and get()?

What kind of errors might clear throw? I think it's just adding a mutation to an in-memory buffer and adding a write conflict range.

Good point. clear is just like set it wouldn't throw any errors.

amouehsan

Thanks for working on this. This looks great to me. Just a couple of questions for you.

flow/flow.h

flow/flow.cpp

fdbclient/NativeAPI.actor.cpp

alecgrieser

I think some of these were discussed in real life, but here are my comments for completeness.

fdbclient/vexillographer/fdb.options

fdbclient/NativeAPI.actor.cpp

ajbeamon · 2019-05-23T23:21:56Z

Is there anything blocking this from being a regular, non-draft PR? If so, what is it?

documentation/sphinx/source/client-testing.rst

ajbeamon · 2019-06-17T15:38:06Z

fdbclient/NativeAPI.actor.cpp

+		case FDBNetworkOptions::CLIENT_BUGGIFY_SECTION_ACTIVATED_PROBABILITY:
+			validateOptionValue(value, true);
+			clearBuggifySections(BuggifyType::Client);
+			P_BUGGIFIED_SECTION_ACTIVATED[int(BuggifyType::Client)] = 100.0/double(extractIntOption(value, 0, 100));


Should this be value/100.0 rather than 100.0/value? The same question applies to CLIENT_BUGGIFY_SECTION_FIRED_PROBABILITY as well.

Oh yes, of course. This is an embarrassing one

fdbclient/NativeAPI.actor.cpp

ajbeamon · 2019-06-17T15:48:40Z

fdbclient/vexillographer/fdb.options

+            description="" />
+    <Option name="client_buggify_section_activated_probability" code="82"
+            paramType="Int" paramDescription="probability expressed as a percentage between 0 and 100"
+            description="Set the probability of a CLIENT_BUGGIFY section being active for the current execution.  Only applies to code paths first traversed AFTER this option is changed." />


Is this true? It looks like the buggified sections get cleared when this option is set. That leads me to another thought, which is that if it's the case that buggified sections can change during a run, we may want to note that somewhere in case someone tries to write a buggify statement that depends on the section being stable during a run.

oh right, this is not true anymore (it used to be but I later introduced the whole clear thingy as I thought it will make it easier to understand).

For your second thought: the normal BUGGIFY which we usually use should be stable, this only applies to CLIENT_BUGGIFY. But nonetheless it would be good to keep this in mind. Where do you want me to document this?

That's a good question, and I'm not sure there's any perfect place to put it. My best suggestion would be to note this as a comment on the macros CLIENT_BUGGIFY* in NativeAPI.actor.h.

I see the comment now, it looks good. Don't forget to also update the description of this option to reflect the new behavior of clearing activated sections.

Co-Authored-By: A.J. Beamon <[email protected]>

ajbeamon · 2019-06-17T17:11:14Z

I see that you are trying to simulate errors in getRange() and commit(). Is there a reason we don't want to buggify clear() and get()?

Does it make sense to add artificial delays in addition to errors, so we can trigger transaction_timeout errors as well?

I just noticed this previous comment, and you had mentioned subsequently that you would add some of these (though for clear we decided there was nothing to do). Was the plan for those changes to be included here or in a future PR that expands the feature?

mpilman · 2019-06-17T17:28:33Z

Good point - I missed the get - this is why I shouldn't keep my PRs open for this long :( I pushed another change

fdbclient/vexillographer/fdb.options

Co-Authored-By: A.J. Beamon <[email protected]>

dongxinEric reviewed Apr 4, 2019

View reviewed changes

ajbeamon reviewed Apr 5, 2019

View reviewed changes

fdbclient/NativeAPI.actor.cpp Show resolved Hide resolved

amouehsan reviewed Apr 5, 2019

View reviewed changes

flow/flow.h Outdated Show resolved Hide resolved

flow/flow.cpp Show resolved Hide resolved

fdbclient/NativeAPI.actor.cpp Show resolved Hide resolved

mpilman force-pushed the features/client-buggify branch from 775baf3 to 32585e5 Compare April 6, 2019 15:01

alecgrieser reviewed Apr 8, 2019

View reviewed changes

fdbclient/vexillographer/fdb.options Outdated Show resolved Hide resolved

fdbclient/NativeAPI.actor.cpp Show resolved Hide resolved

fdbclient/NativeAPI.actor.cpp Outdated Show resolved Hide resolved

Overall framework and first buggify entries

6ea7571

mpilman force-pushed the features/client-buggify branch from 32585e5 to 6ea7571 Compare June 16, 2019 16:10

mpilman marked this pull request as ready for review June 16, 2019 16:10

Address review comments and added documentation

7483cba

ajbeamon self-assigned this Jun 17, 2019

ajbeamon reviewed Jun 17, 2019

View reviewed changes

mpilman and others added 6 commits June 17, 2019 09:32

fixed grammatical mistake

2c105f6

Co-Authored-By: A.J. Beamon <[email protected]>

fixed typo

e5ef995

Co-Authored-By: A.J. Beamon <[email protected]>

Fix typo

130065b

Co-Authored-By: A.J. Beamon <[email protected]>

Fixed typo

8be7efa

Co-Authored-By: A.J. Beamon <[email protected]>

Fixed probability calculation

24ca369

Added proxy_memory_limit_exceeded to possible commit errors

5b7dc4e

Buggified get, added comment

978a5b2

Fixed buggify option comment

9aada56

ajbeamon reviewed Jun 18, 2019

View reviewed changes

fdbclient/vexillographer/fdb.options Outdated Show resolved Hide resolved

Update fdbclient/vexillographer/fdb.options

f7cbc7b

Co-Authored-By: A.J. Beamon <[email protected]>

ajbeamon approved these changes Jun 18, 2019

View reviewed changes

ajbeamon merged commit c3aa581 into apple:master Jun 18, 2019

Overall framework and first buggify entries #1417

Overall framework and first buggify entries #1417

Uh oh!

Conversation

mpilman commented Apr 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apkar commented Apr 5, 2019

Uh oh!

mpilman commented Apr 5, 2019

Uh oh!

ajbeamon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atn34 commented Apr 5, 2019

Uh oh!

apkar commented Apr 5, 2019

Uh oh!

amouehsan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alecgrieser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajbeamon commented May 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajbeamon Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajbeamon commented Jun 17, 2019

Uh oh!

mpilman commented Jun 17, 2019

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ajbeamon Jun 17, 2019 •

edited

Loading