-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add concurrency testing for storage operations #1132
base: mutations/mutations
Are you sure you want to change the base?
Changes from all commits
47a6ed6
49054c2
0e43aff
34f9190
b96c516
c6b7b10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
# This workflow performs interoperability tests across the supported runtimes of the MPL. | ||
name: Library Interoperability Tests | ||
|
||
on: | ||
workflow_call: | ||
inputs: | ||
dafny: | ||
description: "The Dafny version to run" | ||
required: true | ||
type: string | ||
regenerate-code: | ||
description: "Regenerate code using smithy-dafny" | ||
required: false | ||
default: false | ||
type: boolean | ||
|
||
jobs: | ||
generateEncryptVectors: | ||
strategy: | ||
matrix: | ||
library: [AwsCryptographicMaterialProviders] | ||
os: [ | ||
# https://taskei.amazon.dev/tasks/CrypTool-5283 | ||
# windows-latest, | ||
ubuntu-latest, | ||
macos-13, | ||
] | ||
language: [ | ||
java, | ||
# net, | ||
# python, | ||
# rust | ||
] | ||
# https://taskei.amazon.dev/tasks/CrypTool-5284 | ||
dotnet-version: ["6.0.x"] | ||
java-versions: [8, 11, 16, 17] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. non-blocking/comment: For my money, I would only test on Java 8 and Java 17. I do not think the subject of these tests changes behavior under different Java versions. |
||
runs-on: ${{ matrix.os }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. non-blocking/question: Does the subject of these tests change behavior across OS-es? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, copy-pasta |
||
permissions: | ||
id-token: write | ||
contents: read | ||
steps: | ||
- name: Support longpaths on Git checkout | ||
run: | | ||
git config --global core.longpaths true | ||
# Test Vectors need to call KMS | ||
- name: Configure AWS Credentials for Tests | ||
uses: aws-actions/configure-aws-credentials@v2 | ||
with: | ||
aws-region: us-west-2 | ||
role-to-assume: arn:aws:iam::370957321024:role/GitHub-CI-MPL-Dafny-Role-us-west-2 | ||
role-session-name: InterOpTests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: not InterOpTests but Concurrency tests, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup! just copy pasta-ying |
||
|
||
- uses: actions/checkout@v3 | ||
# Not all submodules are needed. | ||
# We manually pull the submodule we DO need. | ||
- run: git submodule update --init libraries | ||
- run: git submodule update --init --recursive smithy-dafny | ||
|
||
# Setup Java in Rust is needed for running polymorph | ||
- name: Setup Java 17 | ||
if: matrix.language == 'java' || matrix.language == 'rust' | ||
uses: actions/setup-java@v3 | ||
with: | ||
distribution: "corretto" | ||
java-version: 17 | ||
|
||
- name: Setup .NET Core SDK '6.0.x' | ||
uses: actions/setup-dotnet@v3 | ||
with: | ||
dotnet-version: "6.0.x" | ||
|
||
- name: Setup Dafny | ||
uses: dafny-lang/[email protected] | ||
with: | ||
dafny-version: ${{ inputs.dafny }} | ||
|
||
- name: Regenerate code using smithy-dafny if necessary | ||
if: ${{ inputs.regenerate-code }} | ||
uses: ./.github/actions/polymorph_codegen | ||
with: | ||
dafny: ${{ inputs.dafny }} | ||
library: ${{ matrix.library }} | ||
diff-generated-code: false | ||
|
||
# Build implementation for each runtime | ||
- name: Build ${{ matrix.library }} implementation in Java | ||
shell: bash | ||
working-directory: ./${{ matrix.library }} | ||
run: | | ||
# This works because `node` is installed by default on GHA runners | ||
CORES=$(node -e 'console.log(os.cpus().length)') | ||
make build_java CORES=$CORES | ||
- name: Setup gradle | ||
if: matrix.language == 'java' | ||
uses: gradle/gradle-build-action@v2 | ||
with: | ||
gradle-version: 7.2 | ||
|
||
- name: Setup Java ${{matrix.java-versions}} | ||
uses: actions/setup-java@v3 | ||
with: | ||
distribution: "corretto" | ||
java-version: ${{matrix.java-versions}} | ||
|
||
- name: Clean for next Java | ||
uses: gradle/gradle-build-action@v3 | ||
with: | ||
arguments: clean | ||
build-root-directory: ./${{ matrix.library }}/runtimes/java | ||
|
||
- name: Compile Java 8 | ||
uses: gradle/gradle-build-action@v3 | ||
with: | ||
arguments: build | ||
build-root-directory: ./${{ matrix.library }}/runtimes/java | ||
|
||
- name: Test Java 8 | ||
uses: gradle/gradle-build-action@v3 | ||
with: | ||
arguments: testConcurrentExamples | ||
build-root-directory: ./${{ matrix.library }}/runtimes/java |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -315,6 +315,30 @@ val testExamples = task<Test>("testExamples") { | |||||
testLogging { | ||||||
events("passed") | ||||||
} | ||||||
filter { | ||||||
excludeTestsMatching("software.amazon.cryptography.example.hierarchy.concurrent.*") | ||||||
} | ||||||
} | ||||||
|
||||||
val testConcurrentExamples = task<Test>("testConcurrentExamples") { | ||||||
description = "Runs examples tests." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
group = "verification" | ||||||
|
||||||
testClassesDirs = sourceSets["testExamples"].output.classesDirs | ||||||
classpath = sourceSets["testExamples"].runtimeClasspath + sourceSets["examples"].output + sourceSets.main.get().output | ||||||
// This will show System.out.println statements | ||||||
testLogging.showStandardStreams = true | ||||||
useTestNG() { | ||||||
suites("src/testExamples/java/software/amazon/cryptography/example/hierarchy/concurrent/testng-parallel.xml") | ||||||
maxParallelForks = 2 | ||||||
} | ||||||
|
||||||
testLogging { | ||||||
events("passed") | ||||||
} | ||||||
Comment on lines
+336
to
+338
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are running 1000 + 150 + more tests, you may not want to log every pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these concurrency tests are expensive and I don't want them to run every pr - if i have them run once a week, I don't see an issue in logging the passes. |
||||||
filter { | ||||||
includeTestsMatching("software.amazon.cryptography.example.hierarchy.concurrent.*") | ||||||
} | ||||||
} | ||||||
|
||||||
fun buildPom(mavenPublication: MavenPublication) { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,156 @@ | ||
package software.amazon.cryptography.example.hierarchy.concurrent; | ||
|
||
import java.text.DateFormat; | ||
import java.text.SimpleDateFormat; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.Date; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.TimeZone; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.concurrent.ConcurrentLinkedDeque; | ||
import org.testng.Assert; | ||
import org.testng.annotations.AfterClass; | ||
import org.testng.annotations.BeforeClass; | ||
import org.testng.annotations.Test; | ||
import software.amazon.awssdk.services.dynamodb.DynamoDbClient; | ||
import software.amazon.awssdk.services.dynamodb.model.AttributeValue; | ||
import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; | ||
import software.amazon.awssdk.services.dynamodb.model.TransactWriteItem; | ||
import software.amazon.awssdk.services.dynamodb.model.TransactWriteItemsRequest; | ||
import software.amazon.awssdk.services.dynamodb.model.TransactWriteItemsResponse; | ||
import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException; | ||
import software.amazon.awssdk.utils.ImmutableMap; | ||
import software.amazon.cryptography.example.Constants; | ||
import software.amazon.cryptography.example.DdbHelper; | ||
import software.amazon.cryptography.example.Fixtures; | ||
|
||
// These concurrent tests check that the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: probably need to finish this description. |
||
public class ConcurrentConditionCheckWriteTest { | ||
|
||
private static final Integer threadCount = 5; | ||
private static final String mLockedId = "concurrency-test-write-key"; | ||
private static final Map<String, String> INDEX_EXPR_ATT_NAMES = | ||
ImmutableMap.of("#pk", "branch-key-id"); | ||
|
||
private static final List<String> identifiers = Collections.unmodifiableList( | ||
Arrays.asList("1", "2", "3", "4", "5") | ||
); | ||
private Map<String, DynamoDbClient> threadIdToDdbClient; | ||
private static Map<String, String> indexToThreadId; | ||
private ConcurrentLinkedDeque<String> unpickedIndices; | ||
|
||
@BeforeClass | ||
public void setup() { | ||
threadIdToDdbClient = new ConcurrentHashMap<>(6, 1, threadCount); | ||
identifiers.forEach(id -> | ||
threadIdToDdbClient.put(id, DynamoDbClient.create()) | ||
); | ||
indexToThreadId = new ConcurrentHashMap<>(6, 1, threadCount); | ||
unpickedIndices = new ConcurrentLinkedDeque<>(identifiers); | ||
} | ||
|
||
@AfterClass | ||
public void teardown() { | ||
DynamoDbClient _ddbClient = DynamoDbClient.create(); | ||
identifiers.forEach(id -> | ||
DdbHelper.deleteKeyStoreDdbItem( | ||
mLockedId, | ||
"branch:ACTIVE", | ||
Fixtures.TEST_KEYSTORE_NAME, | ||
_ddbClient, | ||
true | ||
) | ||
); | ||
} | ||
|
||
public static Map<String, AttributeValue> indexItem( | ||
final AttributeValue value, | ||
final String timestamp | ||
) { | ||
Map<String, AttributeValue> item = new HashMap<>(); | ||
|
||
item.put("branch-key-id", AttributeValue.builder().s(mLockedId).build()); | ||
item.put("type", AttributeValue.builder().s(indexType()).build()); | ||
item.put("value", value); | ||
item.put("timestamp", AttributeValue.builder().s(timestamp).build()); | ||
return item; | ||
} | ||
|
||
private static String indexType() { | ||
return "branch:ACTIVE"; | ||
} | ||
|
||
public static TransactWriteItem conditionalWrite( | ||
final AttributeValue value, | ||
final String timestamp | ||
) { | ||
return TransactWriteItem | ||
.builder() | ||
.put(putBuilder -> | ||
putBuilder | ||
.tableName(Fixtures.TEST_KEYSTORE_NAME) | ||
.item(indexItem(value, timestamp)) | ||
.conditionExpression("attribute_not_exists(#pk)") | ||
.expressionAttributeNames(INDEX_EXPR_ATT_NAMES) | ||
) | ||
.build(); | ||
} | ||
|
||
private DynamoDbClient clientForThread(final String threadIdToIndex) { | ||
return threadIdToDdbClient.computeIfAbsent( | ||
threadIdToIndex, | ||
ddbClient -> DynamoDbClient.create() | ||
); | ||
} | ||
|
||
@Test(threadPoolSize = 5, invocationCount = 30, timeOut = 1000) | ||
public void TestConcurrentWriteCheck() { | ||
String threadId = String.valueOf(Thread.currentThread().getId()); | ||
String threadIdToIndex = indexToThreadId.computeIfAbsent( | ||
threadId, | ||
str -> unpickedIndices.pop() | ||
); | ||
AttributeValue value = AttributeValue.builder().s(threadIdToIndex).build(); | ||
TimeZone tz = TimeZone.getTimeZone("UTC"); | ||
DateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSSS'Z'"); // Quoted "Z" to indicate UTC, no timezone offset | ||
df.setTimeZone(tz); | ||
String timestamp = df.format(new Date()); | ||
|
||
System.out.println( | ||
"Thread ID: " + | ||
Thread.currentThread().getId() + | ||
" ThreadIndex: " + | ||
threadIdToIndex + | ||
" Timestamp: " + | ||
timestamp | ||
); | ||
|
||
try { | ||
DynamoDbClient client = clientForThread(threadIdToIndex); | ||
TransactWriteItemsResponse transactWriteItemsResponse = | ||
client.transactWriteItems( | ||
TransactWriteItemsRequest | ||
.builder() | ||
.transactItems(conditionalWrite(value, timestamp)) | ||
.build() | ||
); | ||
Assert.assertTrue( | ||
transactWriteItemsResponse.sdkHttpResponse().isSuccessful() | ||
); | ||
} catch (TransactionCanceledException exception) { | ||
// We can fail for two reasons, either there's already a transact write in flight | ||
// 0r we have failed the condition check. | ||
exception | ||
.cancellationReasons() | ||
.forEach(cancellationReason -> { | ||
Assert.assertTrue( | ||
(cancellationReason.code().equals("TransactionConflict") || | ||
cancellationReason.code().equals("ConditionalCheckFailed")) | ||
); | ||
}); | ||
} | ||
Comment on lines
+131
to
+154
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking/Question: I am confused.
But there is no guarantee that each of the instances will be thrown at least once during the 5 * 30 = 150 iterations. Is that an issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. After spending hours looking at the logs and adjusting the amount of threads that run this test, those exceptions will be thrown. There are enough threads trying to write, only one will win. Each thread runs the test x times. It is very likely that by the 2 or 3 time it tries to write the thread will submit a request that there is either a transactionconflict or that an item was already written and we now fail because the conditional check failed. Any other error we treat as a legit failure. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
[//]: # "Copyright Amazon.com Inc. or its affiliates. All Rights Reserved." | ||
[//]: # "SPDX-License-Identifier: CC-BY-SA-4.0" | ||
|
||
# AWS Cryptographic Material Providers Library Concurrency Testing Framework | ||
|
||
Welcome to the AWS Cryptographic Material Providers Library Concurrency and Parallelization | ||
Testing Framework 🎉! | ||
Comment on lines
+4
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: I would not call this a framework. I think these tests are valid, but not generic/global enough to be considered a Framework. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suite is correct, I just couldn't think of the word :) |
||
|
||
This framework helps set up scenarios that we would like to run in a parallel or multithreaded environment. | ||
|
||
Some things to keep in mind when you add tests. Think about how you will be creating resources per | ||
thread and what kind of state you need to keep between tests. | ||
|
||
Examples: | ||
|
||
- [Test regular DynamoDB Client TransactWrites](./ConcurrentConditionCheckWriteTest.java) | ||
- [Test ACTIVE branch key reads while branch key creation is inflight](./StorageWriteReadConcurrencyTests.java) | ||
- [Test branch key reads while branch key versioning is inflight](./StorageVersionReadConcurrencyTests.java) | ||
|
||
[Security issue notifications](./CONTRIBUTING.md#security-issue-notifications) | ||
|
||
## Security | ||
|
||
If you discover a potential security issue in this project | ||
we ask that you notify AWS/Amazon Security via our | ||
[vulnerability reporting page](http://aws.amazon.com/security/vulnerability-reporting/). | ||
Please **do not** create a public GitHub issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: We need to fix this up.
Or else the Job will run under the wrong name.