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

ContentCredentials CLI fix for stream #15165

Merged
merged 1 commit into from
May 30, 2024
Merged

Conversation

damoore044
Copy link
Contributor

@damoore044 damoore044 commented May 23, 2024

Problem Statement

In Stream runs, for ContentCredentials, we see some occasional failures in several tests,
that attempt to add/update gpg key associated to repo/product with repo(s), then read info of the product/repo and assert the key name:

   ... setup custom repo, GPG key, add key to repo or repo's product,
        or update a GPG key already added to repo(s)

   repo = target_sat.cli.Repository.info({'id': repo['id']}) 
   assert repo['gpg-key'].get('name') == new_name
E   KeyError: 'gpg-key'

Solution

We need to wait for an in-progress Repository Metadata Generate task,
that is not handled synchronously by cli .update or cli .info. Else, the GPG key is not yet present in repo/product.

In any CLI tests adding/updating a GPG key associated to repo, or adding the key to a product containing repos,
we should wait for these repo metadata task(s), before calling info of product/repo, and asserting the key.

Related Issues

  • Intermittent Stream CLI failures in ContentCredentials, el8 and el9.
  • 1-3 of the affected tests inconsistently fail for same KeyError. Locally reproduced when full cli module is run.
  • The metadata task is one per repo with the key added, and typically lasts 3 seconds or less,
    but it is enough for an intermittent failure when trying to read the entity (repos or product) and find the key, before the task(s) finished.

PRT Case

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py

@damoore044 damoore044 added No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels May 23, 2024
@damoore044 damoore044 self-assigned this May 23, 2024
@damoore044 damoore044 requested a review from a team as a code owner May 23, 2024 19:59
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7100
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : =========== 1 failed, 83 passed, 216 warnings in 1346.78s (0:22:26) ============

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label May 23, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7101
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : ============ 7 passed, 116 warnings, 77 errors in 834.60s (0:13:54) ============

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7107
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : ============ 7 passed, 115 warnings, 77 errors in 733.79s (0:12:13) ============

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7110
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : ============ 7 passed, 115 warnings, 77 errors in 760.52s (0:12:40) ============

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py
nailgun: 1151

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7113
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : ================ 84 passed, 222 warnings in 1270.67s (0:21:10) =================

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels May 24, 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.

Nice catch, ACK!
Non-blocking - the comments are not necessary imho - the function name and docstring are self-describing enough.

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py
nailgun: 1151

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label May 28, 2024
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label May 28, 2024
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7144
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : =========== 1 failed, 83 passed, 222 warnings in 1196.17s (0:19:56) ============

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py
nailgun: 1152

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7145
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : =========== 1 failed, 83 passed, 224 warnings in 1172.98s (0:19:32) ============

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py
nailgun: 1152

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7147
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : =========== 1 failed, 83 passed, 230 warnings in 1196.63s (0:19:56) ============

Copy link
Contributor

@sambible sambible left a comment

Choose a reason for hiding this comment

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

Change looks good, only 1 failure being returned seems positive. Still, I'd like to see a consistent pass here before we merge :)

@shweta83
Copy link
Contributor

@damoore044 Please make sure the test failing is unrelated to this change. The change looks good otherwise.

@damoore044
Copy link
Contributor Author

The remaining failure seems to be the same kind of flakiness in test_positive_update_key_for_product_with_repos
will try to address this soon

Copy link
Contributor

@ColeHiggins2 ColeHiggins2 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 prt

Copy link
Contributor

@ColeHiggins2 ColeHiggins2 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 prt

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7170
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : ================ 84 passed, 237 warnings in 1237.59s (0:20:37) =================

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels May 29, 2024
Comment on lines +48 to +50
def wait_for_repo_metadata_tasks(sat, org_name, repo_name='', product_name=''):
"""Search and wait for any repository metadata generate task(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain the rationale for adding a new function for this instead using existing wait_for_tasks helper func directly in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They all need pretty much the same thing so saw the opportunity to streamline,
and perhaps any future use in other CLI tests?

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_contentcredentials.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7185
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_contentcredentials.py --external-logging
Test Result : ================ 84 passed, 229 warnings in 1235.96s (0:20:35) =================

@vsedmik vsedmik added AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing 6.16.z CherryPick PR needs CherryPick to previous branches and removed No-CherryPick PR doesnt need CherryPick to previous branches labels May 30, 2024
@vsedmik vsedmik merged commit d88e9c0 into SatelliteQE:master May 30, 2024
12 of 15 checks passed
github-actions bot pushed a commit that referenced this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR Stream Introduced in or relating directly to Satellite Stream/Master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants