encryption: add test for temporary KMS service unavailable by hhwyt · Pull Request #17445 · tikv/tikv

hhwyt · 2024-08-27T10:08:16Z

This commit adds a new test to verify TiKV's recovery after temporary unavailability of the KMS service.

What is changed and how it works?

Issue Number: Ref #17410

What's Changed:

This commit adds a new test to verify TiKV's recovery after temporary unavailability of the KMS service.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Release note

no.

ti-chi-bot · 2024-08-27T10:08:28Z

Hi @hhwyt. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hhwyt · 2024-08-27T10:09:00Z

/pull-unit-test

hhwyt · 2024-08-27T10:10:02Z

/cc @glorv @LykxSassinator @overvenus

overvenus · 2024-08-27T10:17:18Z

components/encryption/src/master_key/kms.rs

Suggested change

let fail = (|| {

fail_point!("kms_api_timeout_decrypt", |val| val

.and_then(|x| x.parse::<bool>().ok())

.unwrap_or(false));

false

})();

if fail {

return Err(CloudError::ApiTimeout(box_err!("api timeout")));

}

(|| {

let fail = fail_point!("kms_api_timeout_decrypt", |val| val

.and_then(|x| x.parse::<bool>().ok())

.unwrap_or(false));

if fail {

Err(CloudError::ApiTimeout(box_err!("api timeout")))

} else {

Ok(())

}

})()?;

I refactored the fail_point part to the following simplified version. Let me know if you have any thoughts or suggestions!

fail_point!(fail_point_name, |val| { val.and_then(|x| x.parse::<bool>().ok()) .filter(|&fail| fail) .map(|_| Err(CloudError::ApiTimeout(box_err!("api timeout")))) .unwrap_or(Ok(())) });

overvenus · 2024-08-27T10:17:37Z

components/encryption/src/master_key/kms.rs

See above comments.

LykxSassinator

LGTM

ti-chi-bot · 2024-08-28T04:07:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LykxSassinator, overvenus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [LykxSassinator,overvenus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-08-28T04:07:45Z

[LGTM Timeline notifier]

Timeline:

2024-08-28 02:57:13.058067299 +0000 UTC m=+925428.192517415: ☑️ agreed by LykxSassinator.
2024-08-28 04:07:44.356115709 +0000 UTC m=+929659.490565831: ☑️ agreed by overvenus.

ref tikv#17410 This commit adds a new test to verify TiKV's recovery after temporary unavailability of the KMS service. Signed-off-by: hhwyt <[email protected]>

ti-chi-bot · 2024-08-28T04:13:53Z

@hhwyt: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

hhwyt · 2024-08-28T04:14:19Z

/pull-unit-test

hhwyt · 2024-08-28T04:14:48Z

/retest

ti-chi-bot · 2024-08-28T04:15:04Z

@hhwyt: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

glorv · 2024-08-28T05:42:09Z

/retest

ti-chi-bot · 2024-08-28T06:02:25Z

In response to a cherrypick label: new pull request created to branch release-6.5: #17452.

ref tikv#17410 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-08-28T06:03:02Z

In response to a cherrypick label: new pull request created to branch release-7.1: #17453.

ref tikv#17410 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-08-28T06:03:40Z

In response to a cherrypick label: new pull request created to branch release-7.5: #17454.

ti-chi-bot bot added needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 27, 2024

ti-chi-bot bot requested review from LykxSassinator, glorv and overvenus August 27, 2024 10:10

overvenus reviewed Aug 27, 2024

View reviewed changes

glorv removed needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. labels Aug 27, 2024

hhwyt force-pushed the master branch from 4d39eca to 472290f Compare August 27, 2024 12:42

LykxSassinator approved these changes Aug 28, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Aug 28, 2024

overvenus approved these changes Aug 28, 2024

View reviewed changes

ti-chi-bot bot added the lgtm label Aug 28, 2024

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 28, 2024

encryption: add test for temporary KMS service unavailable

27c55f6

ref tikv#17410 This commit adds a new test to verify TiKV's recovery after temporary unavailability of the KMS service. Signed-off-by: hhwyt <[email protected]>

hhwyt force-pushed the master branch from 472290f to 27c55f6 Compare August 28, 2024 04:13

Merge branch 'master' into master

ca99b78

ti-chi-bot bot merged commit 222e0a4 into tikv:master Aug 28, 2024

ti-chi-bot bot added this to the Pool milestone Aug 28, 2024

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Aug 28, 2024

This is an automated cherry-pick of tikv#17445

6e4ff71

ref tikv#17410 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 28, 2024

encryption: add test for temporary KMS service unavailable (#17445) #17452

Closed

9 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Aug 28, 2024

This is an automated cherry-pick of tikv#17445

901ea26

ref tikv#17410 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 28, 2024

encryption: add test for temporary KMS service unavailable (#17445) #17453

Closed

9 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Aug 28, 2024

This is an automated cherry-pick of tikv#17445

9d2e0ad

ref tikv#17410 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 28, 2024

encryption: add test for temporary KMS service unavailable (#17445) #17454

Closed

9 tasks

-            let fail = (|| {
-                fail_point!("kms_api_timeout_decrypt", |val| val
-                    .and_then(|x| x.parse::<bool>().ok())
-                    .unwrap_or(false));
-                false
-            })();
-            if fail {
-                return Err(CloudError::ApiTimeout(box_err!("api timeout")));
-            }
+            (|| {
+                let fail = fail_point!("kms_api_timeout_decrypt", |val| val
+                    .and_then(|x| x.parse::<bool>().ok())
+                    .unwrap_or(false));
+                if fail {
+                    Err(CloudError::ApiTimeout(box_err!("api timeout")))
+                } else {
+                    Ok(())
+                }
+            })()?;

Conversation

hhwyt commented Aug 27, 2024

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

ti-chi-bot bot commented Aug 27, 2024

Uh oh!

hhwyt commented Aug 27, 2024

Uh oh!

hhwyt commented Aug 27, 2024

Uh oh!

overvenus Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

hhwyt Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

overvenus Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

LykxSassinator left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Aug 28, 2024

Uh oh!

ti-chi-bot bot commented Aug 28, 2024

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot bot commented Aug 28, 2024

Uh oh!

hhwyt commented Aug 28, 2024

Uh oh!

hhwyt commented Aug 28, 2024

Uh oh!

ti-chi-bot bot commented Aug 28, 2024

Uh oh!

glorv commented Aug 28, 2024

Uh oh!

ti-chi-bot commented Aug 28, 2024

Uh oh!

ti-chi-bot commented Aug 28, 2024

Uh oh!

ti-chi-bot commented Aug 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants