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

test(negative): implement backupstore setup #1539

Merged
merged 4 commits into from
Apr 15, 2024

Conversation

yangchiu
Copy link
Member

test: implement backupstore setup

(1) add backupstore.py for robot test cases
(2) only support s3 now, the subprocess parts in backupstore.py need to be refined to make nfs work
(3) fix wrong longorn client url issue when using it out-of-cluster (the same issue as #1300 (comment))

For longhorn/longhorn#6710

Signed-off-by: Yang Chiu yang.chiu@suse.com

Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

Some feedback, but I didn't review all of them. Some need to be resolved first.

backupstore.py seems verbose and unstructured. I would suggest doing the below.

Use different Python modules to aggregate the related codes.

  • backupstore/__init__.py including all common functions for backupstore
    backupstore/nfs.py
    backupstore/minio.py (but actually it's s3)

Also, using the common naming (verb) for the function and method name instead, backupstore_cleanup should be changed to cleanup_backupstore and added to backupstore/__init__.py.

Please review the code and do the corresponding update.

e2e/libs/backupstore.py Outdated Show resolved Hide resolved
e2e/libs/backupstore.py Outdated Show resolved Hide resolved
e2e/libs/backupstore.py Outdated Show resolved Hide resolved
e2e/libs/backupstore.py Outdated Show resolved Hide resolved
e2e/libs/backupstore.py Outdated Show resolved Hide resolved
e2e/libs/backupstore.py Outdated Show resolved Hide resolved
@yangchiu
Copy link
Member Author

yangchiu commented Oct 4, 2023

Some feedback, but I didn't review all of them. Some need to be resolved first.

backupstore.py seems verbose and unstructured. I would suggest doing the below.

Use different Python modules to aggregate the related codes.

* backupstore/__init__.py including all common functions for backupstore
  backupstore/nfs.py
  backupstore/minio.py (but actually it's s3)

Also, using the common naming (verb) for the function and method name instead, backupstore_cleanup should be changed to cleanup_backupstore and added to backupstore/init.py.

Please review the code and do the corresponding update.

Refined to have class structure.

@yangchiu yangchiu requested a review from innobead October 4, 2023 09:15
@yangchiu
Copy link
Member Author

@longhorn/qa @innobead This PR is required to run negative test case Reboot Volume Node While Heavy Writing And Recurring Jobs Exist in v1.4.4 release test plan.

e2e/keywords/common.resource Show resolved Hide resolved
e2e/libs/backupstore/base.py Outdated Show resolved Hide resolved
e2e/libs/backupstore/minio.py Show resolved Hide resolved
e2e/libs/backupstore/base.py Outdated Show resolved Hide resolved
e2e/libs/backupstore/minio.py Outdated Show resolved Hide resolved
@yangchiu yangchiu force-pushed the backupstore branch 2 times, most recently from dbfbd7b to bddaaee Compare October 27, 2023 01:13
c3y1huang
c3y1huang previously approved these changes Oct 27, 2023
Copy link
Collaborator

@c3y1huang c3y1huang left a comment

Choose a reason for hiding this comment

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

I assume most of the codes are moved from the integration tests as I see there are some variables that are still undefined, and the functions could be better organized. However, as long as the tests can run I think it's fine to merge this first and enhance later.

cc @innobead

@mergify
Copy link

mergify bot commented Oct 27, 2023

This pull request is now in conflicts. Could you fix it @yangchiu? 🙏

@yangchiu yangchiu force-pushed the backupstore branch 2 times, most recently from 36a5e11 to 26966bd Compare April 2, 2024 01:52
@yangchiu yangchiu requested review from a team and c3y1huang April 2, 2024 01:58
e2e/libs/keywords/backupstore_keywords.py Outdated Show resolved Hide resolved
e2e/libs/backupstore/base.py Outdated Show resolved Hide resolved
e2e/libs/backupstore/minio.py Outdated Show resolved Hide resolved
e2e/libs/recurringjob/rest.py Outdated Show resolved Hide resolved
e2e/libs/utility/utility.py Outdated Show resolved Hide resolved
(1) add backupstore.py for robot test cases
(2) only support s3 now, the subprocess parts in
    backupstore.py need to be refined to make
    nfs work

Signed-off-by: Yang Chiu <yang.chiu@suse.com>
@yangchiu yangchiu requested review from c3y1huang and a team April 10, 2024 07:52
e2e/libs/backupstore/minio.py Outdated Show resolved Hide resolved
auto-merge was automatically disabled April 15, 2024 01:24

Merge commits are not allowed on this repository

@yangchiu yangchiu merged commit 1d42c1c into longhorn:master Apr 15, 2024
5 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.

4 participants