-
Notifications
You must be signed in to change notification settings - Fork 39
adjust dv size to fit GCP minimumSupportedPvcSize #1246
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
base: cnv-4.19
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
14bc1f5
to
2fe5613
Compare
2fe5613
to
1a3af9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D/S test tox -e verify-bugs-are-open
failed: cnv-tests-tox-executor/13262
/wip cancel |
dc8c8ea
to
dc2e27a
Compare
@@ -237,7 +239,7 @@ def test_virtctl_image_upload_pvc(download_image, namespace, storage_class_name_ | |||
namespace=namespace.name, | |||
pvc=True, | |||
name=pvc_name, | |||
size="1Gi", | |||
size=storage_profile_minimum_supported_pvc_size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if we do this now, and the next provisioner will have a 2Gi minimum size limitation - how will we catch it?
storage_class=storage_class, | ||
access_modes=access_modes, | ||
volume_mode=volume_mode, | ||
client=client, | ||
size=get_default_storage_profile_minimum_supported_pvc_size(), | ||
) as dv: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function gets a storage_class
, will get_default_storage_profile_minimum_supported_pvc_size
return the value for the same storage_class
?
@pytest.fixture(scope="session") | ||
def xfail_test_if_gcp_sc(cluster_storage_classes): | ||
existing_gcp_sc = [sc.name for sc in cluster_storage_classes if sc.name in StorageClassNames.GCP] | ||
if existing_gcp_sc: | ||
pytest.xfail( | ||
reason=f"This test is X failed due to non supporting GCP storage class dv less than 4Gi'{existing_gcp_sc}'" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my cluster has GCP and OCS, and I'm running the test on OCS, this would xfail anyway?
@@ -542,6 +542,7 @@ def test_blank_disk_import_validate_status(data_volume_multi_storage_scope_funct | |||
], | |||
) | |||
def test_vmi_image_size( | |||
xfail_test_if_gcp_sc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the 13Gi param - this test can still run
What should happen if we requested 64M, but got 4Gi PVC? Should VMI see the 4Gi?
@@ -585,7 +587,7 @@ def create_cirros_vm( | |||
source="http", | |||
url=get_http_image_url(image_directory=Images.Cirros.DIR, image_name=Images.Cirros.QCOW2_IMG), | |||
storage_class=storage_class, | |||
size=Images.Cirros.DEFAULT_DV_SIZE, | |||
size=get_default_storage_profile_minimum_supported_pvc_size(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For http import, storage profile annotation should handle this, no?
|
||
def get_default_storage_profile_minimum_supported_pvc_size() -> str: | ||
return StorageProfile( | ||
name=next(iter(py_config["system_storage_class_matrix"][0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of deducing the storage profile name using 'next(item(py_config["system_storage_class_matrix"][0]))' , is that good to pass the storage class name directly to this function and make use of it here ? (i.e) get_default_storage_profile_minimum_supported_pvc_size(storage_class=storage_class)
Short description:
this PR is following the new cloud providors storage class, gcp, aro, arm etcc... that is supporting minimumSupportedPvcSize > 4Gi
there are few tests that are failing because they use dv's that are smaller than minimumSupportedPvcSize, 1Gi etc ...
More details:
there are many test that are failing due to small dv size, need to adjust these test accordingly to minimumSupportedPvcSize, size changed dynamiclly + added a function that fetch the minimumSupportedPvcSize from the storage profile
https://docs.google.com/presentation/d/1wDDKB0ZGbRpObNP9hqSu2Rm8OdccRTQ1St-eNEQaFYM/edit?slide=id.g317d1609066_0_17#slide=id.g317d1609066_0_17
https://drive.google.com/file/d/1MT99MArNHn4e85APbvrawDZydQzHxslk/view
What this PR does / why we need it:
there are many test that are failing due to small dv size, need to adjust these test accordingly to minimumSupportedPvcSiz from storgae profile
Which issue(s) this PR fixes:
T2 failuers
https://jenkins-csb-cnvqe-main.dno.corp.redhat.com/job/test-pytest-cnv-4.19-storage-gcp/5/testReport/
Special notes for reviewer:
jira-ticket:
https://issues.redhat.com/browse/CNV-63567