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

CC Automation - reclaim-space CLI test #14425

Merged
merged 6 commits into from
Jun 5, 2024

Conversation

sambible
Copy link
Contributor

Test for following BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2164997

Also adds the associated CLI endpoint to enable the test.

trigger: test-robottelo
pytest: tests/foreman/cli/test_repositories.py -k 'test_positive_reclaim_space'

@sambible sambible added CherryPick PR needs CherryPick to previous branches 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 labels Mar 15, 2024
@sambible sambible self-assigned this Mar 15, 2024
@sambible sambible requested review from a team as code owners March 15, 2024 18:37
@sambible
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_repositories.py -k 'test_positive_reclaim_space'

Copy link
Contributor

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Let's rerun PRT. Should this also be tested with a custom repo that does an immediate sync? Not sure exactly what reclaim space does but in this scenario I assume all it would be removing is metadata and not actual packages?

@Griffin-Sullivan
Copy link
Contributor

trigger: test-robottelo
pytest: tests/foreman/cli/test_repositories.py -k 'test_positive_reclaim_space'

@vsedmik
Copy link
Contributor

vsedmik commented Mar 18, 2024

Code looks good to me. Let's rerun PRT. Should this also be tested with a custom repo that does an immediate sync? Not sure exactly what reclaim space does but in this scenario I assume all it would be removing is metadata and not actual packages?

@Griffin-Sullivan Reclaim_space actually applies to the on_demand repos (yes, it could be even custom but on_demand). It purges the downloaded rpms to release the storage but keeps the metadata. For reference see: https://docs.pulpproject.org/pulpcore/workflows/reclaim-disk-space.html

@Griffin-Sullivan
Copy link
Contributor

Looks like the actual error is happening when disabling SCA for the entitlement manifest:

Received HTTP 500 response: {"displayMessage":"Content access mode is not present in the content access mode list for this org: entitlement","errors":["Content access mode is not present in the content access mode list for this org: entitlement"]}

Does this need to be an entitlement manifest? If not try with SCA manifest

Copy link
Member

@jyejare jyejare left a comment

Choose a reason for hiding this comment

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

ACK pending question and not-blocking comment !

2. hammer repository reclaim-space --id REPOID --organization-id ORGID


:expectedresults: Command works as expected
Copy link
Member

Choose a reason for hiding this comment

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

Should have a better expected result! Something like the space is reclaimed or more descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should state here that the space is really reclaimed and at the end, it should be asserted. The fact this passes without any error doesn't mean it works.

)
# Checking that the fail message isn't present. On a success, no message is returned
if output != {}:
assert 'Could not reclaim the repository' not in output[0]['message']
Copy link
Member

Choose a reason for hiding this comment

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

Couldnt we have some real scenario of reclaiming space rather than this generic check ?

I mean where we could verify some bytes/mbs are reclaimed ?

Copy link
Member

@rplevka rplevka left a comment

Choose a reason for hiding this comment

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

my comments are not really blocking

releasever=REPOS['kickstart']['rhel8_aps']['version'],
)
repo = module_target_sat.api.Repository(id=rh_repo_id).read()
repo.sync(timeout=600)
Copy link
Member

Choose a reason for hiding this comment

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

I think that all of the steps up until this point should be considered a setup. Consider using the already existing fixtures that prepare the repos for you, e.g. custom_synced_repo and module_repository

@@ -124,3 +124,41 @@ def test_positive_disable_rh_repo_with_basearch(module_target_sat, module_entitl
}
)
assert 'Repository disabled' in disabled_repo[0]['message']


def test_positive_reclaim_space(module_target_sat, module_entitlement_manifest_org):
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably change the name so it is not too misleading - I believe you do not really test that any space has been reclaimed, since you only run it on an on-demand-synced repo, right?

Copy link
Contributor

@lhellebr lhellebr left a comment

Choose a reason for hiding this comment

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

I think this should verify that some space is actually reclaimed. Absence of error message doesn't mean it works.

@lhellebr
Copy link
Contributor

Related: #14397

@sambible
Copy link
Contributor Author

sambible commented Apr 8, 2024

There's a lot of conversation around here that's valuable, but I'd like to ask if it's reasonable to get keep this test more focused on the actual CLI error that the BZ was originally written against, which is what it tests currently. I've changed the name to be more explicit, so hopefully that makes the purpose of this clear.

@devendra104
Copy link
Member

trigger: test-robottelo
pytest: tests/foreman/cli/test_repositories.py -k 'test_reclaim_space_command_no_exception'

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6371
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_repositories.py -k test_reclaim_space_command_no_exception --external-logging
Test Result : ============ 5 deselected, 3 warnings, 1 error in 722.51s (0:12:02) ============

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Apr 8, 2024
@sambible
Copy link
Contributor Author

sambible commented Apr 8, 2024

trigger: test-robottelo
pytest: tests/foreman/cli/test_repositories.py -k 'test_reclaim_space_command_no_exception'

@Satellite-QE Satellite-QE removed the PRT-Failed Indicates that latest PRT run is failed for the PR label Apr 8, 2024
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6373
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_repositories.py -k test_reclaim_space_command_no_exception --external-logging
Test Result : =========== 1 passed, 5 deselected, 4 warnings in 849.48s (0:14:09) ============

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 8, 2024
Copy link
Contributor

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

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

ACK, @sambible can you rebase?

@sambible sambible requested a review from a team as a code owner June 5, 2024 15:32
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 CherryPick PR needs CherryPick to previous branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants