Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc(backup): add manual tests of multiple backup targets #2238

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

mantissahz
Copy link
Contributor

@mantissahz mantissahz commented Dec 30, 2024

Which issue(s) this PR fixes:

Issue # longhorn/longhorn#5411

What this PR does / why we need it:

Manual Test Cases for multiple backup targets.

Special notes for your reviewer:

Additional documentation or context

Summary by CodeRabbit

  • Documentation
    • Added a comprehensive testing guide for managing multiple backup stores in Longhorn.
    • Detailed test scenarios for backup target creation, restoration, and synchronization.
    • Provided guidelines for testing backup functionality across different configurations and upgrade paths.
    • Included verification procedures for backup integrity and resource cleanup during uninstallation.

@mantissahz mantissahz self-assigned this Dec 30, 2024
@mantissahz mantissahz requested a review from a team as a code owner December 30, 2024 17:04
Copy link

coderabbitai bot commented Dec 30, 2024

Walkthrough

This pull request introduces a comprehensive testing guide for managing multiple backup stores in a Longhorn cluster environment. The new document, located in docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md, provides detailed test scenarios and procedures for verifying the functionality of multiple backup stores. The guide covers various aspects such as creating volumes, managing backup targets, performing backups and restores, and ensuring data integrity across different backup scenarios.

Changes

File Change Summary
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md New document added with comprehensive testing guide for multiple backup stores in Longhorn

Possibly related PRs

Suggested reviewers

  • yangchiu
  • c3y1huang
  • roger-ryao
  • chriscchien

Poem

🐰 Backup Bunny's Ballad of Storage Delight
Multiple stores, a Longhorn's might,
Testing paths both wrong and right,
Volumes dance from here to there,
Backups sync with utmost care,
A rabbit's guide to data's flight! 🔒📦


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md (4)

7-11: Format URLs using proper markdown syntax

Convert bare URLs to proper markdown links for better readability.

-https://github.com/longhorn/longhorn/issues/5411
+[longhorn#5411](https://github.com/longhorn/longhorn/issues/5411)

-https://github.com/longhorn/longhorn/pull/6630
+[longhorn#6630](https://github.com/longhorn/longhorn/pull/6630)
🧰 Tools
🪛 Markdownlint (0.37.0)

7-7: null
Bare URL used

(MD034, no-bare-urls)


11-11: null
Bare URL used

(MD034, no-bare-urls)


51-51: Use "cannot" instead of "can not"

For better readability and consistency with standard English.

-The volume C can not be created.
+The volume C cannot be created.
🧰 Tools
🪛 LanguageTool

[style] ~51-~51: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...orage Class C. Verify The volume C can not be created. ## Test Create And Restore...

(CAN_NOT_PREMIUM)


77-81: Add data validation steps for backup restoration

The verification steps should specify how to validate the restored data. Consider adding:

  • Steps to compare the restored data with original data
  • Checksum verification procedure
  • Volume size verification

1-267: Overall assessment: Comprehensive test coverage with minor improvements needed

The test documentation provides thorough coverage of multiple backup target functionality. The Given-Then-Verify format is clear and consistent. However, consider:

  1. Using consistent markdown formatting throughout
  2. Adding more specific data validation steps
  3. Including error scenarios and recovery procedures
  4. Documenting expected timeouts and retry mechanisms

The document is approved with the suggested improvements.

🧰 Tools
🪛 LanguageTool

[style] ~51-~51: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...orage Class C. Verify The volume C can not be created. ## Test Create And Restore...

(CAN_NOT_PREMIUM)


[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...And** Create a volume and attach it. And Write data to the volume. Then C...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~143-~143: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ote backup store S for two clusters. And Create a volume A with the default ba...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~145-~145: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...p target in cluster A and attach it. And Create a volume B with the backup tar...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~147-~147: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rget E in cluster B and attach it. And Write data to the volume A and B. **...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~169-~169: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...01 is synchronized in the cluster A. Then Write data to the volume B and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~183-~183: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...default backup target and attach it. And Create a volume B with the backup tar...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~185-~185: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...the backup target A and attach it. And Write data to the volume A and B. **...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~243-~243: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...up targets are deleted successfully. Verify Uninstall successfully. ## Test Upgr...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~253-~253: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., and wait for the backup completed. And Create a backing image BI, and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~261-~261: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lare removed from global settings. **Verify** The fieldsSpec.BackupTargetName` of...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~263-~263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efault backup target name default. Verify The field Status.BackupTarget and l...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🪛 Markdownlint (0.37.0)

7-7: null
Bare URL used

(MD034, no-bare-urls)


11-11: null
Bare URL used

(MD034, no-bare-urls)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 969d4a7 and ca84e9a.

📒 Files selected for processing (1)
  • docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md

[style] ~51-~51: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...orage Class C. Verify The volume C can not be created. ## Test Create And Restore...

(CAN_NOT_PREMIUM)


[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...And** Create a volume and attach it. And Write data to the volume. Then C...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~143-~143: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ote backup store S for two clusters. And Create a volume A with the default ba...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~145-~145: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...p target in cluster A and attach it. And Create a volume B with the backup tar...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~147-~147: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rget E in cluster B and attach it. And Write data to the volume A and B. **...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~169-~169: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...01 is synchronized in the cluster A. Then Write data to the volume B and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~183-~183: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...default backup target and attach it. And Create a volume B with the backup tar...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~185-~185: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...the backup target A and attach it. And Write data to the volume A and B. **...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~243-~243: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...up targets are deleted successfully. Verify Uninstall successfully. ## Test Upgr...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~253-~253: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., and wait for the backup completed. And Create a backing image BI, and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~261-~261: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lare removed from global settings. **Verify** The fieldsSpec.BackupTargetName` of...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~263-~263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efault backup target name default. Verify The field Status.BackupTarget and l...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🪛 Markdownlint (0.37.0)
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md

7-7: null
Bare URL used

(MD034, no-bare-urls)


11-11: null
Bare URL used

(MD034, no-bare-urls)

Comment on lines +247 to +267
**Given** Longhorn v1.7.x cluster with 3 worker nodes.

**And** Set up the default backup target.

**And** Create a volume and attach it, and create a backup A of the volume A, and wait for the backup completed.

**And** Create a backing image BI, and create a backup BBI of the backing image BI, and wait for the backup completed.

**Then** Upgrade Longhorn to v1.8.0

**Verify** The `default` backup target will not be deleted.

**Verify** The settings `backup-target`, `backup-target-credential-secret`, and `backupstore-poll-interval` are removed from global settings.

**Verify** The fields `Spec.BackupTargetName` of the volume A is the default backup target name `default`.

**Verify** The field `Status.BackupTarget` and labels `backup-target` of the backup A is the default backup target name `default`.

**Verify** The fields `Spec.BackupTargetName` and labels `backup-target` of the backup volume A is the default backup target name `default`, and `Spec.VolumeName` of the backup volume A is the name of the volume A.

**Verify** The fields `Spec.BackupTargetName` and labels of the backing image BI is the default backup target name `default`, and `Spec.BackingImage` of the backup BBI is the name of the backing image BI.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add version compatibility verification steps

The upgrade test section should include additional verification steps:

  1. Verify API compatibility
  2. Verify backup functionality post-upgrade
  3. Document expected behavior for in-progress operations during upgrade

Would you like me to provide a detailed list of additional verification steps for the upgrade section?

🧰 Tools
🪛 LanguageTool

[style] ~253-~253: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., and wait for the backup completed. And Create a backing image BI, and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~261-~261: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lare removed from global settings. **Verify** The fieldsSpec.BackupTargetName` of...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~263-~263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efault backup target name default. Verify The field Status.BackupTarget and l...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

Comment on lines +187 to +190
**Then** Create a system backup.

**Verify The system backup is completed, and the system backup and the volume A is stored on the default backup target, and the volume B is stored on the backup target A.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix formatting and add specific verification steps for system backup

The current verification step has a syntax error (missing closing quote) and lacks specific validation steps.

-**Verify The system backup is completed, and the system backup and the volume A is stored on the default backup target, and the volume B is stored on the backup target A.
+**Verify** The system backup is completed with the following conditions:
+1. System backup is successfully created
+2. Volume A data is stored on the default backup target
+3. Volume B data is stored on backup target A
+4. All metadata is properly preserved
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
**Then** Create a system backup.
**Verify The system backup is completed, and the system backup and the volume A is stored on the default backup target, and the volume B is stored on the backup target A.
**Then** Create a system backup.
**Verify** The system backup is completed with the following conditions:
1. System backup is successfully created
2. Volume A data is stored on the default backup target
3. Volume B data is stored on backup target A
4. All metadata is properly preserved

@mantissahz mantissahz force-pushed the issue5411-manual-tests branch from ca84e9a to 2eeffaf Compare December 31, 2024 14:03
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md (3)

7-11: Format URLs using markdown link syntax

Convert bare URLs to proper markdown links for better readability.

-https://github.com/longhorn/longhorn/issues/5411
+[longhorn#5411](https://github.com/longhorn/longhorn/issues/5411)

-https://github.com/longhorn/longhorn/pull/6630
+[longhorn#6630](https://github.com/longhorn/longhorn/pull/6630)
🧰 Tools
🪛 Markdownlint (0.37.0)

7-7: null
Bare URL used

(MD034, no-bare-urls)


11-11: null
Bare URL used

(MD034, no-bare-urls)


21-21: Add verification steps for backup target URL update

Enhance the verification step to include specific checks:

-**Verify** The default backup target URL is updated as the value of the setting `defaultBackupStore.backupTarget`.
+**Verify**
+1. The default backup target URL is updated in the `ConfigMap` `longhorn-default-backupstore`
+2. The backup target becomes available in the Longhorn UI
+3. The backup target is accessible and can list existing backups (if any)

4-4: Add a test scenarios summary section

Consider adding a summary section after the introduction that lists all test scenarios with their objectives. This will help readers quickly understand the scope of testing.

+## Test Scenarios Overview
+
+1. Default Backup Target Configuration
+   - Verify creation and updates of default backup target settings
+2. Storage Class Integration
+   - Test volume creation with different backup target configurations
+3. Backup Operations
+   - Validate backup creation and restoration across multiple targets
+4. Backup Target URL Management
+   - Test modifications and synchronization of backup target URLs
+5. Backing Image Operations
+   - Verify backing image backup and restoration
+6. DR Volume Functionality
+   - Test DR volume creation and synchronization
+7. System Backup
+   - Validate system-wide backup and restoration
+8. Backup Target Management
+   - Test deletion and cleanup of backup targets
+9. Installation Management
+   - Verify uninstallation and upgrade procedures
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca84e9a and 2eeffaf.

📒 Files selected for processing (1)
  • docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md

[style] ~51-~51: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...orage Class C. Verify The volume C can not be created. ## Test Create And Restore...

(CAN_NOT_PREMIUM)


[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...And** Create a volume and attach it. And Write data to the volume. Then C...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[uncategorized] ~97-~97: Possible missing comma found.
Context: ...et B. Then Modify the extra backup target A URL to an invalid URL. Verify Th...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~103-~103: Possible missing comma found.
Context: ...ed up. Then Empty the extra backup target A URL Verify related backup volume...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~143-~143: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ote backup store S for two clusters. And Create a volume A with the default ba...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~145-~145: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...p target in cluster A and attach it. And Create a volume B with the backup tar...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~147-~147: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rget E in cluster B and attach it. And Write data to the volume A and B. **...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[uncategorized] ~163-~163: The preposition ‘as’ seems more likely in this position.
Context: ...y** data of the volume B-DR is the same to the volume A. Then Create a backup...

(AI_HYDRA_LEO_REPLACE_TO_AS)


[style] ~169-~169: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...01 is synchronized in the cluster A. Then Write data to the volume B and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~183-~183: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...default backup target and attach it. And Create a volume B with the backup tar...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~185-~185: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...the backup target A and attach it. And Write data to the volume A and B. **...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[uncategorized] ~201-~201: Possible missing comma found.
Context: ...fault backup target and an extra backup target A which has existing backups. **Verify...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~221-~221: Possible missing comma found.
Context: ...t be affected. Then Add the backup target A back. Verify Related backup volu...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~243-~243: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...up targets are deleted successfully. Verify Uninstall successfully. ## Test Upgr...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~253-~253: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., and wait for the backup completed. And Create a backing image BI, and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~261-~261: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lare removed from global settings. **Verify** The fieldsSpec.BackupTargetName` of...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~263-~263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efault backup target name default. Verify The field Status.BackupTarget and l...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🪛 Markdownlint (0.37.0)
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md

7-7: null
Bare URL used

(MD034, no-bare-urls)


11-11: null
Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (2)
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md (2)

189-189: Fix formatting and add specific verification steps for system backup

The current verification step needs better structure and specific validation steps.

-**Verify The system backup is completed, and the system backup and the volume A is stored on the default backup target, and the volume B is stored on the backup target A.
+**Verify** The system backup is completed with the following conditions:
+1. System backup is successfully created
+2. Volume A data is stored on the default backup target
+3. Volume B data is stored on backup target A
+4. All metadata is properly preserved

255-267: 🛠️ Refactor suggestion

Enhance upgrade verification steps

Add specific checks to ensure smooth version transition:

 **Then** Upgrade Longhorn to v1.8.0

-**Verify** The `default` backup target will not be deleted.
+**Verify**
+1. The upgrade process completes without errors
+2. The `default` backup target remains intact
+3. All volumes remain healthy and accessible
+4. In-progress operations (if any) complete successfully
+5. API endpoints remain compatible
+6. Existing backup operations continue to work
+7. The following settings and fields are properly migrated:
   - Settings `backup-target`, `backup-target-credential-secret`, and `backupstore-poll-interval` are removed
   - Volume fields: `Spec.BackupTargetName` is set to `default`
   - Backup fields: `Status.BackupTarget` and labels are set to `default`
   - Backup volume fields: `Spec.BackupTargetName`, labels, and `Spec.VolumeName` are correct
   - Backing image fields: `Spec.BackupTargetName`, labels, and backup fields are correct

Likely invalid or redundant comment.

🧰 Tools
🪛 LanguageTool

[style] ~261-~261: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lare removed from global settings. **Verify** The fieldsSpec.BackupTargetName` of...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~263-~263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efault backup target name default. Verify The field Status.BackupTarget and l...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

Copy link
Contributor

@roger-ryao roger-ryao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mantissahz mantissahz requested a review from a team January 2, 2025 06:16
ref: longhorn/longhorn 5411

Signed-off-by: James Lu <jamesluhz@gmail.com>
@mantissahz mantissahz force-pushed the issue5411-manual-tests branch from 2eeffaf to 7f5795f Compare January 6, 2025 02:15
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md (3)

7-11: Format URLs using markdown link syntax

Convert bare URLs to proper markdown links for better readability and navigation.

-https://github.com/longhorn/longhorn/issues/5411
+[longhorn#5411](https://github.com/longhorn/longhorn/issues/5411)

-https://github.com/longhorn/longhorn/pull/6630
+[longhorn#6630](https://github.com/longhorn/longhorn/pull/6630)
🧰 Tools
🪛 Markdownlint (0.37.0)

7-7: null
Bare URL used

(MD034, no-bare-urls)


11-11: null
Bare URL used

(MD034, no-bare-urls)


35-40: Clarify storage class setup and validation steps

The storage class creation steps need more detailed preconditions and validation points.

-**And** Create a Storage Class A without the parameter `backupTargetName`.
-**And** Create a Storage Class B with the parameter `backupTargetName: backupTargetB` (the backup target `backupTargetB` is created.)
-**And** Create a Storage Class C with the parameter `backupTargetName: backupTargetC` (the backup target `backupTargetC` is not created.)

+**And** Set up storage classes:
+1. Storage Class A:
+   - Create without `backupTargetName` parameter
+   - Verify it uses default storage settings
+
+2. Storage Class B:
+   - Create backup target `backupTargetB` with valid configuration
+   - Create storage class with `backupTargetName: backupTargetB`
+   - Verify backup target is properly linked
+
+3. Storage Class C:
+   - Create storage class with `backupTargetName: backupTargetC`
+   - Verify storage class creation fails with appropriate error message
+   - Confirm error indicates missing backup target

51-51: Add specific error validation steps

The current verification step lacks error message validation.

-**Verify** The volume C can not be created.
+**Verify** Volume C creation:
+1. Volume creation fails with appropriate error
+2. Error message indicates invalid/missing backup target
+3. No partial resources are left in the system
🧰 Tools
🪛 LanguageTool

[style] ~51-~51: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...orage Class C. Verify The volume C can not be created. ## Test Create And Restore...

(CAN_NOT_PREMIUM)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2eeffaf and 7f5795f.

📒 Files selected for processing (1)
  • docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md

[style] ~51-~51: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...orage Class C. Verify The volume C can not be created. ## Test Create And Restore...

(CAN_NOT_PREMIUM)


[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...And** Create a volume and attach it. And Write data to the volume. Then C...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~143-~143: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ote backup store S for two clusters. And Create a volume A with the default ba...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~145-~145: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...p target in cluster A and attach it. And Create a volume B with the backup tar...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~147-~147: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rget E in cluster B and attach it. And Write data to the volume A and B. **...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~169-~169: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...01 is synchronized in the cluster A. Then Write data to the volume B and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~183-~183: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...default backup target and attach it. And Create a volume B with the backup tar...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~185-~185: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...the backup target A and attach it. And Write data to the volume A and B. **...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~243-~243: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...up targets are deleted successfully. Verify Uninstall successfully. ## Test Upgr...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~253-~253: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., and wait for the backup completed. And Create a backing image BI, and create...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~261-~261: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lare removed from global settings. **Verify** The fieldsSpec.BackupTargetName` of...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~263-~263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efault backup target name default. Verify The field Status.BackupTarget and l...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🪛 Markdownlint (0.37.0)
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md

7-7: null
Bare URL used

(MD034, no-bare-urls)


11-11: null
Bare URL used

(MD034, no-bare-urls)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build images
  • GitHub Check: Summary
🔇 Additional comments (2)
docs/content/manual/release-specific/v1.8.0/test-multiple-backup-stores.md (2)

195-195: 🛠️ Refactor suggestion

Add specific verification steps for system backup restoration

The current verification step lacks detailed validation points.

-**Verify** Restoration is completed, and backup target A, volume A and B is restored.
+**Verify** System backup restoration is completed with the following conditions:
+1. Backup target A is restored with correct configuration
+2. Volume A is restored with:
+   - Correct data from default backup target
+   - Proper attachment status
+3. Volume B is restored with:
+   - Correct data from backup target A
+   - Proper attachment status
+4. All metadata and configurations are properly restored

Likely invalid or redundant comment.


255-267: 🛠️ Refactor suggestion

Add API compatibility and in-progress operation verification steps

The upgrade verification should include additional checks for API compatibility and handling of in-progress operations.

Add the following verification steps:

 **Then** Upgrade Longhorn to v1.8.0

+**Verify** API compatibility:
+1. All v1.7.x API endpoints continue to work
+2. New v1.8.0 backup target APIs are accessible
+3. No deprecation warnings in logs
+
+**Verify** In-progress operations:
+1. Ongoing backups continue without interruption
+2. Backup synchronization resumes properly
+3. No orphaned or incomplete backup resources
+
 **Verify** The `default` backup target will not be deleted.

Likely invalid or redundant comment.

🧰 Tools
🪛 LanguageTool

[style] ~261-~261: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lare removed from global settings. **Verify** The fieldsSpec.BackupTargetName` of...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~263-~263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efault backup target name default. Verify The field Status.BackupTarget and l...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

@yangchiu yangchiu merged commit c9ba207 into longhorn:master Jan 7, 2025
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants