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.

Sign up for GitHub

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

Jump to bottom

Only consider image properties of public/community images. #724

Merged
merged 5 commits into from
Sep 2, 2024
Merged

Only consider image properties of public/community images. #724

merged 5 commits into from
Sep 2, 2024

Conversation

garloff
Copy link
Contributor

@garloff garloff commented Aug 30, 2024

We may not have the luxury to test against a pristine project, so there may be private images or shared images from others. Only consider provider images with "public" or "community" visibility.
https://wiki.openstack.org/wiki/Glance-v2-community-image-visibility-design

Note: While testing PR #713, this did not fail the overall test, I saw a failed message on check-image-properties which was due to private images ... which could have been avoided.

@garloff garloff added the standards Issues / ADR / pull requests relevant for standardization & certification label Aug 30, 2024
@garloff garloff self-assigned this Aug 30, 2024
@garloff garloff mentioned this pull request Aug 30, 2024
Merged
@berendt
Copy link
Member

berendt commented Aug 30, 2024

This should be configurable if a project is used in which there are no public or community images but only private images. Otherwise, such a project could not be used for testing. In the future, we will at least have the case that some of our projects do not contain images that are public/community. The images in the projects are still managed by us and contain all the necessary meta information.

mbuechse
mbuechse reviewed Aug 31, 2024
View reviewed changes
Tests/iaas/entropy/entropy-check.py Outdated
@@ -428,7 +428,7 @@ def main(argv):
try:
logger.debug(f"Connecting to cloud '{cloud}'")
with openstack.connect(cloud=cloud, timeout=32) as conn:
all_images = conn.list_images()
all_images = list(filter(lambda x: x.visibility == 'public' or x.visibility == 'community', conn.list_images()))
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT, the image-metadata check uses server-side filtering, and it only checks public images. Shall we change that as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Three points:

  1. Having consistency between the two checks would always be preferable.
  2. I did not see an obvious way to do server side filtering, but I may not have looked deeply enough. It's always preferable.
  3. I chose public and community because these are controlled by the cloud operator.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, let's change both tests, then?

@garloff
Copy link
Contributor Author

garloff commented Aug 31, 2024

Wouldn't you fail the image-md check if you don't at least have a public Ubuntu 22.04?
(Also I wonder why you provide your standard images as private images... does not seem efficient to me at all.)

Adding an option --all-visibility is not a big deal. Helps in your case if you call the check manually with the option, but not if run from the conformance test suite...

@berendt
Copy link
Member

berendt commented Aug 31, 2024

In at least one environment we have the requirement to work with private images only. It's not about efficiency there. To be as compliant as possible we make an image like the mentioned Ubuntu 22.04 available as a private image in every project of this environment so that it can be used in general. But it is neither public nor community.

@garloff
Copy link
Contributor Author

garloff commented Sep 1, 2024

You can pass --image-visibility="" (or --image-visibility="public,community,shared,private" if you prefer) now to the entropy-check. The SCS conformance test however won't call the entropy checker with these flags ...

@garloff
Copy link
Contributor Author

garloff commented Sep 2, 2024 •
edited
Loading

@mbuechse -- it seems to me that this is good to merge now.
However, I created a PR against your #650 branch (PR #713), which may not be your preferred way to get this in.
Let me know if you prefer me rebasing against main and doing a PR against it.

garloff and others added 5 commits September 2, 2024 20:43
@garloff @mbuechse
Only consider image properties of public/community images.
24d25f3 
We may not have the luxury to test against a pristine project,
so there may be prviate images or shared images from others.
Only consider provider images with "public" or "community" visibility.
https://wiki.openstack.org/wiki/Glance-v2-community-image-visibility-design

Signed-off-by: Kurt Garloff <[email protected]>
@garloff @mbuechse
acquiesce flake8
37121a0 
Signed-off-by: Kurt Garloff <[email protected]>
@garloff @mbuechse
Allows image filtering by visibility.
6459b83 
Default is public,community.
Passing --image-visibility="" will result in no filtering, same as
passing -V "public,community,shared,private".

Signed-off-by: Kurt Garloff <[email protected]>
@garloff @mbuechse
While my flake8 is OK, github's wasn't. Try to fix.
72bff9b 
Signed-off-by: Kurt Garloff <[email protected]>
@mbuechse
fail if no image left after filtering, filter early, make more Pythonic
f5d7987 
Signed-off-by: Matthias Büchse <[email protected]>
@mbuechse mbuechse force-pushed the feat/entropy-check-only-public-images branch from 48cb865 to f5d7987 Compare September 2, 2024 19:02
@mbuechse mbuechse merged commit fb136e9 into issue/650 Sep 2, 2024
3 checks passed
@mbuechse mbuechse deleted the feat/entropy-check-only-public-images branch September 2, 2024 19:03
@garloff
Copy link
Contributor Author

garloff commented Sep 3, 2024

Comments on the last change:

  • I seem to have grown up b/f list comprehensions seem to have been a thing. So list operations are always map(lambda x: ..., list) or filter() in my book. Once you get used to it, these are pretty nice. I agree that list comprehensions are more readable, though, if you're not used to the old-style functional programming syntax.
  • Unsure whether the setup with set(), update() and then the test for emptyness is really better than starting with a default value ... You can specify the option multiple times now in addition to a comma-separated list ...
  • I could have added "" and "*" to identically imply no filtering, this is no longer supported, but no big thing.
  • The message if no images are left is of course an improvement.

As far as I can see, the changes are absolutely fine, though this time (unlike before), not necessarily a clear improvement.

mbuechse added a commit that referenced this pull request Sep 3, 2024
@garloff @mbuechse
Only consider image properties of public/community images. (#724)
7120c5d 
* Only consider image properties of public/community images.

We may not have the luxury to test against a pristine project,
so there may be prviate images or shared images from others.
Only consider provider images with "public" or "community" visibility.
https://wiki.openstack.org/wiki/Glance-v2-community-image-visibility-design

Signed-off-by: Kurt Garloff <[email protected]>

* Allows image filtering by visibility.

Default is public,community.
Passing --image-visibility="" will result in no filtering, same as
passing -V "public,community,shared,private".

Signed-off-by: Kurt Garloff <[email protected]>

* fail if no image left after filtering, filter early, make more Pythonic

Signed-off-by: Matthias Büchse <[email protected]>

---------

Signed-off-by: Kurt Garloff <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Co-authored-by: Matthias Büchse <[email protected]>
mbuechse added a commit that referenced this pull request Sep 3, 2024
@mbuechse @martinmo
@berendt @garloff
Revise entropy check to work without floating IP (#713)
18a3dae 
* Revise entropy check to work without floating IP
* Bugfix: network must be passed to create_server
* Bugfix: set auto_ip=False
* Only consider image properties of public/community images. (#724)

----

Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Kurt Garloff <[email protected]>
Co-authored-by: Martin Morgenstern <[email protected]>
Co-authored-by: Christian Berendt <[email protected]>
Co-authored-by: Kurt Garloff <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbuechse mbuechse mbuechse left review comments

@martinmo martinmo Awaiting requested review from martinmo

Assignees

@garloff garloff

Labels
standards Issues / ADR / pull requests relevant for standardization & certification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@garloff @berendt @mbuechse