Verify broker can take backup #10374

deepthidevaki · 2022-09-15T15:15:36Z

Description

Add a simple test to verify broker can take backup.
Fixed several minor bugs that were found with this test.

This PR only adds a basic test to verify backup. More scenarios should be tested later.

Also added a Junit5 extension to replace ClusteringRule. Now it is a wrapper around ClusteringRule. Once all tests have been migrated to junit5, we would be able to replace it.

Related issues

closes #10262

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually
The change has been verified by a QA run
The impact of the changes is verified by a benchmark

Documentation:

The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
New content is added to the release announcement
If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

github-actions · 2022-09-15T15:32:19Z

Test Results

  859 files +  1   859 suites +1 1h 52m 39s ⏱️ + 4m 17s
6 697 tests +65 6 686 ✔️ +65 11 💤 ±0 0 ❌ ±0
6 881 runs +65 6 870 ✔️ +65 11 💤 ±0 0 ❌ ±0

Results for commit bcc0b62. ± Comparison against base commit 878cf00.

♻️ This comment has been updated with latest results.

megglos · 2022-09-16T07:44:19Z

This PR only adds a basic test to verify backup. More scenarios should be tested later.

do we have an issue for extended tests already?

megglos

LGTM!
❓ I had a quick look as I was curious if the localstack testcontainer offers any utility to verify certain calls have happened but I doesn't seem like it does? Thus we have to rely on assuming that if nothing failed everything was fine?

megglos · 2022-09-16T07:42:44Z

qa/integration-tests/src/test/java/io/camunda/zeebe/it/backup/BackupIT.java

+
+  private BackupStatus getBackupStatus(final long backupId)
+      throws InterruptedException, ExecutionException, TimeoutException {
+    // TODO: This should be replaced by the Gateway rest api when it is available


❓ how do we usually make such such TODOs are not overlooked? is this listed in the corresponding issue for the gateway rest api?

There is no specific policy. Adding an integration test will be done as part of the issue, so who ever works on this will see this comment.

megglos · 2022-09-16T07:48:15Z

qa/integration-tests/src/test/java/io/camunda/zeebe/it/backup/BackupIT.java

+    // then
+    Awaitility.await("Backup must be completed.")
+        .timeout(Duration.ofMinutes(2))
+        .ignoreExceptions()


🔧 what exceptions can occur which we want to ignore? I tried without and didn't hit any, maybe we can remove it to be aware of any unexpected failure of the getStatus call?

Here we don't want to fail due to exceptions, because sometimes the request can timeout and we want to to just retry. With ignoreExceptions, awaitility will retry. Otherwise, it fails immediately resulting in flaky tests. But it will eventually fail, if nothing good happens.

ok, I guess adding the specific timeout exception would be best to not hide potential other failure cases but in case it eventually resolves anyway I think it's acceptable.

megglos · 2022-09-16T07:53:08Z

❌ We need to mark this to be ignored?

[INFO] --- maven-dependency-plugin:3.3.0:analyze-only (analyze-dependencies) @ zeebe-qa-integration-tests ---
Warning:  Unused declared dependencies found:
Warning:     com.amazonaws:aws-java-sdk-core:jar:1.12.304:test

https://maven.apache.org/plugins/maven-dependency-plugin/analyze-mojo.html#ignoredDependencies

deepthidevaki · 2022-09-16T08:04:42Z

question I had a quick look as I was curious if the localstack testcontainer offers any utility to verify certain calls have happened but I doesn't seem like it does? Thus we have to rely on assuming that if nothing failed everything was fine?

Ya. Since this is integration test, we don't have to verify all the details. There are other tests for S3BackupStore that verified it is actually writing stuff to the backend, So here we can just assume if it gets a status backup it is correct. When we extend these tests, we could also verify that we can restore from this backup. That would be then the actual verification that the backup works.

deepthidevaki · 2022-09-16T08:16:03Z

This PR only adds a basic test to verify backup. More scenarios should be tested later.

do we have an issue for extended tests already?

I haven't created an issue yet. But it is one of the tasks in the epic #9606

deepthidevaki · 2022-09-16T08:48:32Z

bors merge

10332: deps(maven): bump rest-assured from 5.1.1 to 5.2.0 r=npepinpe a=dependabot[bot] Bumps [rest-assured](https://github.com/rest-assured/rest-assured) from 5.1.1 to 5.2.0. <details> <summary>Changelog</summary> Sourced from <a href="https://github.com/rest-assured/rest-assured/blob/master/changelog.txt">rest-assured's changelog</a>. <blockquote> <h2>Changelog 5.2.0 (2022-09-09)</h2> <ul> <li> Improved FilterContext used in Filters by adding the method FilterContext#hasValue(name, object). This makes it easier to check if a value exists and is equal to the expect object. </li> <li> Introducing a much improved CSRF (cross-site request forgery) support. For example: given(). csrf("/users"). formParm("firstName", "John"). formParm("lastName", "Doe"). when(). post("/users"). then(). statusCode(200); This will first make a GET request to /users (due to csrf("/users")) to get an HTML page that contains the CSRF token. Rest Assured will then automatically try to find the input field that contains the CSRF token and include in the POST to /users. Here's an example of what Rest Assured expects as a response for the GET request to /users:  </li> <li> Fixed so that form authentication takes CSRF into account. The previous form authentication CSRF implementation didn't really work (sorry!). Now you can combine csrf with form authentication and it actually works as expected! Note that for requests other than GET or HEAD, you need to specify both form authentication and csrf, e.g. given(). csrf("/users"). formParm("firstName", "John"). formParm("lastName", "Doe"). </li> </ul>  </blockquote> ... (truncated) </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/rest-assured/rest-assured/commit/c8e4ca53e4e0bc3ee761b5dbb07100b5c9fd74ae"><code>c8e4ca5</code></a> [maven-release-plugin] prepare release rest-assured-5.2.0</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/d72c03e0478d9b5de64aedba8ca87a5ad08c1e35"><code>d72c03e</code></a> [ci skip] Updated changelog with release date</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/0db04cb508568cede7914cb88e09cf8d25f6bddb"><code>0db04cb</code></a> Fixed imports</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/0cda32928e18acc88aceeac902227048f3378881"><code>0cda329</code></a> Fixed failing test</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/34c857a22276b823bb2805a1f04aad38d638c6cc"><code>34c857a</code></a> Moving sundr-maven-plugin to release profile since it doesn't seem to work on...</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/42d45528ddfad4e77a7aec4028fc6ce9ccdf388b"><code>42d4552</code></a> Cleanup</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/4c207e887971cbd894884374aca3c361bc43edb7"><code>4c207e8</code></a> Upgrading sundr-maven-plugin to 0.93.0</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/246ba8740607e23f00097e04526a019685f22627"><code>246ba87</code></a> Added csrf to RequestSpecBuilder.java</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/a6216b75f9f05e1cb2e5d4b747999af3bb663ac8"><code>a6216b7</code></a> Fixed some bugs and made some improvements to CSRF</li> <li><a href="https://github.com/rest-assured/rest-assured/commit/50ad97a8f415c02f1f818521d243c0097a5075a2"><code>50ad97a</code></a> Upgraded kotlin module to using Kotlin 1.7.10 (previously 1.6.21 was used)</li> <li>Additional commits viewable in <a href="https://github.com/rest-assured/rest-assured/compare/rest-assured-5.1.1...rest-assured-5.2.0">compare view</a></li> </ul> </details> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=io.rest-assured:rest-assured&package-manager=maven&previous-version=5.1.1&new-version=5.2.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting ``@dependabot` rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> You can trigger Dependabot actions by commenting on this PR: - ``@dependabot` rebase` will rebase this PR - ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it - ``@dependabot` merge` will merge this PR after your CI passes on it - ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it - ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging - ``@dependabot` reopen` will reopen this PR if it is closed - ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> 10374: Verify broker can take backup r=deepthidevaki a=deepthidevaki ## Description - Add a simple test to verify broker can take backup. - Fixed several minor bugs that were found with this test. This PR only adds a basic test to verify backup. More scenarios should be tested later. Also added a Junit5 extension to replace `ClusteringRule`. Now it is a wrapper around `ClusteringRule`. Once all tests have been migrated to junit5, we would be able to replace it. ## Related issues closes #10262 Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>

zeebe-bors-camunda · 2022-09-16T10:11:13Z

This PR was included in a batch that timed out, it will be automatically retried

zeebe-bors-camunda · 2022-09-16T12:56:15Z

Build succeeded:

deepthidevaki mentioned this pull request Sep 15, 2022

Implement take backup api in gateway #9901

Closed

2 tasks

deepthidevaki force-pushed the dd-10262-broker-backup-test branch 2 times, most recently from 40b2939 to 2188095 Compare September 16, 2022 06:46

deepthidevaki added 7 commits September 16, 2022 08:51

test(qa): add Junit5 extension for ClusteringRule to use in qa tests

74c9728

test(qa): verify a cluster can take backup

5351a6e

fix(broker): add BackupStoreTransitionStep to the steps

be154ef

refactor(gateway): allow accessing backup status classes for test

bbdb518

fix(broker): set request id when writing checkpoint command to logstream

97e881c

fix(broker): use correct length for the backup status response

b181e28

fix(gateway): use response schema and template id

9e3c630

deepthidevaki force-pushed the dd-10262-broker-backup-test branch from 2188095 to 0d0caa9 Compare September 16, 2022 06:53

build(pom): fix dependencies

955a9e6

deepthidevaki force-pushed the dd-10262-broker-backup-test branch from 0d0caa9 to 955a9e6 Compare September 16, 2022 06:57

deepthidevaki requested a review from megglos September 16, 2022 06:58

megglos approved these changes Sep 16, 2022

View reviewed changes

build(pom): ignore unused dependency warning

bcc0b62

zeebe-bors-camunda bot merged commit a8d10b2 into main Sep 16, 2022

zeebe-bors-camunda bot deleted the dd-10262-broker-backup-test branch September 16, 2022 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify broker can take backup #10374

Verify broker can take backup #10374

deepthidevaki commented Sep 15, 2022 •

edited

Loading

github-actions bot commented Sep 15, 2022 •

edited

Loading

megglos commented Sep 16, 2022

megglos left a comment

megglos Sep 16, 2022

deepthidevaki Sep 16, 2022

megglos Sep 16, 2022

deepthidevaki Sep 16, 2022

megglos Sep 16, 2022

megglos commented Sep 16, 2022 •

edited

Loading

deepthidevaki commented Sep 16, 2022

deepthidevaki commented Sep 16, 2022

deepthidevaki commented Sep 16, 2022

zeebe-bors-camunda bot commented Sep 16, 2022

zeebe-bors-camunda bot commented Sep 16, 2022

Verify broker can take backup #10374

Verify broker can take backup #10374

Conversation

deepthidevaki commented Sep 15, 2022 • edited Loading

Description

Related issues

Definition of Done

github-actions bot commented Sep 15, 2022 • edited Loading

Test Results

megglos commented Sep 16, 2022

megglos left a comment

Choose a reason for hiding this comment

megglos Sep 16, 2022

Choose a reason for hiding this comment

deepthidevaki Sep 16, 2022

Choose a reason for hiding this comment

megglos Sep 16, 2022

Choose a reason for hiding this comment

deepthidevaki Sep 16, 2022

Choose a reason for hiding this comment

megglos Sep 16, 2022

Choose a reason for hiding this comment

megglos commented Sep 16, 2022 • edited Loading

deepthidevaki commented Sep 16, 2022

deepthidevaki commented Sep 16, 2022

deepthidevaki commented Sep 16, 2022

zeebe-bors-camunda bot commented Sep 16, 2022

zeebe-bors-camunda bot commented Sep 16, 2022

deepthidevaki commented Sep 15, 2022 •

edited

Loading

github-actions bot commented Sep 15, 2022 •

edited

Loading

megglos commented Sep 16, 2022 •

edited

Loading