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

Fixes for teuthology-node-cleanup #1949

Merged
merged 3 commits into from
Jun 17, 2024
Merged

Fixes for teuthology-node-cleanup #1949

merged 3 commits into from
Jun 17, 2024

Conversation

zmc
Copy link
Member

@zmc zmc commented May 28, 2024

This mainly gets the command working without having to invoke with --owner for each user who has jobs running.

Also fix the return type and docstring, which were inconsistent.

Signed-off-by: Zack Cerza <[email protected]>
scripts/node_cleanup.py Outdated Show resolved Hide resolved
@kamoltat
Copy link
Member

Some context as to why you are fixing this in the PR description would be super helpful, thanks!

kamoltat
kamoltat previously approved these changes Jun 17, 2024
Copy link
Member

@kamoltat kamoltat left a comment

Choose a reason for hiding this comment

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

Other than the minor comment, LGTM

Comment on lines 24 to 25
owner_nodes = by_owner.setdefault(node['locked_by'], [])
owner_nodes.append(node)
Copy link
Member

Choose a reason for hiding this comment

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

I know that owner_nodes is a reference to by_owner, can't we just purely use by_owner? Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

by_owner is a dict where keys are e.g. "scheduled_zack@teuthology", and values are lists of dicts containing node data, whereas owner_nodes is a reference to one of those lists - but you're right that there's no real point in holding this reference.

I'll replace the above two lines with:

by_owner.setdefault(node['locked_by'], []).append(node)

Copy link
Member

@kamoltat kamoltat left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing the comment

@kamoltat kamoltat merged commit 8e97141 into main Jun 17, 2024
8 checks passed
@zmc zmc deleted the nodecleanup branch June 17, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants