You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "DaanHoogland (via GitHub)" <gi...@apache.org> on 2026/07/29 09:48:50 UTC
[PR] deal with excessive attempts on failing snapshots [cloudstack]
DaanHoogland opened a new pull request, #13743:
URL: https://github.com/apache/cloudstack/pull/13743
### Description
This PR...
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
Fixes: #13454
Fixes: (some old issue I need to lookup as well)
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5174600376
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5168175559
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5528266888
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19087
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5116840168
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743) **Quality Gate passed**
Issues
 [81 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [48.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5116145808
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13743?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.41%. Comparing base ([`e1cf0f3`](https://app.codecov.io/gh/apache/cloudstack/commit/e1cf0f335a7f2e4b6cd71c3147ef64bcd89d486e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`de2aee2`](https://app.codecov.io/gh/apache/cloudstack/commit/de2aee26a8b3a05121b86c0c85ec3dcaf8d6a68c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (e1cf0f3) and HEAD (de2aee2). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (e1cf0f3) | HEAD (de2aee2) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #13743 +/- ##
=============================================
- Coverage 19.64% 3.41% -16.24%
=============================================
Files 6368 487 -5881
Lines 575119 41867 -533252
Branches 70382 7912 -62470
=============================================
- Hits 112994 1429 -111565
+ Misses 449839 40238 -409601
+ Partials 12286 200 -12086
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13743/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13743/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.41% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13743/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/13743?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5539000082
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5539506786
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19102
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub) via commits" <co...@cloudstack.apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-6766136505
@nvazquez , should be ready (testing needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5176690608
<b>[SF] Trillian Build Failed (tid-16695)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5168200176
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5174751219
<b>[SF] Trillian Build Failed (tid-16687)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5527657279
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5566559135
<b>[SF] Trillian Build Failed (tid-16910)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5529837916
<b>[SF] Trillian Build Failed (tid-16894)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5528644052
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5551290217
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5174597141
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5116845365
## 🔴 Test Coverage Grade: `D` — Marginal
| Metric | Value |
|--------|-------|
| Line coverage | **24.52%** |
| Branch coverage | **18.68%** |
### Grade Scale
| Grade | Line Coverage | Meaning |
|-------|--------------|---------|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
> Branch coverage is shown as a secondary signal. Grade is determined by **line coverage**.
> [View full Actions run](https://github.com/apache/cloudstack/actions/runs/30441159825)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5522403372
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5515521332
<!-- coverage-grade-bot -->
## 🔴 Test Coverage Grade: `D` — Marginal
| Metric | Value |
|--------|-------|
| Line coverage | **24.69%** |
| Branch coverage | **18.90%** |
### Grade Scale
| Grade | Line Coverage | Meaning |
|-------|--------------|---------|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
> Branch coverage is shown as a secondary signal. Grade is determined by **line coverage**.
> [View full Actions run](https://github.com/apache/cloudstack/actions/runs/33668816543)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5551279264
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5128131560
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743) **Quality Gate passed**
Issues
 [81 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [48.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub) via commits" <co...@cloudstack.apache.org>.
sonarqubecloud[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-6584433098
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [73.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "nvazquez (via GitHub) via commits" <co...@cloudstack.apache.org>.
nvazquez commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-6746389449
Hi @DaanHoogland any further fixes required to address the Copilot comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5168800744
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18746
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5128134978
## 🔴 Test Coverage Grade: `D` — Marginal
| Metric | Value |
|--------|-------|
| Line coverage | **24.52%** |
| Branch coverage | **18.68%** |
### Grade Scale
| Grade | Line Coverage | Meaning |
|-------|--------------|---------|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
> Branch coverage is shown as a secondary signal. Grade is determined by **line coverage**.
> [View full Actions run](https://github.com/apache/cloudstack/actions/runs/30520667311)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5524821845
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [47.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5529334979
<b>[SF] Trillian Build Failed (tid-16889)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5538958124
@blueorangutan shutup
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5514579081
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub) via commits" <co...@cloudstack.apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-6765554766
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19969
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#discussion_r3673112606
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +354,155 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Handles a synchronous failure to dispatch the CreateSnapshotCmd (e.g. an allocation error) for a recurring
+ * snapshot. Without this, the schedule's {@code scheduledTimestamp} is never advanced, so it gets retried on
+ * every poll (every {@code snapshot.poll.interval} seconds) forever. Instead: log a failure event keyed by the
+ * volume (so consecutive failures can be counted from event history), and either back off by the configured
+ * retry interval, or - once the configured maximum consecutive failures is reached - give up until the next
+ * regularly scheduled run and raise a WARN notification event.
+ */
+ protected void handleFailedSnapshotDispatch(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume,
+ final SnapshotScheduleVO lockedSchedule, final Long eventId, final Exception cause) {
+ final long volumeId = volume.getId();
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures = getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume, account);
+ final int retryInterval = getScopedConfigValue(SnapshotManager.SnapshotRecurringRetryInterval, volume, account);
+ final int totalFailures = countConsecutiveFailedAttempts(volumeId, maxFailures) + 1;
+
+ final String failureMessage = String.format("Failed to create scheduled snapshot for volume [%s]: %s", volume, cause.getMessage());
+ if (eventId != null) {
+ ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, failureMessage, volumeId, ApiCommandResourceType.Volume.toString(), eventId);
+ } else {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, true, failureMessage, volumeId, ApiCommandResourceType.Volume.toString());
+ }
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ final Date nextRegularRun = getNextScheduledTime(snapshotToBeExecuted.getPolicyId(), _currentTimestamp);
+ lockedSchedule.setScheduledTimestamp(nextRegularRun);
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed [{}] consecutive times; it will not be retried until its next regularly scheduled run at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRegularRun);
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_WARN, EventTypes.EVENT_SNAPSHOT_CREATE, true,
+ String.format("Recurring snapshot for volume [%s] has failed %d consecutive times and will not be retried until its next regularly scheduled run.", volume, totalFailures),
+ volumeId, ApiCommandResourceType.Volume.toString());
+ } else {
+ final Date nextRetry = new Date(_currentTimestamp.getTime() + retryInterval * 1000L);
+ lockedSchedule.setScheduledTimestamp(nextRetry);
+ logger.debug("Snapshot schedule [{}] for volume [{}] failed [{}] time(s); retrying at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRetry);
+ }
+ _snapshotScheduleDao.update(lockedSchedule.getId(), lockedSchedule);
+ }
+
+ /**
+ * Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} events logged for this volume are
+ * consecutive failures (level ERROR), starting from the most recent event and stopping at the first
+ * non-failure (or absent) event. This derives the "number of failed attempts" from event history instead of a
+ * dedicated counter column.
+ */
+ protected int countConsecutiveFailedAttempts(final long volumeId, final int limit) {
+ if (limit <= 0) {
+ return 0;
+ }
+ final List<EventVO> recentEvents = _eventDao.listLatestEventsByResource(volumeId, ApiCommandResourceType.Volume.toString(),
+ EventTypes.EVENT_SNAPSHOT_CREATE, limit);
+ int count = 0;
+ for (final EventVO event : recentEvents) {
+ if (!EventVO.LEVEL_ERROR.equals(event.getLevel())) {
+ break;
+ }
+ count++;
+ }
+ return count;
+ }
+
+ /**
+ * Resolves a config value in order of most to least specific scope: account, domain, zone, then global. A
+ * {@link ConfigKey} can only walk a single scope-parent chain automatically (Account->Domain->Global, or
+ * Zone->Global), so the four scopes are resolved manually here.
+ */
+ protected <T> T getScopedConfigValue(final ConfigKey<T> key, final VolumeVO volume, final Account account) {
+ T value = key.valueInScope(ConfigKey.Scope.Account, volume.getAccountId(), true);
+ if (value == null && account != null) {
+ value = key.valueInScope(ConfigKey.Scope.Domain, account.getDomainId(), true);
+ }
+ if (value == null) {
+ value = key.valueInScope(ConfigKey.Scope.Zone, volume.getDataCenterId(), true);
+ }
+ if (value == null) {
+ value = key.value();
+ }
+ return value;
+ }
Review Comment:
`getScopedConfigValue` is intended to manually resolve Account → Domain → Zone → Global, but passing `true` to `valueInScope` typically enables fallback to defaults up the scope chain. That means the first call (Account scope) can return a Domain/Global value and prevent Zone overrides from ever being considered. Use non-fallback lookups for Account/Domain/Zone (e.g., `useDefault=false`) and only fall back to `key.value()` at the end, so Zone-scoped config is honored.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -206,6 +220,40 @@ protected void scheduleNextSnapshotJobIfNecessary(SnapshotScheduleVO snapshotSch
scheduleNextSnapshotJob(snapshotSchedule);
}
+ /**
+ * Logs an event for the outcome of a recurring snapshot job (keyed by the volume, since a fresh snapshot entity
+ * ID is minted on every attempt) so that consecutive failures can be counted from event history, and raises a
+ * WARN notification once {@link SnapshotManager#SnapshotRecurringMaxFailures} consecutive failures are reached.
+ */
+ protected void recordSnapshotAttemptOutcome(final SnapshotScheduleVO snapshotSchedule, final boolean succeeded, final String failureResult) {
+ final VolumeVO volume = _volsDao.findByIdIncludingRemoved(snapshotSchedule.getVolumeId());
+ if (volume == null) {
+ return;
+ }
+
+ if (succeeded) {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_INFO, EventTypes.EVENT_SNAPSHOT_CREATE, true,
+ String.format("Scheduled snapshot creation job for volume [%s] succeeded.", volume),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+ return;
+ }
+
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures = getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume, account);
+ final int totalFailures = countConsecutiveFailedAttempts(volume.getId(), maxFailures) + 1;
+
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR, EventTypes.EVENT_SNAPSHOT_CREATE, true,
+ String.format("Scheduled snapshot creation job for volume [%s] failed: %s", volume, failureResult),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed [{}] consecutive times.", snapshotSchedule, volume, totalFailures);
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_WARN, EventTypes.EVENT_SNAPSHOT_CREATE, true,
+ String.format("Recurring snapshot for volume [%s] has failed %d consecutive times.", volume, totalFailures),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+ }
Review Comment:
The WARN notification is logged using the same `EventTypes.EVENT_SNAPSHOT_CREATE` type that `countConsecutiveFailedAttempts` scans, and `countConsecutiveFailedAttempts` stops at the first non-ERROR level. This means once a WARN is emitted, the latest event is no longer an ERROR and the failure counter effectively resets, causing threshold behavior to repeat incorrectly and undermining “consecutive failures” tracking. Consider logging threshold notifications under a different event type (recommended), or update the counting logic to skip/ignore WARN/INFO notification events and fetch enough history to still count the latest N ERROR attempts accurately.
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -105,6 +112,16 @@ public List<EventVO> listToArchiveOrDeleteEvents(List<Long> ids, String type, Da
return search(sc, null);
}
+ @Override
+ public List<EventVO> listLatestEventsByResource(long resourceId, String resourceType, String type, int limit) {
+ SearchCriteria<EventVO> sc = LatestEventsByResourceSearch.create();
+ sc.setParameters("resourceId", resourceId);
+ sc.setParameters("resourceType", resourceType);
+ sc.setParameters("type", type);
+ Filter filter = new Filter(EventVO.class, "createDate", false, 0L, (long) limit);
+ return listBy(sc, filter);
+ }
Review Comment:
`listLatestEventsByResource` doesn’t constrain results to non-archived events, even though `EventVO` supports archiving and other searches in this DAO explicitly filter by `archived`. This can make consecutive-failure counting incorrect (e.g., counting old archived failures) and may be surprising for callers expecting “recent” active events. Add an `archived=false` criterion to `LatestEventsByResourceSearch` (or otherwise ensure only non-archived events are returned).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5551525288
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19111
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5527644572
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5522420383
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#discussion_r3682943831
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +354,155 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Handles a synchronous failure to dispatch the CreateSnapshotCmd (e.g. an allocation error) for a recurring
+ * snapshot. Without this, the schedule's {@code scheduledTimestamp} is never advanced, so it gets retried on
+ * every poll (every {@code snapshot.poll.interval} seconds) forever. Instead: log a failure event keyed by the
+ * volume (so consecutive failures can be counted from event history), and either back off by the configured
+ * retry interval, or - once the configured maximum consecutive failures is reached - give up until the next
+ * regularly scheduled run and raise a WARN notification event.
+ */
+ protected void handleFailedSnapshotDispatch(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume,
+ final SnapshotScheduleVO lockedSchedule, final Long eventId, final Exception cause) {
+ final long volumeId = volume.getId();
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures = getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume, account);
+ final int retryInterval = getScopedConfigValue(SnapshotManager.SnapshotRecurringRetryInterval, volume, account);
+ final int totalFailures = countConsecutiveFailedAttempts(volumeId, maxFailures) + 1;
+
+ final String failureMessage = String.format("Failed to create scheduled snapshot for volume [%s]: %s", volume, cause.getMessage());
+ if (eventId != null) {
+ ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, failureMessage, volumeId, ApiCommandResourceType.Volume.toString(), eventId);
+ } else {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, true, failureMessage, volumeId, ApiCommandResourceType.Volume.toString());
+ }
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ final Date nextRegularRun = getNextScheduledTime(snapshotToBeExecuted.getPolicyId(), _currentTimestamp);
+ lockedSchedule.setScheduledTimestamp(nextRegularRun);
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed [{}] consecutive times; it will not be retried until its next regularly scheduled run at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRegularRun);
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_WARN, EventTypes.EVENT_SNAPSHOT_RECURRING_FAILURE_LIMIT_REACHED, true,
+ String.format("Recurring snapshot for volume [%s] has failed %d consecutive times and will not be retried until its next regularly scheduled run.", volume, totalFailures),
+ volumeId, ApiCommandResourceType.Volume.toString());
+ } else {
+ final Date nextRetry = new Date(_currentTimestamp.getTime() + retryInterval * 1000L);
+ lockedSchedule.setScheduledTimestamp(nextRetry);
+ logger.debug("Snapshot schedule [{}] for volume [{}] failed [{}] time(s); retrying at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRetry);
+ }
+ _snapshotScheduleDao.update(lockedSchedule.getId(), lockedSchedule);
+ }
+
+ /**
+ * Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} events logged for this volume are
+ * consecutive failures (level ERROR), starting from the most recent event and stopping at the first
+ * non-failure (or absent) event. This derives the "number of failed attempts" from event history instead of a
+ * dedicated counter column.
+ */
+ protected int countConsecutiveFailedAttempts(final long volumeId, final int limit) {
+ if (limit <= 0) {
+ return 0;
+ }
+ final List<EventVO> recentEvents = _eventDao.listLatestEventsByResource(volumeId, ApiCommandResourceType.Volume.toString(),
+ EventTypes.EVENT_SNAPSHOT_CREATE, limit);
+ int count = 0;
+ for (final EventVO event : recentEvents) {
+ if (!EventVO.LEVEL_ERROR.equals(event.getLevel())) {
+ break;
+ }
+ count++;
+ }
+ return count;
+ }
+
+ /**
+ * Resolves a config value in order of most to least specific scope: account, domain, zone, then global. A
+ * {@link ConfigKey} can only walk a single scope-parent chain automatically (Account->Domain->Global, or
+ * Zone->Global), so the four scopes are resolved manually here.
+ */
+ protected <T> T getScopedConfigValue(final ConfigKey<T> key, final VolumeVO volume, final Account account) {
+ T value = key.valueInScope(ConfigKey.Scope.Account, volume.getAccountId(), true);
+ if (value == null && account != null) {
+ value = key.valueInScope(ConfigKey.Scope.Domain, account.getDomainId(), true);
+ }
+ if (value == null) {
+ value = key.valueInScope(ConfigKey.Scope.Zone, volume.getDataCenterId(), true);
+ }
+ if (value == null) {
+ value = key.value();
+ }
+ return value;
+ }
Review Comment:
The scoped fallback logic is likely bypassed by calling `valueInScope(..., true)` for Account/Domain/Zone. If the boolean enables parent/default fallback, `value` will rarely (or never) be null, so Domain/Zone/global precedence won’t work as intended (e.g., a Zone override could be ignored because Account-scope lookup already returned a default). Use a mode that returns null when not explicitly set for that scope (or otherwise detect ‘unset’) for Account/Domain/Zone, and only fall back to `key.value()` at the end.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -206,6 +220,40 @@ protected void scheduleNextSnapshotJobIfNecessary(SnapshotScheduleVO snapshotSch
scheduleNextSnapshotJob(snapshotSchedule);
}
+ /**
+ * Logs an event for the outcome of a recurring snapshot job (keyed by the volume, since a fresh snapshot entity
+ * ID is minted on every attempt) so that consecutive failures can be counted from event history, and raises a
+ * WARN notification once {@link SnapshotManager#SnapshotRecurringMaxFailures} consecutive failures are reached.
+ */
+ protected void recordSnapshotAttemptOutcome(final SnapshotScheduleVO snapshotSchedule, final boolean succeeded, final String failureResult) {
+ final VolumeVO volume = _volsDao.findByIdIncludingRemoved(snapshotSchedule.getVolumeId());
+ if (volume == null) {
+ return;
+ }
+
+ if (succeeded) {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_INFO, EventTypes.EVENT_SNAPSHOT_CREATE, true,
+ String.format("Scheduled snapshot creation job for volume [%s] succeeded.", volume),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+ return;
+ }
+
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures = getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume, account);
+ final int totalFailures = countConsecutiveFailedAttempts(volume.getId(), maxFailures) + 1;
+
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR, EventTypes.EVENT_SNAPSHOT_CREATE, true,
+ String.format("Scheduled snapshot creation job for volume [%s] failed: %s", volume, failureResult),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed [{}] consecutive times.", snapshotSchedule, volume, totalFailures);
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_WARN, EventTypes.EVENT_SNAPSHOT_CREATE, true,
+ String.format("Recurring snapshot for volume [%s] has failed %d consecutive times.", volume, totalFailures),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+ }
Review Comment:
The WARN notification is logged with `EventTypes.EVENT_SNAPSHOT_CREATE`, which means it becomes part of the same event stream that `countConsecutiveFailedAttempts(..., EVENT_SNAPSHOT_CREATE, ...)` scans. Since it’s not LEVEL_ERROR, it will stop the scan and effectively reset the consecutive-failure count on the next attempt. Log this WARN with a distinct event type (e.g. `EVENT_SNAPSHOT_RECURRING_FAILURE_LIMIT_REACHED`, consistent with `handleFailedSnapshotDispatch`) so failure counting remains accurate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "Copilot (via GitHub) via commits" <co...@cloudstack.apache.org>.
Copilot commented on code in PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#discussion_r4444853897
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -237,58 +295,21 @@ protected void scheduleSnapshots() {
for (final SnapshotScheduleVO snapshotToBeExecuted : snapshotsToBeExecuted) {
SnapshotScheduleVO tmpSnapshotScheduleVO = null;
+ Long eventId = null;
final long snapshotScheId = snapshotToBeExecuted.getId();
- final long policyId = snapshotToBeExecuted.getPolicyId();
- final long volumeId = snapshotToBeExecuted.getVolumeId();
final VolumeVO volume = _volsDao.findByIdIncludingRemoved(snapshotToBeExecuted.getVolumeId());
try {
- if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ if (shouldSkipSchedule(snapshotToBeExecuted, volume)) {
continue;
}
tmpSnapshotScheduleVO = _snapshotScheduleDao.acquireInLockTable(snapshotScheId);
- final Long eventId =
- ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for volume Id:" +
- volume.getUuid(), volumeId, ApiCommandResourceType.Volume.toString(), true, 0);
-
- logger.trace("Mapping parameters required to generate a CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
- final Map<String, String> params = new HashMap<String, String>();
- params.put(ApiConstants.VOLUME_ID, "" + volumeId);
- params.put(ApiConstants.POLICY_ID, "" + policyId);
- params.put("ctxUserId", "1");
- params.put("ctxAccountId", "" + volume.getAccountId());
- params.put("ctxStartEventId", String.valueOf(eventId));
- List<? extends ResourceTag> resourceTags = taggedResourceService.listByResourceTypeAndId(ResourceTag.ResourceObjectType.SnapshotPolicy, policyId);
- if (resourceTags != null && !resourceTags.isEmpty()) {
- int tagNumber = 0;
- for (ResourceTag resourceTag : resourceTags) {
- params.put("tags[" + tagNumber + "].key", resourceTag.getKey());
- params.put("tags[" + tagNumber + "].value", resourceTag.getValue());
- tagNumber++;
- }
- }
-
- logger.trace("Generating a CreateSnapshotCmd for snapshot [{}] with parameters: [{}].", snapshotToBeExecuted, params.toString());
- final CreateSnapshotCmd cmd = new CreateSnapshotCmd();
- ComponentContext.inject(cmd);
- _dispatcher.dispatchCreateCmd(cmd, params);
- params.put("id", "" + cmd.getEntityId());
- params.put("ctxStartEventId", "1");
-
- final Date scheduledTimestamp = snapshotToBeExecuted.getScheduledTimestamp();
- displayTime = DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, scheduledTimestamp);
- logger.debug("Scheduling snapshot [{}] for volume [{}] at [{}].", snapshotToBeExecuted, volume, displayTime);
- AsyncJobVO job = new AsyncJobVO("", User.UID_SYSTEM, volume.getAccountId(), CreateSnapshotCmd.class.getName(),
- ApiGsonHelper.getBuilder().create().toJson(params), cmd.getEntityId(),
- cmd.getApiResourceType() != null ? cmd.getApiResourceType().toString() : null, null);
- job.setDispatcher(_asyncDispatcher.getName());
- final long jobId = _asyncMgr.submitAsyncJob(job);
- logger.debug("Scheduled snapshot [{}] for volume [{}] as job [{}].", snapshotToBeExecuted, volume, job);
-
- tmpSnapshotScheduleVO.setAsyncJobId(jobId);
- _snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
+ eventId = dispatchSnapshotCreateJob(snapshotToBeExecuted, volume, tmpSnapshotScheduleVO);
} catch (final Exception e) {
logger.error("The scheduling of snapshot [{}] for volume [{}] failed due to [{}].", snapshotToBeExecuted, volume, e.toString(), e);
+ if (tmpSnapshotScheduleVO != null) {
+ handleFailedSnapshotDispatch(snapshotToBeExecuted, volume, tmpSnapshotScheduleVO, eventId, e);
+ }
} finally {
Review Comment:
If `dispatchSnapshotCreateJob` throws after it has already created the “scheduled” action event (but before returning), `eventId` will still be `null` here and `handleFailedSnapshotDispatch` won’t complete the scheduled event, leaving it dangling. A concrete fix is to ensure the scheduled event id is assigned in the caller before subsequent dispatch steps can throw (e.g., split “create scheduled event” from the rest, or have `dispatchSnapshotCreateJob` catch/rewrap exceptions with the `eventId` so the catch block can always call `onCompletedActionEvent` when applicable).
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -125,6 +137,20 @@ public List<EventVO> listToArchiveOrDeleteEvents(List<Long> ids, String type, Da
return search(sc, null);
}
+ @Override
+ public List<EventVO> listLatestOutcomeEventsByResource(long resourceId, String resourceType, String type, int limit) {
+ SearchCriteria<EventVO> sc = LatestEventsByResourceSearch.create();
+ sc.setParameters(FIELD_RESOURCE_ID, resourceId);
+ sc.setParameters(FIELD_RESOURCE_TYPE, resourceType);
+ sc.setParameters("type", type);
+ sc.setParameters("state", State.Created, State.Completed);
+ sc.setParameters("archived", false);
+ Filter filter = new Filter(EventVO.class, "createDate", false, 0L, (long) limit);
Review Comment:
The ordering field `"createDate"` looks inconsistent with the comment (“created”) and the expected SQL in `EventDaoImplTest` (which asserts `ORDER BY event.created ...`). If the mapped field is actually `created`, this will silently sort incorrectly (or not at all) depending on ORM mapping. Prefer ordering by the same property used elsewhere (likely `"created"`) and keep the secondary `id` ordering for tie-breaking.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +318,225 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Returns true (after rescheduling as needed) when this iteration's snapshot should not be dispatched: either
+ * because it can't be scheduled at all, or because it's a redundant snapshot of an unchanged volume that gets
+ * skipped and rescheduled to its next regular run instead. Kept as a single decision point so the caller only
+ * needs one {@code continue}.
+ */
+ protected boolean shouldSkipSchedule(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume) {
+ if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ return true;
+ }
+ if (shouldSkipUnchangedVolumeSnapshot(volume)) {
+ skipAndRescheduleSnapshot(snapshotToBeExecuted, volume);
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Builds and dispatches the CreateSnapshotCmd async job for a scheduled snapshot, returning the "scheduled"
+ * action event id so the caller can complete it if dispatch subsequently fails.
+ */
+ protected Long dispatchSnapshotCreateJob(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO tmpSnapshotScheduleVO) throws Exception {
+ final long snapshotScheId = snapshotToBeExecuted.getId();
+ final long policyId = snapshotToBeExecuted.getPolicyId();
+ final long volumeId = snapshotToBeExecuted.getVolumeId();
+
+ final Long eventId =
+ ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for volume Id:" +
+ volume.getUuid(), volumeId, ApiCommandResourceType.Volume.toString(), true, 0);
+
+ logger.trace("Mapping parameters required to generate a CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
+ final Map<String, String> params = new HashMap<String, String>();
+ params.put(ApiConstants.VOLUME_ID, "" + volumeId);
+ params.put(ApiConstants.POLICY_ID, "" + policyId);
+ params.put("ctxUserId", "1");
+ params.put("ctxAccountId", "" + volume.getAccountId());
+ params.put("ctxStartEventId", String.valueOf(eventId));
+ List<? extends ResourceTag> resourceTags = taggedResourceService.listByResourceTypeAndId(ResourceTag.ResourceObjectType.SnapshotPolicy, policyId);
+ if (resourceTags != null && !resourceTags.isEmpty()) {
+ int tagNumber = 0;
+ for (ResourceTag resourceTag : resourceTags) {
+ params.put("tags[" + tagNumber + "].key", resourceTag.getKey());
+ params.put("tags[" + tagNumber + "].value", resourceTag.getValue());
+ tagNumber++;
+ }
+ }
+
+ logger.trace("Generating a CreateSnapshotCmd for snapshot [{}] with parameters: [{}].", snapshotToBeExecuted, params.toString());
+ final CreateSnapshotCmd cmd = new CreateSnapshotCmd();
+ ComponentContext.inject(cmd);
+ _dispatcher.dispatchCreateCmd(cmd, params);
+ params.put("id", "" + cmd.getEntityId());
+ params.put("ctxStartEventId", "1");
+
+ final Date scheduledTimestamp = snapshotToBeExecuted.getScheduledTimestamp();
+ final String displayTime = DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, scheduledTimestamp);
+ logger.debug("Scheduling snapshot [{}] for volume [{}] at [{}].", snapshotToBeExecuted, volume, displayTime);
+ AsyncJobVO job = new AsyncJobVO("", User.UID_SYSTEM, volume.getAccountId(), CreateSnapshotCmd.class.getName(),
+ ApiGsonHelper.getBuilder().create().toJson(params), cmd.getEntityId(),
+ cmd.getApiResourceType() != null ? cmd.getApiResourceType().toString() : null, null);
+ job.setDispatcher(_asyncDispatcher.getName());
+ final long jobId = _asyncMgr.submitAsyncJob(job);
+ logger.debug("Scheduled snapshot [{}] for volume [{}] as job [{}].", snapshotToBeExecuted, volume, job);
+
+ tmpSnapshotScheduleVO.setAsyncJobId(jobId);
+ _snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
+
+ return eventId;
+ }
+
+ /**
+ * Handles a synchronous failure to dispatch the CreateSnapshotCmd (e.g. an allocation error) for a recurring
+ * snapshot. Without this, the schedule's {@code scheduledTimestamp} is never advanced, so it gets retried on
+ * every poll (every {@code snapshot.poll.interval} seconds) forever. Instead: log a failure event keyed by the
+ * volume (so consecutive failures can be counted from event history), and either back off by the configured
+ * retry interval, or - once the configured maximum consecutive failures is reached - give up until the next
+ * regularly scheduled run and raise a WARN notification event.
+ */
+ protected void handleFailedSnapshotDispatch(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume,
+ final SnapshotScheduleVO lockedSchedule, final Long eventId, final Exception cause) {
+ final long volumeId = volume.getId();
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures = getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume, account);
+ final int retryInterval = getScopedConfigValue(SnapshotManager.SnapshotRecurringRetryInterval, volume, account);
+ final int totalFailures = countConsecutiveFailedAttempts(volumeId, maxFailures) + 1;
+
+ final String failureMessage = String.format("Failed to create scheduled snapshot for volume [%s]: %s", volume, cause.getMessage());
+ if (eventId != null) {
+ ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, failureMessage, volumeId, ApiCommandResourceType.Volume.toString(), eventId);
+ } else {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, true, failureMessage, volumeId, ApiCommandResourceType.Volume.toString());
+ }
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ final Date nextRegularRun = getNextScheduledTime(snapshotToBeExecuted.getPolicyId(), _currentTimestamp);
+ lockedSchedule.setScheduledTimestamp(nextRegularRun);
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed [{}] consecutive times; it will not be retried until its next regularly scheduled run at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRegularRun);
+ raiseFailureLimitReachedEvent(volume, String.format(
+ "Recurring snapshot for volume [%s] has failed %d consecutive times and will not be retried until its next regularly scheduled run.", volume, totalFailures));
+ } else {
+ final Date nextRetry = new Date(_currentTimestamp.getTime() + retryInterval * 1000L);
+ lockedSchedule.setScheduledTimestamp(nextRetry);
+ logger.debug("Snapshot schedule [{}] for volume [{}] failed [{}] time(s); retrying at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRetry);
+ }
+ _snapshotScheduleDao.update(lockedSchedule.getId(), lockedSchedule);
+ }
+
+ /**
+ * Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} outcome events logged for this volume are
+ * consecutive failures (level ERROR), starting from the most recent event and stopping at the first
+ * non-failure (or absent) event. The Scheduled event logged before every dispatch is not an outcome and is
+ * excluded by the DAO, otherwise it would interrupt every run of failures. This derives the "number of failed attempts" from event history instead of a
+ * dedicated counter column.
+ */
+ protected int countConsecutiveFailedAttempts(final long volumeId, final int limit) {
+ if (limit <= 0) {
+ return 0;
+ }
+ final List<EventVO> recentEvents = eventDao.listLatestOutcomeEventsByResource(volumeId, ApiCommandResourceType.Volume.toString(),
+ EventTypes.EVENT_SNAPSHOT_CREATE, limit);
+ int count = 0;
+ for (final EventVO event : recentEvents) {
+ if (!EventVO.LEVEL_ERROR.equals(event.getLevel())) {
+ break;
+ }
+ count++;
+ }
+ return count;
+ }
+
+ /**
+ * Resolves a config value in order of most to least specific scope: account, domain, zone, then global. A
+ * {@link ConfigKey} can only walk a single scope-parent chain automatically (Account->Domain->Global, or
+ * Zone->Global), so the four scopes are resolved manually here.
+ */
+ protected <T> T getScopedConfigValue(final ConfigKey<T> key, final VolumeVO volume, final Account account) {
+ T value = key.valueInScope(ConfigKey.Scope.Account, volume.getAccountId(), true);
+ if (value == null && account != null) {
+ value = key.valueInScope(ConfigKey.Scope.Domain, account.getDomainId(), true);
+ }
+ if (value == null) {
+ value = key.valueInScope(ConfigKey.Scope.Zone, volume.getDataCenterId(), true);
+ }
+ if (value == null) {
+ value = key.value();
+ }
+ return value;
+ }
Review Comment:
Using `valueInScope(..., inherit=true)` for Account/Domain can return a Global value before the Zone scope is checked, which makes Zone overrides unreachable when Account/Domain aren’t set. To preserve the intended precedence (Account → Domain → Zone → Global), query each scope with `inherit=false` and only fall back to `key.value()` at the end (or otherwise ensure Zone is consulted before any implicit Global fallback is applied).
##########
api/src/main/java/com/cloud/event/EventTypes.java:
##########
@@ -1094,6 +1096,8 @@ public class EventTypes {
// Snapshots
entityEventDetails.put(EVENT_SNAPSHOT_CREATE, Snapshot.class);
+ entityEventDetails.put(EVENT_SNAPSHOT_SKIPPED, Snapshot.class);
+ entityEventDetails.put(EVENT_SNAPSHOT_RECURRING_FAILURE_LIMIT_REACHED, Snapshot.class);
Review Comment:
The new events (`SNAPSHOT.SKIPPED`, `SNAPSHOT.RECURRING.FAILURE.LIMIT.REACHED`) are logged against the Volume resource in `SnapshotSchedulerImpl` (resource id = volume id, resource type = Volume). Mapping them to `Snapshot.class` risks incorrect entity lookups (volumeId ≠ snapshotId) in any code that relies on `entityEventDetails`. Consider mapping these new event types to `Volume.class` (or aligning the logged resource type/id with Snapshot if that’s required by the event system).
##########
engine/schema/src/test/java/com/cloud/event/dao/EventDaoImplTest.java:
##########
@@ -0,0 +1,85 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.event.dao;
+
+import com.cloud.event.Event;
+import com.cloud.event.EventVO;
+import com.cloud.utils.db.Filter;
+import com.cloud.utils.db.SearchCriteria;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.InjectMocks;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.verify;
+
+@RunWith(MockitoJUnitRunner.class)
+public class EventDaoImplTest {
+
+ @Spy
+ @InjectMocks
+ EventDaoImpl eventDaoImpl = new EventDaoImpl();
+
+ @Test
+ public void listLatestOutcomeEventsByResourceQueriesOutcomesByResourceAndTypeExcludingArchived() {
+ List<EventVO> expected = Collections.singletonList(new EventVO());
+ doReturn(expected).when(eventDaoImpl).listBy(any(SearchCriteria.class), any(Filter.class));
+
+ List<EventVO> result = eventDaoImpl.listLatestOutcomeEventsByResource(1L, "Volume", "SNAPSHOT.CREATE", 3);
+
+ Assert.assertSame(expected, result);
+
+ ArgumentCaptor<SearchCriteria> scCaptor = ArgumentCaptor.forClass(SearchCriteria.class);
+ ArgumentCaptor<Filter> filterCaptor = ArgumentCaptor.forClass(Filter.class);
+ verify(eventDaoImpl).listBy(scCaptor.capture(), filterCaptor.capture());
+
+ SearchCriteria<EventVO> sc = scCaptor.getValue();
+ Assert.assertEquals("event.resource_id = ? AND event.resource_type = ? AND event.type = ? AND event.state IN (?,?) AND event.archived = ? ",
+ sc.getWhereClause());
Review Comment:
This assertion is brittle because it depends on the exact formatting/order of the generated where-clause string (which can change with minor DAO/SearchBuilder changes). It would be more robust to assert on the captured parameters (which you already do) and only minimally validate the presence of key predicates rather than exact full-string equality.
##########
server/src/test/java/com/cloud/storage/snapshot/SnapshotSchedulerImplTest.java:
##########
@@ -202,76 +253,458 @@ public void canSnapshotBeScheduledTestAccountIsRemovedOrDisabledReturnFalse() {
public void canSnapshotBeScheduledTestSnapshotPolicyIsRemovedCallRemove() {
SnapshotScheduleVO snapshotScheduleVO = new SnapshotScheduleVO();
- Mockito.doReturn(1l).when(volumeVoMock).getPoolId();
- Mockito.doReturn(false).when(snapshotSchedulerImplSpy).isAccountRemovedOrDisabled(Mockito.any(), Mockito.any());
- Mockito.doReturn(null).when(snapshotPolicyDaoMock).findById(Mockito.any());
+ doReturn(1l).when(volumeVoMock).getPoolId();
+ doReturn(false).when(snapshotSchedulerImplSpy).isAccountRemovedOrDisabled(Mockito.any(), Mockito.any());
+ doReturn(null).when(snapshotPolicyDaoMock).findById(Mockito.any());
boolean result = snapshotSchedulerImplSpy.canSnapshotBeScheduled(snapshotScheduleVO, volumeVoMock);
Assert.assertTrue(result);
- Mockito.verify(snapshotScheduleDaoMock).remove(Mockito.anyLong());
+ verify(snapshotScheduleDaoMock).remove(Mockito.anyLong());
}
@Test
public void canSnapshotBeScheduledTestSnapshotPolicyIsNotRemovedDoNotCallRemove() {
SnapshotScheduleVO snapshotScheduleVO = new SnapshotScheduleVO();
SnapshotPolicyVO snapshotPolicyVO = new SnapshotPolicyVO();
- Mockito.doReturn(1l).when(volumeVoMock).getPoolId();
- Mockito.doReturn(false).when(snapshotSchedulerImplSpy).isAccountRemovedOrDisabled(Mockito.any(), Mockito.any());
- Mockito.doReturn(snapshotPolicyVO).when(snapshotPolicyDaoMock).findById(Mockito.any());
+ doReturn(1l).when(volumeVoMock).getPoolId();
+ doReturn(false).when(snapshotSchedulerImplSpy).isAccountRemovedOrDisabled(Mockito.any(), Mockito.any());
+ doReturn(snapshotPolicyVO).when(snapshotPolicyDaoMock).findById(Mockito.any());
boolean result = snapshotSchedulerImplSpy.canSnapshotBeScheduled(snapshotScheduleVO, volumeVoMock);
Assert.assertTrue(result);
- Mockito.verify(snapshotScheduleDaoMock, Mockito.never()).remove(Mockito.anyLong());
+ verify(snapshotScheduleDaoMock, never()).remove(Mockito.anyLong());
}
@Test
public void scheduleNextSnapshotJobIfNecessaryTestAsyncJobIsNullThenScheduleNextSnapshot() {
- Mockito.doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
- Mockito.doReturn(null).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
- Mockito.doReturn(new Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
+ doReturn(null).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
+ doReturn(new Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
snapshotSchedulerImplSpy.scheduleNextSnapshotJobIfNecessary(snapshotScheduleVoMock);
- Mockito.verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
}
@Test
public void scheduleNextSnapshotJobIfNecessaryTestAsyncJobSucceededThenScheduleNextSnapshot() {
- Mockito.doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
- Mockito.doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
- Mockito.doReturn(JobInfo.Status.SUCCEEDED).when(asyncJobVoMock).getStatus();
- Mockito.doReturn(new Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
+ doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
+ doReturn(JobInfo.Status.SUCCEEDED).when(asyncJobVoMock).getStatus();
+ doReturn(new Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
snapshotSchedulerImplSpy.scheduleNextSnapshotJobIfNecessary(snapshotScheduleVoMock);
- Mockito.verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
}
@Test
public void scheduleNextSnapshotJobIfNecessaryTestAsyncJobFailedThenScheduleNextSnapshot() {
- Mockito.doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
- Mockito.doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
- Mockito.doReturn(JobInfo.Status.FAILED).when(asyncJobVoMock).getStatus();
- Mockito.doReturn(new Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
+ doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
+ doReturn(JobInfo.Status.FAILED).when(asyncJobVoMock).getStatus();
+ doReturn(new Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
snapshotSchedulerImplSpy.scheduleNextSnapshotJobIfNecessary(snapshotScheduleVoMock);
- Mockito.verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
}
@Test
public void scheduleNextSnapshotJobIfNecessaryTestAsyncJobInProgressThenDoNothing() {
- Mockito.doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
- Mockito.doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
- Mockito.doReturn(JobInfo.Status.IN_PROGRESS).when(asyncJobVoMock).getStatus();
+ doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
+ doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
+ doReturn(JobInfo.Status.IN_PROGRESS).when(asyncJobVoMock).getStatus();
snapshotSchedulerImplSpy.scheduleNextSnapshotJobIfNecessary(snapshotScheduleVoMock);
- Mockito.verify(snapshotSchedulerImplSpy, Mockito.never()).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ verify(snapshotSchedulerImplSpy, never()).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ }
+
+ // --- countConsecutiveFailedAttempts (#13454) ---
+
+ @Test
+ public void countConsecutiveFailedAttemptsTestLimitZeroReturnsZeroWithoutQuerying() {
+ int result = snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 0);
+
+ Assert.assertEquals(0, result);
+ verify(eventDaoMock, never()).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
+ }
+
+ @Test
+ public void countConsecutiveFailedAttemptsTestStopsAtFirstNonErrorEvent() {
+ EventVO error1 = mock(EventVO.class);
+ doReturn(EventVO.LEVEL_ERROR).when(error1).getLevel();
+ EventVO error2 = mock(EventVO.class);
+ doReturn(EventVO.LEVEL_ERROR).when(error2).getLevel();
+ EventVO success = mock(EventVO.class);
+ doReturn(EventVO.LEVEL_INFO).when(success).getLevel();
+
+ doReturn(List.of(error1, error2, success)).when(eventDaoMock).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
+
+ int result = snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 3);
+
+ Assert.assertEquals(2, result);
+ }
+
+ @Test
+ public void countConsecutiveFailedAttemptsTestNoEventsReturnsZero() {
+ doReturn(Collections.emptyList()).when(eventDaoMock).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
+
+ int result = snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 3);
+
+ Assert.assertEquals(0, result);
+ }
+
+ // --- getScopedConfigValue (#13454) ---
+
+ @Test
+ public void getScopedConfigValueTestFallsBackToGlobalDefaultWhenNoScopeOverrideConfigured() {
+ doReturn(1L).when(volumeVoMock).getAccountId();
+ doReturn(1L).when(volumeVoMock).getDataCenterId();
+ doReturn(1L).when(accountVoMock).getDomainId();
+
+ Integer result = snapshotSchedulerImplSpy.getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volumeVoMock, accountVoMock);
+
+ Assert.assertEquals(Integer.valueOf(3), result);
+ }
+
Review Comment:
The tests only cover the global fallback case, but the new logic is specifically about precedence across Account/Domain/Zone/Global. Add unit tests that set different values per scope (especially Zone-only overrides) and assert the chosen value matches the documented precedence; this would also catch regressions like implicit fallback short-circuiting later scopes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5515511695
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743) **Quality Gate passed**
Issues
 [81 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [48.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5514595609
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5175506062
<b>[SF] Trillian Build Failed (tid-16691)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5551614181
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [71.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5538984717
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "DaanHoogland (via GitHub) via commits" <co...@cloudstack.apache.org>.
DaanHoogland commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-6763788602
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub) via commits" <co...@cloudstack.apache.org>.
sonarqubecloud[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-6765181029
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13743&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13743&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [73.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13743&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13743)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5515120265
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19071
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5508065741
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5523048817
<b>[SF] Trillian Build Failed (tid-16882)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-5528671181
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-6487201839
Hi @DaanHoogland is this PR ready for review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "Copilot (via GitHub) via commits" <co...@cloudstack.apache.org>.
Copilot commented on code in PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#discussion_r4445194368
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java:
##########
@@ -53,6 +55,18 @@ public interface SnapshotManager extends Configurable {
public static final ConfigKey<Integer> BackupRetryInterval = new ConfigKey<Integer>(Integer.class, "backup.retry.interval", "Advanced", "300",
"Time in seconds between retries in backing up snapshot to secondary", false, ConfigKey.Scope.Global, null);
+ ConfigKey<Integer> SnapshotRecurringMaxFailures = new ConfigKey<>(Integer.class, "snapshot.recurring.max.failures", "Snapshots", "3",
+ "Maximum number of consecutive failed attempts allowed for a recurring snapshot before it is left until its next regularly scheduled run and a failure event is logged. Set to 0 to retry indefinitely.",
+ true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
+
+ ConfigKey<Integer> SnapshotRecurringRetryInterval = new ConfigKey<>(Integer.class, "snapshot.recurring.retry.interval", "Snapshots", "300",
+ "Time in seconds to wait before retrying a failed recurring snapshot attempt.",
+ true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
+
+ ConfigKey<Boolean> SnapshotSkipIfVmNotRunning = new ConfigKey<>(Boolean.class, "snapshot.skip.if.vm.not.running", "Snapshots", "false",
+ "For recurring snapshots, skip taking a new snapshot of a volume (and log a skipped event instead) when the VM it is attached to has not been running since the last snapshot was taken, since nothing on the volume could have changed.",
+ true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
Review Comment:
Including `Scope.Global` alongside non-global scopes makes `ConfigDepotImpl.populateConfiguration()` dereference a missing entry in its scope map, aborting management-server startup. Remove `Scope.Global` from all three new scope lists (also lines 64 and 68); global values are supported implicitly.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +320,230 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Returns true (after rescheduling as needed) when this iteration's snapshot should not be dispatched: either
+ * because it can't be scheduled at all, or because it's a redundant snapshot of an unchanged volume that gets
+ * skipped and rescheduled to its next regular run instead. Kept as a single decision point so the caller only
+ * needs one {@code continue}.
+ */
+ protected boolean shouldSkipSchedule(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume) {
+ if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ return true;
+ }
+ if (shouldSkipUnchangedVolumeSnapshot(volume)) {
+ skipAndRescheduleSnapshot(snapshotToBeExecuted, volume);
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Logs the "scheduled" action event for a recurring snapshot of the volume and returns its id, which
+ * {@link #dispatchSnapshotCreateJob} passes on to the job and {@link #handleFailedSnapshotDispatch} completes if
+ * dispatching fails.
+ */
+ protected Long createScheduledSnapshotEvent(final VolumeVO volume) {
+ return ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for volume Id:" +
+ volume.getUuid(), volume.getId(), ApiCommandResourceType.Volume.toString(), true, 0);
+ }
+
+ /**
+ * Builds and dispatches the CreateSnapshotCmd async job for a scheduled snapshot, started from the given
+ * "scheduled" action event.
+ */
+ protected void dispatchSnapshotCreateJob(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO tmpSnapshotScheduleVO,
+ final Long eventId) throws Exception {
+ final long snapshotScheId = snapshotToBeExecuted.getId();
+ final long policyId = snapshotToBeExecuted.getPolicyId();
+ final long volumeId = snapshotToBeExecuted.getVolumeId();
+
+ logger.trace("Mapping parameters required to generate a CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
+ final Map<String, String> params = new HashMap<String, String>();
+ params.put(ApiConstants.VOLUME_ID, "" + volumeId);
+ params.put(ApiConstants.POLICY_ID, "" + policyId);
+ params.put("ctxUserId", "1");
+ params.put("ctxAccountId", "" + volume.getAccountId());
+ params.put("ctxStartEventId", String.valueOf(eventId));
+ List<? extends ResourceTag> resourceTags = taggedResourceService.listByResourceTypeAndId(ResourceTag.ResourceObjectType.SnapshotPolicy, policyId);
+ if (resourceTags != null && !resourceTags.isEmpty()) {
+ int tagNumber = 0;
+ for (ResourceTag resourceTag : resourceTags) {
+ params.put("tags[" + tagNumber + "].key", resourceTag.getKey());
+ params.put("tags[" + tagNumber + "].value", resourceTag.getValue());
+ tagNumber++;
+ }
+ }
+
+ logger.trace("Generating a CreateSnapshotCmd for snapshot [{}] with parameters: [{}].", snapshotToBeExecuted, params.toString());
+ final CreateSnapshotCmd cmd = new CreateSnapshotCmd();
+ ComponentContext.inject(cmd);
+ _dispatcher.dispatchCreateCmd(cmd, params);
+ params.put("id", "" + cmd.getEntityId());
+ params.put("ctxStartEventId", "1");
+
+ final Date scheduledTimestamp = snapshotToBeExecuted.getScheduledTimestamp();
+ final String displayTime = DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, scheduledTimestamp);
+ logger.debug("Scheduling snapshot [{}] for volume [{}] at [{}].", snapshotToBeExecuted, volume, displayTime);
+ AsyncJobVO job = new AsyncJobVO("", User.UID_SYSTEM, volume.getAccountId(), CreateSnapshotCmd.class.getName(),
+ ApiGsonHelper.getBuilder().create().toJson(params), cmd.getEntityId(),
+ cmd.getApiResourceType() != null ? cmd.getApiResourceType().toString() : null, null);
+ job.setDispatcher(_asyncDispatcher.getName());
+ final long jobId = _asyncMgr.submitAsyncJob(job);
+ logger.debug("Scheduled snapshot [{}] for volume [{}] as job [{}].", snapshotToBeExecuted, volume, job);
+
+ tmpSnapshotScheduleVO.setAsyncJobId(jobId);
+ _snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
+ }
+
+ /**
+ * Handles a synchronous failure to dispatch the CreateSnapshotCmd (e.g. an allocation error) for a recurring
+ * snapshot. Without this, the schedule's {@code scheduledTimestamp} is never advanced, so it gets retried on
+ * every poll (every {@code snapshot.poll.interval} seconds) forever. Instead: log a failure event keyed by the
+ * volume (so consecutive failures can be counted from event history), and either back off by the configured
+ * retry interval, or - once the configured maximum consecutive failures is reached - give up until the next
+ * regularly scheduled run and raise a WARN notification event.
+ */
+ protected void handleFailedSnapshotDispatch(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume,
+ final SnapshotScheduleVO lockedSchedule, final Long eventId, final Exception cause) {
+ final long volumeId = volume.getId();
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures = getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume, account);
+ final int retryInterval = getScopedConfigValue(SnapshotManager.SnapshotRecurringRetryInterval, volume, account);
+ final int totalFailures = countConsecutiveFailedAttempts(volumeId, maxFailures) + 1;
+
+ final String failureMessage = String.format("Failed to create scheduled snapshot for volume [%s]: %s", volume, cause.getMessage());
+ if (eventId != null) {
+ ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, failureMessage, volumeId, ApiCommandResourceType.Volume.toString(), eventId);
+ } else {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, true, failureMessage, volumeId, ApiCommandResourceType.Volume.toString());
+ }
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ final Date nextRegularRun = getNextScheduledTime(snapshotToBeExecuted.getPolicyId(), _currentTimestamp);
+ lockedSchedule.setScheduledTimestamp(nextRegularRun);
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed [{}] consecutive times; it will not be retried until its next regularly scheduled run at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRegularRun);
+ raiseFailureLimitReachedEvent(volume, String.format(
+ "Recurring snapshot for volume [%s] has failed %d consecutive times and will not be retried until its next regularly scheduled run.", volume, totalFailures));
+ } else {
+ final Date nextRetry = new Date(_currentTimestamp.getTime() + retryInterval * 1000L);
+ lockedSchedule.setScheduledTimestamp(nextRetry);
+ logger.debug("Snapshot schedule [{}] for volume [{}] failed [{}] time(s); retrying at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRetry);
+ }
+ _snapshotScheduleDao.update(lockedSchedule.getId(), lockedSchedule);
+ }
+
+ /**
+ * Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} outcome events logged for this volume are
+ * consecutive failures (level ERROR), starting from the most recent event and stopping at the first
+ * non-failure (or absent) event. The Scheduled event logged before every dispatch is not an outcome and is
+ * excluded by the DAO, otherwise it would interrupt every run of failures. This derives the "number of failed attempts" from event history instead of a
+ * dedicated counter column.
+ */
+ protected int countConsecutiveFailedAttempts(final long volumeId, final int limit) {
+ if (limit <= 0) {
+ return 0;
+ }
+ final List<EventVO> recentEvents = eventDao.listLatestOutcomeEventsByResource(volumeId, ApiCommandResourceType.Volume.toString(),
+ EventTypes.EVENT_SNAPSHOT_CREATE, limit);
+ int count = 0;
+ for (final EventVO event : recentEvents) {
+ if (!EventVO.LEVEL_ERROR.equals(event.getLevel())) {
+ break;
+ }
+ count++;
+ }
+ return count;
+ }
+
+ /**
+ * Resolves a config value in order of most to least specific scope: account, domain, zone, then global. A
+ * {@link ConfigKey} can only walk a single scope-parent chain automatically (Account->Domain->Global, or
+ * Zone->Global), so the four scopes are resolved manually here.
+ */
+ protected <T> T getScopedConfigValue(final ConfigKey<T> key, final VolumeVO volume, final Account account) {
+ T value = key.valueInScope(ConfigKey.Scope.Account, volume.getAccountId(), true);
+ if (value == null && account != null) {
+ value = key.valueInScope(ConfigKey.Scope.Domain, account.getDomainId(), true);
+ }
+ if (value == null) {
+ value = key.valueInScope(ConfigKey.Scope.Zone, volume.getDataCenterId(), true);
+ }
+ if (value == null) {
+ value = key.value();
+ }
+ return value;
+ }
+
+ /**
+ * Implements https://github.com/apache/cloudstack/issues/6827: a recurring snapshot is redundant when nothing
+ * could have changed on the volume since the last one was taken, i.e. when the attached VM has not been running
+ * at any point since then. Volumes with no snapshot yet, or that are not attached to a VM, are never skipped.
+ */
+ protected boolean shouldSkipUnchangedVolumeSnapshot(final VolumeVO volume) {
+ final Account account = _acctDao.findById(volume.getAccountId());
+ if (!Boolean.TRUE.equals(getScopedConfigValue(SnapshotManager.SnapshotSkipIfVmNotRunning, volume, account))) {
+ return false;
+ }
+
+ final Long instanceId = volume.getInstanceId();
+ if (instanceId == null) {
+ return false;
+ }
+
+ final SnapshotVO lastSnapshot = findLastSnapshot(volume.getId());
+ if (lastSnapshot == null) {
+ return false;
+ }
+
+ final VMInstanceVO vm = vmInstanceDao.findById(instanceId);
+ if (vm == null || vm.getPowerState() == VirtualMachine.PowerState.PowerOn) {
Review Comment:
This also treats `PowerUnknown` as powered off. If that state predates the last snapshot, the timestamp check below skips future recurring snapshots even though an unknown power state does not establish that the VM stopped running. Please require a confirmed powered-off state before using its timestamp to suppress backups.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +320,230 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Returns true (after rescheduling as needed) when this iteration's snapshot should not be dispatched: either
+ * because it can't be scheduled at all, or because it's a redundant snapshot of an unchanged volume that gets
+ * skipped and rescheduled to its next regular run instead. Kept as a single decision point so the caller only
+ * needs one {@code continue}.
+ */
+ protected boolean shouldSkipSchedule(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume) {
+ if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ return true;
+ }
+ if (shouldSkipUnchangedVolumeSnapshot(volume)) {
+ skipAndRescheduleSnapshot(snapshotToBeExecuted, volume);
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Logs the "scheduled" action event for a recurring snapshot of the volume and returns its id, which
+ * {@link #dispatchSnapshotCreateJob} passes on to the job and {@link #handleFailedSnapshotDispatch} completes if
+ * dispatching fails.
+ */
+ protected Long createScheduledSnapshotEvent(final VolumeVO volume) {
+ return ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for volume Id:" +
+ volume.getUuid(), volume.getId(), ApiCommandResourceType.Volume.toString(), true, 0);
+ }
+
+ /**
+ * Builds and dispatches the CreateSnapshotCmd async job for a scheduled snapshot, started from the given
+ * "scheduled" action event.
+ */
+ protected void dispatchSnapshotCreateJob(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO tmpSnapshotScheduleVO,
+ final Long eventId) throws Exception {
+ final long snapshotScheId = snapshotToBeExecuted.getId();
+ final long policyId = snapshotToBeExecuted.getPolicyId();
+ final long volumeId = snapshotToBeExecuted.getVolumeId();
+
+ logger.trace("Mapping parameters required to generate a CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
+ final Map<String, String> params = new HashMap<String, String>();
+ params.put(ApiConstants.VOLUME_ID, "" + volumeId);
+ params.put(ApiConstants.POLICY_ID, "" + policyId);
+ params.put("ctxUserId", "1");
+ params.put("ctxAccountId", "" + volume.getAccountId());
+ params.put("ctxStartEventId", String.valueOf(eventId));
+ List<? extends ResourceTag> resourceTags = taggedResourceService.listByResourceTypeAndId(ResourceTag.ResourceObjectType.SnapshotPolicy, policyId);
+ if (resourceTags != null && !resourceTags.isEmpty()) {
+ int tagNumber = 0;
+ for (ResourceTag resourceTag : resourceTags) {
+ params.put("tags[" + tagNumber + "].key", resourceTag.getKey());
+ params.put("tags[" + tagNumber + "].value", resourceTag.getValue());
+ tagNumber++;
+ }
+ }
+
+ logger.trace("Generating a CreateSnapshotCmd for snapshot [{}] with parameters: [{}].", snapshotToBeExecuted, params.toString());
+ final CreateSnapshotCmd cmd = new CreateSnapshotCmd();
+ ComponentContext.inject(cmd);
+ _dispatcher.dispatchCreateCmd(cmd, params);
+ params.put("id", "" + cmd.getEntityId());
+ params.put("ctxStartEventId", "1");
+
+ final Date scheduledTimestamp = snapshotToBeExecuted.getScheduledTimestamp();
+ final String displayTime = DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, scheduledTimestamp);
+ logger.debug("Scheduling snapshot [{}] for volume [{}] at [{}].", snapshotToBeExecuted, volume, displayTime);
+ AsyncJobVO job = new AsyncJobVO("", User.UID_SYSTEM, volume.getAccountId(), CreateSnapshotCmd.class.getName(),
+ ApiGsonHelper.getBuilder().create().toJson(params), cmd.getEntityId(),
+ cmd.getApiResourceType() != null ? cmd.getApiResourceType().toString() : null, null);
+ job.setDispatcher(_asyncDispatcher.getName());
+ final long jobId = _asyncMgr.submitAsyncJob(job);
+ logger.debug("Scheduled snapshot [{}] for volume [{}] as job [{}].", snapshotToBeExecuted, volume, job);
+
+ tmpSnapshotScheduleVO.setAsyncJobId(jobId);
+ _snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
+ }
+
+ /**
+ * Handles a synchronous failure to dispatch the CreateSnapshotCmd (e.g. an allocation error) for a recurring
+ * snapshot. Without this, the schedule's {@code scheduledTimestamp} is never advanced, so it gets retried on
+ * every poll (every {@code snapshot.poll.interval} seconds) forever. Instead: log a failure event keyed by the
+ * volume (so consecutive failures can be counted from event history), and either back off by the configured
+ * retry interval, or - once the configured maximum consecutive failures is reached - give up until the next
+ * regularly scheduled run and raise a WARN notification event.
+ */
+ protected void handleFailedSnapshotDispatch(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume,
+ final SnapshotScheduleVO lockedSchedule, final Long eventId, final Exception cause) {
+ final long volumeId = volume.getId();
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures = getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume, account);
+ final int retryInterval = getScopedConfigValue(SnapshotManager.SnapshotRecurringRetryInterval, volume, account);
+ final int totalFailures = countConsecutiveFailedAttempts(volumeId, maxFailures) + 1;
+
+ final String failureMessage = String.format("Failed to create scheduled snapshot for volume [%s]: %s", volume, cause.getMessage());
+ if (eventId != null) {
+ ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, failureMessage, volumeId, ApiCommandResourceType.Volume.toString(), eventId);
+ } else {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, true, failureMessage, volumeId, ApiCommandResourceType.Volume.toString());
+ }
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ final Date nextRegularRun = getNextScheduledTime(snapshotToBeExecuted.getPolicyId(), _currentTimestamp);
+ lockedSchedule.setScheduledTimestamp(nextRegularRun);
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed [{}] consecutive times; it will not be retried until its next regularly scheduled run at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRegularRun);
+ raiseFailureLimitReachedEvent(volume, String.format(
+ "Recurring snapshot for volume [%s] has failed %d consecutive times and will not be retried until its next regularly scheduled run.", volume, totalFailures));
+ } else {
+ final Date nextRetry = new Date(_currentTimestamp.getTime() + retryInterval * 1000L);
+ lockedSchedule.setScheduledTimestamp(nextRetry);
+ logger.debug("Snapshot schedule [{}] for volume [{}] failed [{}] time(s); retrying at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRetry);
+ }
+ _snapshotScheduleDao.update(lockedSchedule.getId(), lockedSchedule);
+ }
+
+ /**
+ * Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} outcome events logged for this volume are
+ * consecutive failures (level ERROR), starting from the most recent event and stopping at the first
+ * non-failure (or absent) event. The Scheduled event logged before every dispatch is not an outcome and is
+ * excluded by the DAO, otherwise it would interrupt every run of failures. This derives the "number of failed attempts" from event history instead of a
+ * dedicated counter column.
+ */
+ protected int countConsecutiveFailedAttempts(final long volumeId, final int limit) {
+ if (limit <= 0) {
+ return 0;
+ }
+ final List<EventVO> recentEvents = eventDao.listLatestOutcomeEventsByResource(volumeId, ApiCommandResourceType.Volume.toString(),
+ EventTypes.EVENT_SNAPSHOT_CREATE, limit);
+ int count = 0;
+ for (final EventVO event : recentEvents) {
+ if (!EventVO.LEVEL_ERROR.equals(event.getLevel())) {
+ break;
+ }
+ count++;
+ }
+ return count;
+ }
+
+ /**
+ * Resolves a config value in order of most to least specific scope: account, domain, zone, then global. A
+ * {@link ConfigKey} can only walk a single scope-parent chain automatically (Account->Domain->Global, or
+ * Zone->Global), so the four scopes are resolved manually here.
+ */
+ protected <T> T getScopedConfigValue(final ConfigKey<T> key, final VolumeVO volume, final Account account) {
+ T value = key.valueInScope(ConfigKey.Scope.Account, volume.getAccountId(), true);
+ if (value == null && account != null) {
+ value = key.valueInScope(ConfigKey.Scope.Domain, account.getDomainId(), true);
+ }
+ if (value == null) {
+ value = key.valueInScope(ConfigKey.Scope.Zone, volume.getDataCenterId(), true);
+ }
+ if (value == null) {
+ value = key.value();
+ }
+ return value;
+ }
+
+ /**
+ * Implements https://github.com/apache/cloudstack/issues/6827: a recurring snapshot is redundant when nothing
+ * could have changed on the volume since the last one was taken, i.e. when the attached VM has not been running
+ * at any point since then. Volumes with no snapshot yet, or that are not attached to a VM, are never skipped.
+ */
+ protected boolean shouldSkipUnchangedVolumeSnapshot(final VolumeVO volume) {
+ final Account account = _acctDao.findById(volume.getAccountId());
+ if (!Boolean.TRUE.equals(getScopedConfigValue(SnapshotManager.SnapshotSkipIfVmNotRunning, volume, account))) {
+ return false;
+ }
+
+ final Long instanceId = volume.getInstanceId();
+ if (instanceId == null) {
+ return false;
+ }
+
+ final SnapshotVO lastSnapshot = findLastSnapshot(volume.getId());
+ if (lastSnapshot == null) {
+ return false;
+ }
+
+ final VMInstanceVO vm = vmInstanceDao.findById(instanceId);
+ if (vm == null || vm.getPowerState() == VirtualMachine.PowerState.PowerOn) {
+ return false;
+ }
+
+ final Date poweredOffSince = vm.getPowerStateUpdateTime();
+ // If the VM's power state changed at or after the last snapshot, it may have been running (and the volume
+ // may have changed) at some point since; only skip when it has been off since strictly before that snapshot.
+ return poweredOffSince != null && poweredOffSince.before(lastSnapshot.getCreated());
Review Comment:
If a data volume is modified after its snapshot on VM A, then attached to VM B that has been off since before that snapshot, this check skips backups of those unprotected writes. Also require the current attachment to predate the snapshot, and keep taking snapshots when the attachment time is unknown; `VolumeDaoImpl.attachVolume()` records that time in `attached`.
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -125,6 +137,20 @@ public List<EventVO> listToArchiveOrDeleteEvents(List<Long> ids, String type, Da
return search(sc, null);
}
+ @Override
+ public List<EventVO> listLatestOutcomeEventsByResource(long resourceId, String resourceType, String type, int limit) {
+ SearchCriteria<EventVO> sc = LatestEventsByResourceSearch.create();
+ sc.setParameters(FIELD_RESOURCE_ID, resourceId);
+ sc.setParameters(FIELD_RESOURCE_TYPE, resourceType);
+ sc.setParameters("type", type);
+ sc.setParameters("state", State.Created, State.Completed);
+ sc.setParameters("archived", false);
+ Filter filter = new Filter(EventVO.class, "createDate", false, 0L, (long) limit);
+ // created has one-second granularity; break ties by insertion order
+ filter.addOrderBy(EventVO.class, "id", false);
+ return listBy(sc, filter);
Review Comment:
The event table has no resource-scoped index, so a failed volume with no matching outcome history can require scanning retained events despite the `LIMIT`. These lookups run under the global `snapshot.poll` lock, so many failing volumes can delay all snapshot scheduling. Add a schema migration with a composite index supporting the resource/type predicates and newest-first ordering.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +320,230 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Returns true (after rescheduling as needed) when this iteration's snapshot should not be dispatched: either
+ * because it can't be scheduled at all, or because it's a redundant snapshot of an unchanged volume that gets
+ * skipped and rescheduled to its next regular run instead. Kept as a single decision point so the caller only
+ * needs one {@code continue}.
+ */
+ protected boolean shouldSkipSchedule(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume) {
+ if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ return true;
+ }
+ if (shouldSkipUnchangedVolumeSnapshot(volume)) {
+ skipAndRescheduleSnapshot(snapshotToBeExecuted, volume);
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Logs the "scheduled" action event for a recurring snapshot of the volume and returns its id, which
+ * {@link #dispatchSnapshotCreateJob} passes on to the job and {@link #handleFailedSnapshotDispatch} completes if
+ * dispatching fails.
+ */
+ protected Long createScheduledSnapshotEvent(final VolumeVO volume) {
+ return ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for volume Id:" +
+ volume.getUuid(), volume.getId(), ApiCommandResourceType.Volume.toString(), true, 0);
+ }
+
+ /**
+ * Builds and dispatches the CreateSnapshotCmd async job for a scheduled snapshot, started from the given
+ * "scheduled" action event.
+ */
+ protected void dispatchSnapshotCreateJob(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO tmpSnapshotScheduleVO,
+ final Long eventId) throws Exception {
+ final long snapshotScheId = snapshotToBeExecuted.getId();
+ final long policyId = snapshotToBeExecuted.getPolicyId();
+ final long volumeId = snapshotToBeExecuted.getVolumeId();
+
+ logger.trace("Mapping parameters required to generate a CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
+ final Map<String, String> params = new HashMap<String, String>();
+ params.put(ApiConstants.VOLUME_ID, "" + volumeId);
+ params.put(ApiConstants.POLICY_ID, "" + policyId);
+ params.put("ctxUserId", "1");
+ params.put("ctxAccountId", "" + volume.getAccountId());
+ params.put("ctxStartEventId", String.valueOf(eventId));
+ List<? extends ResourceTag> resourceTags = taggedResourceService.listByResourceTypeAndId(ResourceTag.ResourceObjectType.SnapshotPolicy, policyId);
+ if (resourceTags != null && !resourceTags.isEmpty()) {
+ int tagNumber = 0;
+ for (ResourceTag resourceTag : resourceTags) {
+ params.put("tags[" + tagNumber + "].key", resourceTag.getKey());
+ params.put("tags[" + tagNumber + "].value", resourceTag.getValue());
+ tagNumber++;
+ }
+ }
+
+ logger.trace("Generating a CreateSnapshotCmd for snapshot [{}] with parameters: [{}].", snapshotToBeExecuted, params.toString());
+ final CreateSnapshotCmd cmd = new CreateSnapshotCmd();
+ ComponentContext.inject(cmd);
+ _dispatcher.dispatchCreateCmd(cmd, params);
+ params.put("id", "" + cmd.getEntityId());
+ params.put("ctxStartEventId", "1");
+
+ final Date scheduledTimestamp = snapshotToBeExecuted.getScheduledTimestamp();
+ final String displayTime = DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, scheduledTimestamp);
+ logger.debug("Scheduling snapshot [{}] for volume [{}] at [{}].", snapshotToBeExecuted, volume, displayTime);
+ AsyncJobVO job = new AsyncJobVO("", User.UID_SYSTEM, volume.getAccountId(), CreateSnapshotCmd.class.getName(),
+ ApiGsonHelper.getBuilder().create().toJson(params), cmd.getEntityId(),
+ cmd.getApiResourceType() != null ? cmd.getApiResourceType().toString() : null, null);
+ job.setDispatcher(_asyncDispatcher.getName());
+ final long jobId = _asyncMgr.submitAsyncJob(job);
+ logger.debug("Scheduled snapshot [{}] for volume [{}] as job [{}].", snapshotToBeExecuted, volume, job);
+
+ tmpSnapshotScheduleVO.setAsyncJobId(jobId);
+ _snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
+ }
+
+ /**
+ * Handles a synchronous failure to dispatch the CreateSnapshotCmd (e.g. an allocation error) for a recurring
+ * snapshot. Without this, the schedule's {@code scheduledTimestamp} is never advanced, so it gets retried on
+ * every poll (every {@code snapshot.poll.interval} seconds) forever. Instead: log a failure event keyed by the
+ * volume (so consecutive failures can be counted from event history), and either back off by the configured
+ * retry interval, or - once the configured maximum consecutive failures is reached - give up until the next
+ * regularly scheduled run and raise a WARN notification event.
+ */
+ protected void handleFailedSnapshotDispatch(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume,
+ final SnapshotScheduleVO lockedSchedule, final Long eventId, final Exception cause) {
+ final long volumeId = volume.getId();
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures = getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume, account);
+ final int retryInterval = getScopedConfigValue(SnapshotManager.SnapshotRecurringRetryInterval, volume, account);
+ final int totalFailures = countConsecutiveFailedAttempts(volumeId, maxFailures) + 1;
+
+ final String failureMessage = String.format("Failed to create scheduled snapshot for volume [%s]: %s", volume, cause.getMessage());
+ if (eventId != null) {
+ ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, failureMessage, volumeId, ApiCommandResourceType.Volume.toString(), eventId);
+ } else {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, true, failureMessage, volumeId, ApiCommandResourceType.Volume.toString());
+ }
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ final Date nextRegularRun = getNextScheduledTime(snapshotToBeExecuted.getPolicyId(), _currentTimestamp);
+ lockedSchedule.setScheduledTimestamp(nextRegularRun);
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed [{}] consecutive times; it will not be retried until its next regularly scheduled run at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRegularRun);
+ raiseFailureLimitReachedEvent(volume, String.format(
+ "Recurring snapshot for volume [%s] has failed %d consecutive times and will not be retried until its next regularly scheduled run.", volume, totalFailures));
+ } else {
+ final Date nextRetry = new Date(_currentTimestamp.getTime() + retryInterval * 1000L);
+ lockedSchedule.setScheduledTimestamp(nextRetry);
+ logger.debug("Snapshot schedule [{}] for volume [{}] failed [{}] time(s); retrying at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRetry);
+ }
+ _snapshotScheduleDao.update(lockedSchedule.getId(), lockedSchedule);
+ }
+
+ /**
+ * Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} outcome events logged for this volume are
+ * consecutive failures (level ERROR), starting from the most recent event and stopping at the first
+ * non-failure (or absent) event. The Scheduled event logged before every dispatch is not an outcome and is
+ * excluded by the DAO, otherwise it would interrupt every run of failures. This derives the "number of failed attempts" from event history instead of a
+ * dedicated counter column.
+ */
+ protected int countConsecutiveFailedAttempts(final long volumeId, final int limit) {
+ if (limit <= 0) {
+ return 0;
+ }
+ final List<EventVO> recentEvents = eventDao.listLatestOutcomeEventsByResource(volumeId, ApiCommandResourceType.Volume.toString(),
+ EventTypes.EVENT_SNAPSHOT_CREATE, limit);
+ int count = 0;
+ for (final EventVO event : recentEvents) {
+ if (!EventVO.LEVEL_ERROR.equals(event.getLevel())) {
+ break;
+ }
+ count++;
+ }
+ return count;
+ }
+
+ /**
+ * Resolves a config value in order of most to least specific scope: account, domain, zone, then global. A
+ * {@link ConfigKey} can only walk a single scope-parent chain automatically (Account->Domain->Global, or
+ * Zone->Global), so the four scopes are resolved manually here.
+ */
+ protected <T> T getScopedConfigValue(final ConfigKey<T> key, final VolumeVO volume, final Account account) {
+ T value = key.valueInScope(ConfigKey.Scope.Account, volume.getAccountId(), true);
+ if (value == null && account != null) {
+ value = key.valueInScope(ConfigKey.Scope.Domain, account.getDomainId(), true);
+ }
+ if (value == null) {
+ value = key.valueInScope(ConfigKey.Scope.Zone, volume.getDataCenterId(), true);
+ }
+ if (value == null) {
+ value = key.value();
+ }
+ return value;
+ }
+
+ /**
+ * Implements https://github.com/apache/cloudstack/issues/6827: a recurring snapshot is redundant when nothing
+ * could have changed on the volume since the last one was taken, i.e. when the attached VM has not been running
+ * at any point since then. Volumes with no snapshot yet, or that are not attached to a VM, are never skipped.
+ */
+ protected boolean shouldSkipUnchangedVolumeSnapshot(final VolumeVO volume) {
+ final Account account = _acctDao.findById(volume.getAccountId());
+ if (!Boolean.TRUE.equals(getScopedConfigValue(SnapshotManager.SnapshotSkipIfVmNotRunning, volume, account))) {
+ return false;
+ }
+
+ final Long instanceId = volume.getInstanceId();
+ if (instanceId == null) {
+ return false;
+ }
+
+ final SnapshotVO lastSnapshot = findLastSnapshot(volume.getId());
+ if (lastSnapshot == null) {
+ return false;
+ }
+
+ final VMInstanceVO vm = vmInstanceDao.findById(instanceId);
+ if (vm == null || vm.getPowerState() == VirtualMachine.PowerState.PowerOn) {
+ return false;
+ }
+
+ final Date poweredOffSince = vm.getPowerStateUpdateTime();
+ // If the VM's power state changed at or after the last snapshot, it may have been running (and the volume
+ // may have changed) at some point since; only skip when it has been off since strictly before that snapshot.
+ return poweredOffSince != null && poweredOffSince.before(lastSnapshot.getCreated());
+ }
+
+ protected SnapshotVO findLastSnapshot(final long volumeId) {
+ final Filter filter = new Filter(SnapshotVO.class, "created", false, 0L, 1L);
+ final List<SnapshotVO> snapshots = _snapshotDao.listByVolumeId(filter, volumeId);
Review Comment:
`listByVolumeId` does not filter snapshot state, so this can return an `Error` or merely `Allocated` snapshot left by a failed attempt. With skipping enabled, a stopped VM's failed first snapshot then becomes the baseline, and every subsequent recurring run is skipped despite there being no usable snapshot. Please restrict the query to successfully taken snapshots before applying the one-row limit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with excessive attempts on failing snapshots [cloudstack]
Posted by "blueorangutan (via GitHub) via commits" <co...@cloudstack.apache.org>.
blueorangutan commented on PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#issuecomment-6763804610
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org