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

e2e: Improve storage class names and supported storage checks #1635

Closed
Tracked by #1717
nirs opened this issue Oct 31, 2024 · 1 comment · Fixed by #1714
Closed
Tracked by #1717

e2e: Improve storage class names and supported storage checks #1635

nirs opened this issue Oct 31, 2024 · 1 comment · Fixed by #1714
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed test Testing related issue

Comments

@nirs
Copy link
Member

nirs commented Oct 31, 2024

The e2e framework uses a list of storage class names and access modes:

pvcspecs:
  - storageclassname: rook-cephfs
    accessmodes: ReadWriteMany
  - storageclassname: rook-ceph-block
    accessmodes: ReadWriteOnce

Based on this list, we generate test names:

func generateSuffix(storageClassName string) string {
	suffix := storageClassName

	if strings.ToLower(storageClassName) == "rook-ceph-block" {
		suffix = "rbd"
	}

	if strings.ToLower(storageClassName) == "rook-cephfs" {
		suffix = "cephfs"
	}

	return suffix
}

This works only for drenv environment. For openshift environment the storage class names are different so the returned suffix with be the actual storage class name, which is way to long to use as a test name or for using resource names.

Even worse, there is code hidden deployers/discoveredapps.go, filtering out cephfs workloads for discovered apps, based on the suffix:

func (d DiscoveredApps) IsWorkloadSupported(w workloads.Workload) bool {
	return w.GetName() != "Deploy-cephfs"
}

This will not match for when using real cluster storage classes, trying to run unsupported combination.

Possible fix

Move the logic to the config:

storage:
  - name: file
    storageclassname: rook-cephfs
    accessmodes: ReadWriteMany
    supports-discovered-apps: false
  - name: block
    storageclassname: rook-ceph-block
    accessmodes: ReadWriteOnce
    supports-discovered-apps: true
  • With this layout we can define any storage class and enable the storage class for discovered apps without changing the code.
  • We don't need the code mapping the actual storage class names to test names, instead we can use the name property.

Blocks #1625

@nirs nirs added the test Testing related issue label Oct 31, 2024
@nirs
Copy link
Member Author

nirs commented Dec 8, 2024

@raghavendra-talur what do you think?

@nirs nirs added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Dec 8, 2024
nirs added a commit to nirs/ramen that referenced this issue Dec 11, 2024
We have configuration allowing any storage class name, but the code was
hard-coding the names "rook-ceph-block" and "rook-cephfs", limiting the
e2e tests to drenv environment.

Add a Name key to each PVCSpec entry, eliminating the code to create a
suffix. We use the name to crate the name of the test.

Part-of: RamenDR#1635
Signed-off-by: Nir Soffer <[email protected]>
nirs added a commit to nirs/ramen that referenced this issue Dec 11, 2024
We checked if a deployer supports a workload by checking the hardcoded
string "Deploy-cephfs". This wrong it 2 ways, assuming that we use the
"Deploy" prefix, and assuming that the storage name is "cephfs".

Fix by adding "unsupportedDeployers" lists to PVCSpec. The cephfs
default configuration include the "disapp" as unsupported deployer.

Move the supported check from the deployer to the workload, since the
deployer has no state and it cannot tell if a workload is supported.

To make it easier to filter deployers names are now lower case. We
anyway use lower case for test names.

With this change, we can use any PVCSpec name, and we can keep multiple
configurations running all of some the tests.

Fixes: RamenDR#1635
Signed-off-by: Nir Soffer <[email protected]>
This was referenced Dec 11, 2024
nirs added a commit to nirs/ramen that referenced this issue Dec 11, 2024
We have configuration allowing any storage class name, but the code was
hard-coding the names "rook-ceph-block" and "rook-cephfs", limiting the
e2e tests to drenv environment.

Add a Name key to each PVCSpec entry, eliminating the code to create a
suffix. We use the name to crate the name of the test.

