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

JeOS: expand image information #14873

Merged
merged 1 commit into from May 19, 2022
Merged

Conversation

jlausuch
Copy link
Contributor

@jlausuch jlausuch commented May 10, 2022

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files

@jlausuch
Copy link
Contributor Author

select * from image_size
1652191977687102429 x86_64  20220509 JeOS-for-kvm-and-xen     openSUSE-Tumbleweed-Minimal-VM.x86_64-1.0.0-kvm-and-xen-Snapshot20220509.qcow2 https://openqa.opensuse.org/tests/2338516 uncompressed 216.125         Tumbleweed

Copy link
Member

@pdostal pdostal left a comment

Choose a reason for hiding this comment

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

This is nice 😄

tests/jeos/image_info.pm Outdated Show resolved Hide resolved
record_info('GlobalReserve', "$globalreserve_mb MB");

# Get size of the root directories in KB (except /proc).
my $dirs = script_output('du -d 1 --block-size=1K --exclude=/proc /');
Copy link
Member

Choose a reason for hiding this comment

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

IMO not that useful, except maybe /usr and /var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but as I'm running that in some loop way for all dirs, we can always chose to represent which data to display in the Graphs. Otherwise I would need to create a list of directories to analyze which may vary in the future. This way we just collect data for all directories under / and later one we show what we want.

Copy link
Member

Choose a reason for hiding this comment

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

You already exclude /proc and you'd have to exclude some others anyway as the sizes are nonsensical (/sys, /dev/, /.snapshots)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer to collect only /usr and /var or do:

du -d 1 --block-size=1K --exclude=/proc --exclude=/sys --exclude=/dev  /

?
Actually /.snapshots is not empty when running this module (maybe due to firstboot)

Copy link
Member

Choose a reason for hiding this comment

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

IMO just mentioning those two is simpler. Actually, there's also /tmp, unlike on TW that's not a tmpfs yet.

/.snapshots doesn't show up as empty, but the returned size is useless as it's backed by btrfs magic 🌈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will get the info of those 2 dirs then, thanks for the feedback!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I also forgot about usr-merge only being in TW... For SLE we'd need /(s)bin and /lib(64) as well. I also forgot about /boot... So excluding "special" mounts is probably the best idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, moving back to my original idea :D

@jlausuch jlausuch force-pushed the jeos_image_info branch 4 times, most recently from 6e7a041 to bc121e1 Compare May 12, 2022 19:40
@jlausuch
Copy link
Contributor Author

All suggestions have been addressed.
New VR: https://openqa.opensuse.org/tests/2344846

@jlausuch jlausuch force-pushed the jeos_image_info branch 2 times, most recently from 2606f0d to 8550f9c Compare May 13, 2022 13:53
@jlausuch
Copy link
Contributor Author

@jlausuch
Copy link
Contributor Author

Anyway, I'm gonna work on a small modification.

@jlausuch jlausuch force-pushed the jeos_image_info branch 2 times, most recently from 753b85f to be6793d Compare May 13, 2022 14:34
@jlausuch
Copy link
Contributor Author

With latest changes: https://openqa.opensuse.org/tests/2345457#
Before merge, I need to remove this comment return if (get_required_var('CASEDIR') !~ m/^sle$|^opensuse$/);, so please don't merge before that.

@jlausuch jlausuch added the Ready Ready for review label May 13, 2022
@jlausuch
Copy link
Contributor Author

@Vogtinator @mloviska any more feedback on this?

tests/jeos/image_info.pm Outdated Show resolved Hide resolved
tests/jeos/image_info.pm Outdated Show resolved Hide resolved
Copy link
Contributor

@mloviska mloviska left a comment

Choose a reason for hiding this comment

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

LGTM, just neat picking comments :)

@jlausuch jlausuch force-pushed the jeos_image_info branch 2 times, most recently from 45dae97 to f6e1a74 Compare May 19, 2022 09:59
@mloviska mloviska merged commit e2e4b5c into os-autoinst:master May 19, 2022
@ggardet
Copy link
Collaborator

ggardet commented May 20, 2022

This PR breaks tests on RPi 2/3/4 on O3: https://progress.opensuse.org/issues/111362

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready Ready for review
Projects
None yet
5 participants