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

MemoryError seen on expire_distros execution #240

Merged
merged 1 commit into from
Nov 14, 2024
Merged

MemoryError seen on expire_distros execution #240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion LabController/cron.hourly/beaker_expire_distros
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
#!/bin/sh
exec flock -n /var/run/beaker_expire_distros.cron.lock beaker-expire-distros
exec flock -n /var/run/beaker_expire_distros.cron.lock beaker-expire-distros --arch=all
73 changes: 59 additions & 14 deletions LabController/src/bkr/labcontroller/expire_distros.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,12 @@ def check_url(url):
def check_all_trees(ignore_errors=False,
dry_run=False,
lab_controller='http://localhost:8000',
remove_all=False):
remove_all=False,
filter=None):
filter_on_arch = True if (filter is not None and filter and 'arch' in filter.keys()) else False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
filter_on_arch = True if (filter is not None and filter and 'arch' in filter.keys()) else False
filter_on_arch = filter and 'arch' in filter

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could streamline this further by converting filter to dict if it is set to None.

proxy = xmlrpc_client.ServerProxy(lab_controller, allow_none=True)
rdistro_trees = []
distro_trees = proxy.get_distro_trees()
distro_trees = proxy.get_distro_trees(filter)
if not remove_all:
for distro_tree in distro_trees:
accessible = False
Expand All @@ -108,21 +110,29 @@ def check_all_trees(ignore_errors=False,
else:
rdistro_trees = distro_trees

print('INFO: expire_distros to remove %d entries for arch %s' % (len(rdistro_trees),
filter['arch'] if (filter_on_arch) else 'unset'))
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do filter.get('arch', 'unset') and maybe skip the filter_on_arch variable altogether, it is only used for this purpose.


# If all distro_trees are expired then something is wrong
# Unless there is intention to remove all distro_trees
if len(distro_trees) != len(rdistro_trees) or remove_all:
if (len(distro_trees) != len(rdistro_trees)) or remove_all:
for distro_tree in rdistro_trees:
if dry_run:
print('Distro marked for remove %s:%d' % (distro_tree['distro_name'],
distro_tree['distro_tree_id']))
distro_tree['distro_tree_id']))
else:
print('Removing distro %s:%d' % (distro_tree['distro_name'],
distro_tree['distro_tree_id']))
proxy.remove_distro_trees([distro_tree['distro_tree_id']])
else:
sys.stderr.write('All distros are missing! Please check your server!\n')
sys.exit(1)

if (len(distro_trees) == 0):
if (filter is None):
Copy link
Contributor

Choose a reason for hiding this comment

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

We have changed the business logic here. In this particular case it will report that distributions are missing if the filter is not set, but if we use `all' it will simply not work and even if the controller is empty we will not report anything as we are going arch by arch. My suggestion is to remove the non-filtered way altogether and always run arch by arch and we can write business logic to take that into account.

sys.stderr.write('All distros are missing! Please check your server!\n')
sys.exit(1)
else:
sys.stderr.write('Stopped removal of all distros for arch %s!! Please check your '
'server.\nYou can manually force removal using --remove-all.\n' %
(filter['arch'] if (filter_on_arch) else 'unset'))

def main():
from optparse import OptionParser
Expand All @@ -137,14 +147,49 @@ def main():
'Defaults to http://localhost:8000.')
parser.add_option('--remove-all', default=False, action='store_true',
help='Remove all distros from lab controller.')
parser.add_option('--name', default=None,
help='Remove all distros with given name. Use "%" for wildcard.')
parser.add_option('--family', default=None,
help='Remove all distros for a given family.')
parser.add_option('--arch', default=None,
help='Remove all distros for a given architecture. When set to "all", '
'steps thru each available arch to reduce memory usage.')
options, args = parser.parse_args()
try:
check_all_trees(options.ignore_errors,
options.dry_run,
options.lab_controller,
options.remove_all)
except KeyboardInterrupt:
pass
startmsg = str("INFO: expire_distros running with --lab-controller=" + options.lab_controller)
for i in range(1,len(sys.argv)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idea, but for sure not required:

startmsg += ' '.join(sys.argv[1:])

Copy link
Contributor

Choose a reason for hiding this comment

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

This would work for sure, but do we want to report it that way? I mean, we are not using these options from argv, but rather parsed options (L157). I would work with that instead data structure instead.

startmsg += ' ' + sys.argv[i]
print('%s' % (startmsg))
filter = {}
if options.name:
filter['name'] = options.name
if options.family:
filter['family'] = options.family

arch_list = []
if options.arch:
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean having all is the same as not filtering at all. We should prepare the value before and we can skip this if else completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have thought about this further and I think there is no point in having an alternative behavior between None arch and all. We are basically doing the same thing, the difference is whether we iterate over the dataset at once or not. I think we should unify this and just go per arch as doing it all at once will always be a problem on larger instances.

if options.arch == "all":
arch_list = [ "x86_64", "ppc", "ppc64le", "ppc64", "i386", "s390", "s390x", "aarch64", "ia64", "arm", "armhfp" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Question - this list is quite fragile, isn't there an API that can tell us what kind of architectures we have?

else:
arch_list = [ options.arch ]
for arch in arch_list:
filter['arch'] = arch
try:
check_all_trees(options.ignore_errors,
options.dry_run,
options.lab_controller,
options.remove_all,
filter)
except KeyboardInterrupt:
pass
else:
try:
check_all_trees(options.ignore_errors,
options.dry_run,
options.lab_controller,
options.remove_all,
filter)
except KeyboardInterrupt:
pass


if __name__ == '__main__':
Expand Down
Loading