DaveWysochanskiRH
commented
2 years ago @audrabaker some feedback.
-
commit message needs limited to some reasonable # of columns (70?). Do a 'git show' on other commits and you'll get an idea. It shows as one very long line.
$ git show audrabaker/master commit f1e647b28389b793ce54425d306e0dda150786af Author: Audra Mitchell <aubaker@redhat.com> Date: Tue Jan 18 15:43:26 2022 -0500 Add logic to determine if we save a stripped vmcore Currently we do not check to see if running makedumpfile and creating a stripped vmcore is actually worthwhile. In some situations we may see little to no benefit to stripping down a vmcore and Signed-off-by: Audra Mitchell <aubaker@redhat.com> -
I think the name of SpaceSavePercent is ok but probably needs improved. We probably need vmcore in there because this only applies to vmcores. Also, it's makedumpfile related so I wonder if we can clue in there too. The only other parameter though is "VmcoreDumpLevel" so maybe "VmcoreDumpSavePercent" or is a little long but at least it's grouped somewhat with the other name.
-
Suggest moving the following code inside vmcore.strip_extra_pages since inside that function you're logging other things and you've got the sizes there.
+ if vmcore.strip_extra_pages(): + dur = int(time.time() - start) + newsize = vmcore_path.stat().st_size + log_info("Stripped size: %s" % human_readable_size(newsize)) + log_info("Makedumpfile took %d seconds and saved %s" % (dur, human_readable_size(oldsize - newsize))) -
Another thought I had was is there any point in running makedumpfile if the vmcore is smaller than a certain size, but I'm not suggesting you do it. I think not but I don't have a good solution for that and don't really think yet another config variable is a good idea. Maybe a hardcoded minimum but maybe that's not worthwhile either. Alternatively, it might be worthwhile to have a button to avoid makedumpfile on a vmcore but that's a separate discussion / patch.
Let me do some testing and see if there's anything else I see and let you know.
lgtm-com[bot]
codecov-commenter
mgrabovsky
audrabaker
Currently we do not check to see if running makedumpfile and creating a stripped vmcore is actually worthwhile. In some situations we may see little to no benefit to stripping down a vmcore and replacing the original with the newly stripped vmcore. For those situation, this patch adds the config variable SpaceSavePercent which is meant to indicate the percentage of space a stripped vmcore will need to save for us to choose to save it in place of the original. The proposed default is 10%. This patch also adjusts the logic so that the relevant message about makedumpfile will be printed (i.e. if makedumpfile is not run or we don't use the stripped vmcore from makedumpfile we do not print how much space we saved).
Signed-off-by: Audra Mitchell aubaker@redhat.com #445