Skip to content

Conversation

@zmc
Copy link
Member

@zmc zmc commented Sep 18, 2024

pulpito-ng is gaining the ability to kill runs, and for non-admin users will match the user's github username with the test run's owner value to decide if the user is allowed to kill the run (admin users can kill any run). To generate the run name, we normally use the active user account's name.

It could be confusing if a user passed --owner but then still sees their unix account name used to name the run; this change lets us use more consistent naming without adding yet another flag (--user) to teuthology-suite.

Edit: The pulpito-ng PR: ceph/pulpito-ng#51

@zmc zmc requested a review from VallariAg September 18, 2024 17:45
VallariAg
VallariAg previously approved these changes Sep 19, 2024
Copy link
Member

@VallariAg VallariAg left a comment

Choose a reason for hiding this comment

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

If I understand it right, the run's username and owner both will be set to value of --owner input with this PR?

In the current implementation of pulpito-ng's kill-run, we check if run belongs to user by looking at the owner (which is stored at job level) and never decide ownership based on the username. So if you use --owner zmc then pulpito-ng should see you as the owner of the run even if the run name starts with "zack-". Let me know if that's not the case!

Here are ownership checks in:
t-api: https://github.com/ceph/teuthology-api/pull/55/files#diff-02d3c9819201f270bdf30499d8e189622dc5d969e96d6582aef93f0e9e029c48R31-R33

pulpito-ng:

  1. https://github.com/ceph/pulpito-ng/pull/51/files#diff-aeb4ab3acaee02c9aa9c960ec5ee9b3e56c316802dc7e7ae0284b5702016c027R59
  2. https://github.com/ceph/pulpito-ng/pull/51/files#diff-5db46c41bfe3b6e18f278e8721577a9c75851c2f3e6e519a61b6fe54a74d7b03R39

@zmc zmc force-pushed the suite-user branch 2 times, most recently from f8e7b76 to fcfe995 Compare September 23, 2024 20:12
@zmc
Copy link
Member Author

zmc commented Sep 23, 2024

Thanks @VallariAg for pointing out my conflation of terms - the change is more about making the run name consistent with what the user might expect. I've reworded the PR description and cleaned up the code a little more to make things more clear.

Copy link
Member

@VallariAg VallariAg left a comment

Choose a reason for hiding this comment

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

Looks great! Tested it when scheduling https://pulpito.ceph.com/vallari-2024-10-01_13:15:47-nvmeof:basic-main-distro-default-smithi/
Owner and username matched as expected!

@zmc zmc merged commit e2eb3a9 into main Oct 7, 2024
@zmc zmc deleted the suite-user branch October 7, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants