Add monitor backup API endpoint #10418

npepinpe · 2022-09-20T16:34:53Z

Description

This PR extends the backup management endpoint to add a monitoring API under GET /{id} (e.g. if the actuator is at http://localhost:9600/actuator/backups, then GET http://localhost:9600/actuator/backups/{id}).

At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety.

The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old BackupIT tests, those were deleted.

Related issues

closes #9902

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.

npepinpe · 2022-09-20T16:36:01Z

This PR is blocked by #10411 - please review that first.

github-actions · 2022-09-20T17:14:03Z

Test Results

  926 files +    1   926 suites +1 2h 2m 45s ⏱️ + 2m 36s
7 227 tests - 148 7 217 ✔️ - 148 10 💤 ±0 0 ❌ ±0
7 415 runs - 148 7 405 ✔️ - 148 10 💤 ±0 0 ❌ ±0

Results for commit 2cf33e7. ± Comparison against base commit c7baaf4.

♻️ This comment has been updated with latest results.

npepinpe · 2022-09-20T17:35:52Z

I'm confused why these changes would cause so many flaky tests that are unrelated to backups.

lenaschoenburg

Looks good to me 👍

I guess as long as we can change the return types in a backwards compatible manner we don't need to do any type conversion 🤷

npepinpe · 2022-09-21T14:43:25Z

Right now they happen to match. So as long as we don't change them, 👌 This means we should work on improving the API now and not in a year, as the longer we wait the higher the likelihood of us needing to change these internal types. That said, I did look at them, and while they're internal, they're only used for this specific use case, so the likelihood that we change them is probably quite low 😄

npepinpe · 2022-09-21T14:43:31Z

bors merge

10418: Add monitor backup API endpoint r=npepinpe a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 10428: Remove the extra submit from ProcessingScheduleServiceImpl r=Zelldon a=Zelldon ## Description And another round! 😁 After #10414 and #10390 I think I found a good way to replace the additional Actor#submit from the production code and move it to where it is necessary in tests. As described here #10414 (comment) it is not easily possible to create a separate Actor for the ScheduleService, due to concurrency issues and further work we would need to do here. ## Details In this PR I iterate over the ProcessingScheduleServiceImpl and remove the StreamProcessorContext from the constructor. The service gets all necessary dependencies injected on the constructor (except the actor), mostly as suppliers. This allows to lazy load dependencies and reduce dependencies for tests. It allowed to write some more unit-test like tests. The service gets an open method to initialize the service with the actor control and create its own writer. This allows us to no longer share the writer with the StreamProcessor, since this might lead to issues especially if we now run in between (during processing phases). Due to the later initializing of the writers we reduce resources on followers, if the service is never scheduled we don't need to create a writer. When the StreamProcessor is in the closing phase it will close the scheduled service as well.  ## Related issues  closes #10414 closes #10390 closes #10291 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com> Co-authored-by: Christopher Zell <zelldon91@googlemail.com>

zeebe-bors-camunda · 2022-09-21T15:45:24Z

Build failed (retrying...):

Test summary

10418: Add monitor backup API endpoint r=npepinpe a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

zeebe-bors-camunda · 2022-09-21T16:27:37Z

Build failed:

Test summary

npepinpe · 2022-09-21T19:25:21Z

bors r+

10418: Add monitor backup API endpoint r=npepinpe a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

zeebe-bors-camunda · 2022-09-21T19:40:34Z

Build failed:

Test summary

npepinpe · 2022-09-22T08:20:31Z

bors r+

10418: Add monitor backup API endpoint r=npepinpe a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

npepinpe · 2022-09-22T08:31:23Z

bors r- (networking issues on GitHub hosted runners, cool stuff)
bors r+

zeebe-bors-camunda · 2022-09-22T08:31:25Z

Canceled.

npepinpe · 2022-09-22T08:31:46Z

bors r+

10418: Add monitor backup API endpoint r=npepinpe a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

zeebe-bors-camunda · 2022-09-22T08:31:48Z

Already running a review

zeebe-bors-camunda · 2022-09-22T09:32:51Z

Timed out.

npepinpe · 2022-09-22T09:32:59Z

bors r+

10418: Add monitor backup API endpoint r=npepinpe a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

npepinpe · 2022-09-22T11:12:33Z

Guess it will time out again

npepinpe · 2022-09-22T11:13:15Z

Anyway one of the job failed since it couldn't fetch a dependency.

npepinpe · 2022-09-22T11:13:54Z

bors r-

zeebe-bors-camunda · 2022-09-22T11:13:56Z

Canceled.

lenaschoenburg · 2022-09-22T16:02:15Z

bors r+

10412: Add a standalone app that can be used to restore a broker from a backup r=oleschoenburg a=deepthidevaki ## Description * Added a new module for restore. * Adds a RestoreManager that coordinates restoring of all partitions. It first determines which partitions to restore from based on the given `BrokerCfg`. This configuration must be the same one used by the Broker when it is started after the restore. Because of this, restore module depens on broker module. But since it needs access to the configuration and `RaftPartitionGroupFactory`, there is no other way. There is scope of splitting broker to take these shared configs out of it. * Added a `RestoreApp` in dist. My plan was to add it in restore module. But it needs access to BrokerConfig, which is built in dist. So the app is added to dist module. Alternative is to copy components `BrokerConfiguration` and `WorkerDirectory` to restore module. * The backup id to restore from is passed as a command line argument `--backupId=x`. This is parsed by spring boot. I decided to keep it simple for now, since we are planning to release it as experimental feature anyway. If we decide to support this app long term, then I suggest to switch to something like picocli that we use in zdb. A basic test to verify a broker can be restored is added. More scenarios should be added later. PS:- App is not added to the distribution yet. ## Related issues related #10263 10418: Add monitor backup API endpoint r=oleschoenburg a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 10443: Do not take a backup if it already exists r=oleschoenburg a=deepthidevaki ## Description After restore, the log is truncated to the checkpoint position. So the checkpoint record is processed again and will trigger a new backup with the same Id of the backup it restored from. With this PR, `BackupService` handles this case gracefully. In addition, we also do not take a new backup if existing backup is failed or in progress. Alternatively, we can delete this backup and take a new one. But chances of it happening (i.e triggering a new backup when one already is in progress/failed) is very low. So we can keep this simple. ## Related issues closes #10430 Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com> Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

zeebe-bors-camunda · 2022-09-22T16:39:07Z

Build failed (retrying...):

Test summary

10418: Add monitor backup API endpoint r=oleschoenburg a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 10443: Do not take a backup if it already exists r=oleschoenburg a=deepthidevaki ## Description After restore, the log is truncated to the checkpoint position. So the checkpoint record is processed again and will trigger a new backup with the same Id of the backup it restored from. With this PR, `BackupService` handles this case gracefully. In addition, we also do not take a new backup if existing backup is failed or in progress. Alternatively, we can delete this backup and take a new one. But chances of it happening (i.e triggering a new backup when one already is in progress/failed) is very low. So we can keep this simple. ## Related issues closes #10430 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com> Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>

zeebe-bors-camunda · 2022-09-22T19:26:38Z

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

10418: Add monitor backup API endpoint r=oleschoenburg a=npepinpe ## Description This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`). At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety. The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted. ## Related issues closes #9902 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

zeebe-bors-camunda · 2022-09-22T20:22:18Z

Build failed:

Test summary

deepthidevaki · 2022-09-26T07:18:15Z

bors merge

zeebe-bors-camunda · 2022-09-26T07:30:55Z

Merge conflict.

deepthidevaki · 2022-09-26T08:00:32Z

I will resolve the conflict and get this PR merged.

Adds a new actuator endpoint to get the status of a backup. This endpoint is simply `GET /{id}` of the backup under whichever route the actuator is set up. Currently we do not do any response mapping and use the gateway types directly. While this is not a good approach long term (as it exposes internal types), this is fine for now as we will later use the OpenAPI spec which will generate models for us, and then we will do the mapping. Doing the mapping to our custom types now would end up simply creating busy work we would throw out later.

Adds mapping to get the backup status to the `BackupActuator`. Again we reuse the same internal types, but later this whole client will be mostly generated by the OpenAPI generator. One thing to note, we need to add the `Jdk8Module` to our `JacksonDecoder` - this is done on the server side automatically by Spring, but here we have to do it manually to decode `Optional` types.

Updates the acceptance tests for backups to also make use of the status API endpoint.

deepthidevaki · 2022-09-26T09:34:04Z

bors merge

zeebe-bors-camunda · 2022-09-26T10:18:46Z

Build succeeded:

npepinpe force-pushed the 9902-monitor-backup branch from 52d780a to 7832afc Compare September 20, 2022 16:35

npepinpe force-pushed the 9902-monitor-backup branch from 7832afc to cbbaf47 Compare September 20, 2022 16:44

npepinpe force-pushed the 9901-take-backup branch 4 times, most recently from 6366355 to a9aa867 Compare September 21, 2022 12:45

npepinpe force-pushed the 9902-monitor-backup branch from edd0742 to e0264ee Compare September 21, 2022 12:50

npepinpe requested a review from lenaschoenburg September 21, 2022 13:06

Base automatically changed from 9901-take-backup to main September 21, 2022 13:36

lenaschoenburg approved these changes Sep 21, 2022

View reviewed changes

npepinpe mentioned this pull request Sep 22, 2022

Add a standalone app that can be used to restore a broker from a backup #10412

Merged

10 tasks

deepthidevaki self-assigned this Sep 26, 2022

npepinpe added 3 commits September 26, 2022 10:01

test(qa): update acceptance tests

2cf33e7

Updates the acceptance tests for backups to also make use of the status API endpoint.

deepthidevaki force-pushed the 9902-monitor-backup branch from e0264ee to 2cf33e7 Compare September 26, 2022 08:02

zeebe-bors-camunda bot merged commit 721b6b4 into main Sep 26, 2022

zeebe-bors-camunda bot deleted the 9902-monitor-backup branch September 26, 2022 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add monitor backup API endpoint #10418

Add monitor backup API endpoint #10418

npepinpe commented Sep 20, 2022

npepinpe commented Sep 20, 2022

github-actions bot commented Sep 20, 2022 •

edited

npepinpe commented Sep 20, 2022

lenaschoenburg left a comment

npepinpe commented Sep 21, 2022

npepinpe commented Sep 21, 2022

zeebe-bors-camunda bot commented Sep 21, 2022

zeebe-bors-camunda bot commented Sep 21, 2022

npepinpe commented Sep 21, 2022

zeebe-bors-camunda bot commented Sep 21, 2022

npepinpe commented Sep 22, 2022

npepinpe commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

npepinpe commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

npepinpe commented Sep 22, 2022

npepinpe commented Sep 22, 2022

npepinpe commented Sep 22, 2022

npepinpe commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

lenaschoenburg commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

deepthidevaki commented Sep 26, 2022

zeebe-bors-camunda bot commented Sep 26, 2022

deepthidevaki commented Sep 26, 2022

deepthidevaki commented Sep 26, 2022

zeebe-bors-camunda bot commented Sep 26, 2022

Add monitor backup API endpoint #10418

Add monitor backup API endpoint #10418

Conversation

npepinpe commented Sep 20, 2022

Description

Related issues

Definition of Done

npepinpe commented Sep 20, 2022

github-actions bot commented Sep 20, 2022 • edited

Test Results

npepinpe commented Sep 20, 2022

lenaschoenburg left a comment

Choose a reason for hiding this comment

npepinpe commented Sep 21, 2022

npepinpe commented Sep 21, 2022

zeebe-bors-camunda bot commented Sep 21, 2022

zeebe-bors-camunda bot commented Sep 21, 2022

npepinpe commented Sep 21, 2022

zeebe-bors-camunda bot commented Sep 21, 2022

npepinpe commented Sep 22, 2022

npepinpe commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

npepinpe commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

npepinpe commented Sep 22, 2022

npepinpe commented Sep 22, 2022

npepinpe commented Sep 22, 2022

npepinpe commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

lenaschoenburg commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

zeebe-bors-camunda bot commented Sep 22, 2022

deepthidevaki commented Sep 26, 2022

zeebe-bors-camunda bot commented Sep 26, 2022

deepthidevaki commented Sep 26, 2022

deepthidevaki commented Sep 26, 2022

zeebe-bors-camunda bot commented Sep 26, 2022

github-actions bot commented Sep 20, 2022 •

edited