HBASE-23085: Network and Data related Actions #675
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add monkey actions: - manipulate network packages with tc (reorder, loose,...) - add CPU load - fill the disk - corrupt or delete regionserver data files Create monkey factories for the new actions Extend HBaseClusterManager to allow sudo calls Fix a copy/paste issue wih monkey constants in some factories
|
🎊 +1 overall
This message was automatically generated. |
meszibalu
requested changes
Oct 7, 2019
hbase-it/src/test/java/org/apache/hadoop/hbase/HBaseClusterManager.java
Outdated
Show resolved
Hide resolved
hbase-it/src/test/java/org/apache/hadoop/hbase/HBaseClusterManager.java
Outdated
Show resolved
Hide resolved
hbase-it/src/test/java/org/apache/hadoop/hbase/HBaseClusterManager.java
Outdated
Show resolved
Hide resolved
hbase-it/src/test/java/org/apache/hadoop/hbase/chaos/actions/AddCPULoadAction.java
Outdated
Show resolved
Hide resolved
Comment on lines
+60
to
+61
| //This will always happen. We use timeout to kill a continously running process | ||
| //after the duration expires |
There was a problem hiding this comment.
I think we should add /bin/true at the end of the command line. We don't know at this point whether we got a network error for example or the script returned with error.
There was a problem hiding this comment.
@meszibalu As discussed for /bin/true to make a difference we should increase the outer timeout and that would not be much better so I will leave it as it is.
Comment on lines
+34
to
+35
| "seq 1 %s | xargs -I{} -n 1 -P %s timeout %s dd if=/dev/urandom of=/dev/null bs=1M " + | ||
| "iflag=fullblock"; |
There was a problem hiding this comment.
%s is used but numbers are added to the String.format arguments.
There was a problem hiding this comment.
That is intentional. %s uses Integer.toString, which is predictable while %d uses locale specific formatting that might change.
hbase-it/src/test/java/org/apache/hadoop/hbase/chaos/actions/CommandAction.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private String getCommand(String operation){ | ||
| return String.format("tc qdisc %s dev eth0 root netem corrupt %s%%", operation, ratio * 100); |
There was a problem hiding this comment.
Please format floats with %f.
There was a problem hiding this comment.
What would be the advantage? For a ration of 0.35f, the generated string in it's current form returns "35.0%", if I would replace it with "%f%%", the output would change to "35.000000%". Would you prefer a specific format, like round to 2 decimals?
There was a problem hiding this comment.
I don't understand it. It is possible to set the precision as well. If the floating point number is too small for example, Double.toString will return 0.53242e-17 for example. Is it also supported by tc? If you want to simply concatenate strings, String.format is not the fastest approach.
There was a problem hiding this comment.
@meszibalu As discussed it is not a performance sensitive part of the application and to conform to existing code I'll leave it as it is.
hbase-it/src/test/java/org/apache/hadoop/hbase/chaos/actions/DelayPackagesCommandAction.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private String getCommand(String operation){ | ||
| return String.format("tc qdisc %s dev eth0 root netem delay %sms reorder %s%% 50%", |
There was a problem hiding this comment.
Can we parameterize the name of the interface?
joshelser
requested changes
Oct 8, 2019
There was a problem hiding this comment.
Neat stuff, @BukrosSzabolcs !
What kind of cluster testing have you done with these so far?
hbase-it/src/test/java/org/apache/hadoop/hbase/chaos/actions/CommandAction.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public void perform() throws Exception { | ||
| if(clusterManager == null){ |
There was a problem hiding this comment.
Any reason to not have this in init()?
There was a problem hiding this comment.
This way if it runs on a non-distributed cluster it just does nothing. It might be better in case someone mixes them with older actions
| /** | ||
| * Action corrupts region server data. | ||
| */ | ||
| public class CorruptDataFilesAction extends Action { |
There was a problem hiding this comment.
Neat idea, but how could we tell via automation when a file was expectedly corrupted vs. unexpectedly?
There was a problem hiding this comment.
We can't as far as I'm aware. It's not clear to me what is the intended use of these tests, but they were requested by stack so I added them. They are so destructive I couldn't eve restart hbase after running them and had to delete every hbase related data from hdfs and zokeeper to be able to run hbase on the cluster again.
My best guess is to use them for active testing and run them in the background while monitoring hbase status/behavior.
There was a problem hiding this comment.
I was worried with corrupting something critical like hbase:meta, a table descriptor, or something like that. I think corrupting a single hfile for a user-table is a more "reasonable" failure condition which wouldn't have long-lasting impact on the ability for HBase to keep working.
@saintstack that jive with what you were thinking or you have something else in mind?
There was a problem hiding this comment.
Resolving this -- defaulting to $hbase.root/data/default is a nice compromise! Thanks for changing Szabolcs!
| } | ||
|
|
||
| private String getCommand(String operation){ | ||
| return String.format("tc qdisc %s dev eth0 root netem corrupt %s%%", operation, ratio * 100); |
There was a problem hiding this comment.
Probably need to allow the network device to be specified, both for predictable network interface names and bonds.
https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/
hbase-it/src/test/java/org/apache/hadoop/hbase/chaos/actions/DeleteDataFilesAction.java
Show resolved
Hide resolved
rename base class for new actions to better reflect it's role make network iterface configurable for tc commands fix typos and logging
|
💔 -1 overall
This message was automatically generated. |
joshelser
reviewed
Oct 9, 2019
...it/src/test/java/org/apache/hadoop/hbase/chaos/factories/DistributedIssuesMonkeyFactory.java
Outdated
Show resolved
Hide resolved
fix checkstyle
|
🎊 +1 overall
This message was automatically generated. |
joshelser
requested changes
Oct 14, 2019
There was a problem hiding this comment.
Trying to summarize:
- Javadoc needs to be updated (left comments) for the new network interface configuration (the change looks good otherwise! Thanks)
- Need feedback from @saintstack on how he was expecting HFile corruption to work. I would almost certainly expect he meant corruption on user table files only, but we should check.
- A couple of configs (network, disk fill size and timeout) would need to be tweaked by a user. Do we need docs somewhere to describe how these monkies are configured? I'm not sure how this gets wired up off the top of my head.
- Two questions/requests by Balazs -- not sure if these reflect work to do or if they just need an ACK from Balazs as to why you're not incorporating that change.
Thanks Szabolcs!
hbase-it/src/test/java/org/apache/hadoop/hbase/chaos/actions/CorruptPackagesCommandAction.java
Show resolved
Hide resolved
...e-it/src/test/java/org/apache/hadoop/hbase/chaos/actions/DuplicatePackagesCommandAction.java
Show resolved
Hide resolved
hbase-it/src/test/java/org/apache/hadoop/hbase/chaos/actions/LosePackagesCommandAction.java
Show resolved
Hide resolved
restrict file based monkeys to HFiles extend javadoc make sure new monkey properties are loaded from generic properties
|
Hi @joshelser
Thanks for the review! |
|
🎊 +1 overall
This message was automatically generated. |
joshelser
approved these changes
Nov 5, 2019
| /** | ||
| * Action corrupts region server data. | ||
| */ | ||
| public class CorruptDataFilesAction extends Action { |
There was a problem hiding this comment.
Resolving this -- defaulting to $hbase.root/data/default is a nice compromise! Thanks for changing Szabolcs!
meszibalu
approved these changes
Nov 6, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add monkey actions:
Create monkey factories for the new actions
Extend HBaseClusterManager to allow sudo calls
Fix a copy/paste issue wih monkey constants in some factories