Part-of: RamenDR#1635
Signed-off-by: Nir Soffer <[email protected]>
nirs added a commit to nirs/ramen that referenced this issue Dec 11, 2024
We checked if a deployer supports a workload by checking the hardcoded
string "Deploy-cephfs". This wrong it 2 ways, assuming that we use the
"Deploy" prefix, and assuming that the storage name is "cephfs".

Fix by adding "unsupportedDeployers" lists to PVCSpec. The cephfs
default configuration include the "disapp" as unsupported deployer.

Move the supported check from the deployer to the workload, since the
deployer has no state and it cannot tell if a workload is supported.

To make it easier to filter deployers names are now lower case. We
anyway use lower case for test names.

With this change, we can use any PVCSpec name, and we can keep multiple
configurations running all of some the tests.

Fixes: RamenDR#1635
Signed-off-by: Nir Soffer <[email protected]>
nirs added a commit to nirs/ramen that referenced this issue Dec 11, 2024
We checked if a deployer supports a workload by checking the hardcoded
string "Deploy-cephfs". This wrong it 2 ways, assuming that we use the
"Deploy" prefix, and assuming that the storage name is "cephfs".

Fix by adding "unsupportedDeployers" lists to PVCSpec. The cephfs
default configuration include the "disapp" as unsupported deployer.

Move the supported check from the deployer to the workload, since the
deployer has no state and it cannot tell if a workload is supported.

To make it easier to filter deployers, names are now lower case. We
anyway use lower case for test names.

With this change, we can use any PVCSpec name, and we can keep multiple
configurations running all of some the tests.

Fixes: RamenDR#1635
Signed-off-by: Nir Soffer <[email protected]>
nirs added a commit to nirs/ramen that referenced this issue Dec 11, 2024
We have configuration allowing any storage class name, but the code was
hard-coding the names "rook-ceph-block" and "rook-cephfs", limiting the
e2e tests to drenv environment.

Add a Name key to each PVCSpec entry, eliminating the code to create a
suffix. We use the name to crate the name of the test.

Part-of: RamenDR#1635
Signed-off-by: Nir Soffer <[email protected]>
nirs added a commit to nirs/ramen that referenced this issue Dec 11, 2024
We checked if a deployer supports a workload by checking the hardcoded
string "Deploy-cephfs". This wrong it 2 ways, assuming that we use the
"Deploy" prefix, and assuming that the storage name is "cephfs".

Fix by adding "unsupportedDeployers" lists to PVCSpec. The cephfs
default configuration include the "disapp" as unsupported deployer.

Move the supported check from the deployer to the workload, since the
deployer has no state and it cannot tell if a workload is supported.

To make it easier to filter deployers, names are now lower case. We
anyway use lower case for test names.

With this change, we can use any PVCSpec name, and we can keep multiple
configurations running all of some the tests.

Fixes: RamenDR#1635
Signed-off-by: Nir Soffer <[email protected]>
raghavendra-talur pushed a commit that referenced this issue Dec 12, 2024
We have configuration allowing any storage class name, but the code was
hard-coding the names "rook-ceph-block" and "rook-cephfs", limiting the
e2e tests to drenv environment.

Add a Name key to each PVCSpec entry, eliminating the code to create a
suffix. We use the name to crate the name of the test.

Part-of: #1635
Signed-off-by: Nir Soffer <[email protected]>
raghavendra-talur pushed a commit that referenced this issue Dec 12, 2024
We checked if a deployer supports a workload by checking the hardcoded
string "Deploy-cephfs". This wrong it 2 ways, assuming that we use the
"Deploy" prefix, and assuming that the storage name is "cephfs".

Fix by adding "unsupportedDeployers" lists to PVCSpec. The cephfs
default configuration include the "disapp" as unsupported deployer.

Move the supported check from the deployer to the workload, since the
deployer has no state and it cannot tell if a workload is supported.

To make it easier to filter deployers, names are now lower case. We
anyway use lower case for test names.

With this change, we can use any PVCSpec name, and we can keep multiple
configurations running all of some the tests.

Fixes: #1635
Signed-off-by: Nir Soffer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed test Testing related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant