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

smbtorture: Prefer method to find specific flapping list #92

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anoopcs9
Copy link
Collaborator

@anoopcs9 anoopcs9 commented Aug 28, 2024

samba-in-kubernetes/sit-environment#121 adds method to share section in test-info.yml. With variant no longer relevant, prefer method to determine the need for VFS specific flapping file.

@anoopcs9 anoopcs9 requested a review from spuiuk August 28, 2024 10:37
@anoopcs9
Copy link
Collaborator Author

/retest centos-ci/cephfs

@spuiuk
Copy link
Collaborator

spuiuk commented Aug 28, 2024

I would rather keep the sharename -> variant logic out of sit-test-cases. Can you instead make this change in the sit-environment repo when building the test-info.yml file? This will keep the repo logic in naming the shares within the sit-environment repo itself and avoid complexity.

All testing access is over SMB only and the backend-variant combination is simply an arbitrary sting used to select the right list of tests in the smbtorture test suite.

@anoopcs9
Copy link
Collaborator Author

I would rather keep the sharename -> variant logic out of sit-test-cases. Can you instead make this change in the sit-environment repo when building the test-info.yml file? This will keep the repo logic in naming the shares within the sit-environment repo itself and avoid complexity.

This would mean we contradict ourselves with the meaning of variant in sit-environment itself. Right now variant indicates how we perform samba/ctdb setup. If we then introduce the logic to define variant based on methods in test-info.yml, that contradicts the earlier assumption.

All testing access is over SMB only and the backend-variant combination is simply an arbitrary sting used to select the right list of tests in the smbtorture test suite.

Sorry, I didn't get that correctly. IIUC, backend/variant doesn't affect the decision on what smbtorture tests are run against shares. Don't we run everything from smbtorture-tests-info.yml blindly against every listed shares?

@spuiuk
Copy link
Collaborator

spuiuk commented Aug 29, 2024

I would rather keep the sharename -> variant logic out of sit-test-cases. Can you instead make this change in the sit-environment repo when building the test-info.yml file? This will keep the repo logic in naming the shares within the sit-environment repo itself and avoid complexity.

This would mean we contradict ourselves with the meaning of variant in sit-environment itself. Right now variant indicates how we perform samba/ctdb setup. If we then introduce the logic to define variant based on methods in test-info.yml, that contradicts the earlier assumption.

We could rename the fields in the test-info.yml if that helps. There aren't many external users at the moment and we can make the changes without risk of regression.

All testing access is over SMB only and the backend-variant combination is simply an arbitrary sting used to select the right list of tests in the smbtorture test suite.

Sorry, I didn't get that correctly. IIUC, backend/variant doesn't affect the decision on what smbtorture tests are run against shares. Don't we run everything from smbtorture-tests-info.yml blindly against every listed shares?

You are right, we run all tests but we list the tests where we choose to ignore the tests in the flapping lists.

@anoopcs9 anoopcs9 force-pushed the flapping-from-share-name branch 3 times, most recently from beefbd1 to 695fc11 Compare August 30, 2024 10:47
@anoopcs9 anoopcs9 changed the title smbtorture: Find flapping list based on share name smbtorture: Prefer method to find specific flapping list Aug 30, 2024
@anoopcs9 anoopcs9 changed the title smbtorture: Prefer method to find specific flapping list smbtorture: Prefer method to find specific flapping list Aug 30, 2024
samba-in-kubernetes/sit-environment#121 adds
'method' to share section in test-info.yml. With 'variant' no longer
relevant, prefer 'method' to determine the need for VFS specific
flapping file.

Signed-off-by: Anoop C S <[email protected]>
@anoopcs9
Copy link
Collaborator Author

I would rather keep the sharename -> variant logic out of sit-test-cases. Can you instead make this change in the sit-environment repo when building the test-info.yml file? This will keep the repo logic in naming the shares within the sit-environment repo itself and avoid complexity.

This would mean we contradict ourselves with the meaning of variant in sit-environment itself. Right now variant indicates how we perform samba/ctdb setup. If we then introduce the logic to define variant based on methods in test-info.yml, that contradicts the earlier assumption.

We could rename the fields in the test-info.yml if that helps. There aren't many external users at the moment and we can make the changes without risk of regression.

Please see samba-in-kubernetes/sit-environment#121.

@spuiuk
Copy link
Collaborator

spuiuk commented Sep 3, 2024

There is no reason to use the logic from sit-environment to sit-test-cases. I would prefer to keep the configuration for sit-test-cases simple.

A possible way of keeping this simple is setting up the fields in test-info.yml in the following manner-
backend: cephfs
variant: default-vfs or default-vfs-new or default-kclient or mgr-vfs or mgr-kclient.

or
backend: cephfs-default / cephfs-mgr
variant: vfs / vfs-new / kclient

Both these cases will need us to rename the flapping lists for the smbtorture tests.

To simplify it further, we can remove the variant too. But the reason we had that was because we wanted a default set of flapping list and only a few 'variant' needed exceptions to be made to the flapping list. Removing the 'variant' field will require us to create a list for each backend-variant-method so may not be feasible.

I prefer the second approach from the two options given above.

To avoid confusion of terminology of sit-environment and sit-test-cases being different, we can rename the field variant to backend_option.

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