Skip to content

Conversation

@JAVGan
Copy link
Contributor

@JAVGan JAVGan commented Jan 21, 2026

This commit changes the get_image_by_tags call on publish method for
AWSService so it will search for the snapshot tags alonside the
requested tags.

We need that to ensure the snapshot was properly created in order to
avoid retrieving the wrong AMI during the search.

Refers to SPSTRAT-615

@JAVGan JAVGan force-pushed the fix_import_snapshot_issue branch from 8413c6f to 59fa787 Compare January 21, 2026 18:40
@JAVGan
Copy link
Contributor Author

JAVGan commented Jan 21, 2026

@lslebodn @rbikar PTAL

@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.09%. Comparing base (57e906f) to head (f19a3bd).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
cloudimg/aws.py 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   97.44%   96.09%   -1.35%     
==========================================
  Files           3        3              
  Lines         586      615      +29     
==========================================
+ Hits          571      591      +20     
- Misses         15       24       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@rbikar rbikar left a comment

Choose a reason for hiding this comment

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

LGTM, @JAVGan only a little question, the "snapshot_tags" is kind of hardcoded here. Is it something required by aws API or rather is it required minimum for tags set on image/snapshot?

cloudimg/aws.py Outdated
self.get_image_by_tags(
metadata.tags,
required_tags=["snapshot_tags"],
)
Copy link

@lslebodn lslebodn Jan 22, 2026

Choose a reason for hiding this comment

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

There are couple of tag parameters passed to he publish method via metada from the pubtools-marketplacesvm.

  1. tags

They are automatically extended by "custom_tags" provided by user via starmap-mappings.

  1. ami_tags && snapshot_tags passed here

So far they are the same and just contain billing code

Different objects should be find by following tags:

  • s3 object should be bound by tags
  • snapshot should by find by union of tags and snapshot_tags
  • ami should by find by union of tags and ami_tags

We tried to merge tags via add_tags local function for snapshot
(BTW when I return back to the code; it always take me a minute to understand how add_tags works :-) )

But it was not done for for ami_tags.

So I would say get_image_by_tags needn't have new parameter. We should just pass right values there via current parameter tags

Anyway, It would be nice to improve logging a bit. We automatically fallback to finding image by tags if it cannot be find by name. But it is impossible to identify from logs.
It will be much simpler to analyze issue when one could see a message.

DEBUG ... Cannot find ami by name {metadata.image_name}. Going to try by tags ...

Choose a reason for hiding this comment

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

TY

@lslebodn
Copy link

lslebodn commented Jan 22, 2026

Cloudimg support python3.6+

Python3.9 introduced simple way of creating a new dictionary with the merged keys (dict1 | dict2)

It would allow "oneliner" fix

-            self.get_image_by_tags(metadata.tags)
+            self.get_image_by_tags(metadata.tags | metadata.snapshot_tags)

One would just need to ensure both are dict (none None)

@JAVGan JAVGan force-pushed the fix_import_snapshot_issue branch from 59fa787 to 93cb2fd Compare January 22, 2026 13:07
@JAVGan
Copy link
Contributor Author

JAVGan commented Jan 22, 2026

@lslebodn I've changed this PR so instead of using a new parameter for get_image_by_tags it passes the snapshot_tags from metadata alongside the tags, thanks a lot for the suggestion!

Since this is not using Python3.9 exclusively I've opted out for a solution which is widely compatible instead of the operator |.

I'll add a new commit on top of it to improve the logs.

@rbikar FYI

@JAVGan
Copy link
Contributor Author

JAVGan commented Jan 22, 2026

@lslebodn PTAL again

@JAVGan JAVGan requested a review from lslebodn January 22, 2026 13:15
@JAVGan JAVGan force-pushed the fix_import_snapshot_issue branch 2 times, most recently from f74d17b to 7fc2c9e Compare January 22, 2026 14:16
This commit changes the `get_image_by_tags` call on `publish` method for
`AWSService` so it will search for the snapshot tags alonside the
requested tags.

We need that to ensure the snapshot was properly created in order to
avoid retrieving the wrong AMI during the search.

Refers to SPSTRAT-615

Signed-off-by: Jonathan Gangi <jgangi@redhat.com>
This commit adds more debug logs for the filtering of images and
snapshots on AWSService.

Signed-off-by: Jonathan Gangi <jgangi@redhat.com>
@JAVGan JAVGan force-pushed the fix_import_snapshot_issue branch from 7fc2c9e to f19a3bd Compare January 22, 2026 14:33
@JAVGan JAVGan requested a review from lslebodn January 22, 2026 14:33
Copy link

@lslebodn lslebodn left a comment

Choose a reason for hiding this comment

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

LGTM

@rbikar please re-review

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.

4 participants