test(robot): uninstall when backing image exists #2240
Conversation
WalkthroughThe pull request introduces modifications to the backing image management system across multiple files. The primary changes involve updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
c3y1huang
force-pushed
the
10044-backing-image-uninstall-failed
branch
2 times, most recently
from
0352f17 to
879e698
Compare
Signed-off-by: Chin-Ya Huang <[email protected]>
…empty Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-10044 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
10044-backing-image-uninstall-failed
branch
2 times, most recently
from
4d63b50 to
06bae72
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/libs/backing_image/backing_image.py (1)
38-46: Refactor repeated CRD instantiationMultiple methods are duplicating the CRD instantiation code. Consider refactoring this into a helper method or handling it in the constructor based on operation type.
Example refactor:
def _ensure_crd_backend(self): if not isinstance(self.backing_image, CRD): self.backing_image = CRD()Then use it in methods:
def delete_backing_image_manager(self, name): self._ensure_crd_backend() return self.backing_image.delete_backing_image_manager(name)e2e/tests/regression/test_backing_image.robot (2)
42-47: Document LOOP_COUNT and add verification steps
- The
LOOP_COUNTvariable should be documented- Consider adding verification steps to ensure backing images are properly created before uninstalling:
- Verify backing image status
- Check min_copies requirement is met
Add verification steps:
And Verify all disk file status of backing image v1-bi-qcow2 are ready And Verify all disk file status of backing image v1-bi-raw are ready And Verify all disk file status of backing image v2-bi-qcow2-1 are ready And Verify all disk file status of backing image v2-bi-raw-1 are ready
49-52: Add error handling verificationConsider adding steps to verify error handling during uninstallation:
- Check for appropriate error messages
- Verify cleanup of resources
When Uninstall Longhorn Then Verify no error messages in logs And Check all resources are properly cleaned up And Check all Longhorn CRD removed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/libs/backing_image/backing_image.py(3 hunks)e2e/libs/backing_image/base.py(1 hunks)e2e/libs/backing_image/crd.py(1 hunks)e2e/libs/backing_image/rest.py(4 hunks)e2e/libs/backupstore/base.py(1 hunks)e2e/libs/keywords/backing_image_keywords.py(1 hunks)e2e/tests/regression/test_backing_image.robot(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
e2e/libs/backing_image/crd.py (1)
17-17: Method signature needs implementationThe
createmethod signature has been updated to includedata_engineandmin_copiesparameters, but the method still returnsNotImplemented. This method needs to be implemented to support the new test case for uninstalling Longhorn when backing images exist.Would you like me to help implement this method based on the CRD schema?
e2e/libs/backing_image/rest.py (2)
Line range hint
15-25: LGTM! Proper implementation of the create methodThe updated create method correctly uses the new parameters
data_engineandmin_copiesin the backing image creation API call.
Line range hint
115-124: Implement backing image manager methodsThese newly added methods are required for managing backing image managers but are currently unimplemented. They are essential for the test case that verifies uninstallation behavior with existing backing images.
Would you like me to help implement these methods? The implementation should mirror the functionality in the CRD class, using REST API calls instead of Kubernetes API.
e2e/libs/backupstore/base.py (1)
43-46: LGTM! Proper null check for backupstore configurationGood defensive programming practice to handle the case when
LONGHORN_BACKUPSTOREenvironment variable is not set. This prevents potential errors when backupstore configuration is not required for certain test scenarios.
longhorn/longhorn-10044 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
10044-backing-image-uninstall-failed
branch
from
06bae72 to
f8d2e4d
Compare
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10044
What this PR does / why we need it:
Adding a test for uninstalling Longhorn when there are backing images. This is for reproducing the issue described in longhorn/longhorn#10044. While the issue cannot be consistently reproduced, automating the test steps can help simplify the process.
Special notes for your reviewer:
NoneAdditional documentation or context
NoneSummary by CodeRabbit
New Features
Bug Fixes
Chores