teuthology: use test user name from the config #2071
Conversation
|
This is a good change; let's just get this tested with .e.g. a single scheduled job in sepia |
phlogistonjohn
changed the title
phlogistonjohn
requested review from
kamoltat and
amathuria
and removed request for
a team
|
OK, is that something you would like me to do, or something you will take care of? |
|
https://pulpito.ceph.com/phlogistonjohn-2025-07-31_13:58:12-orch-wip-phlogistonjohn-testing-1-2025-07-22-0801-distro-default-gibba/ |
| # sentinel value to indicate the default user value is to be used. | ||
| # None and the empty string are reserved for specifying no user value | ||
| # to be backwards compatible with older code. | ||
| _default_user = '.' |
There was a problem hiding this comment.
Not sure I understand this, why 'None' does not work, in the context of the change it is the same.
Introducing the _default_user with value '.', instead of, for example, 'ubuntu', overcomplicates the code.
There was a problem hiding this comment.
I guess the comment doesn't explain the situation enough. The issue at hand is twofold - I do not want to use the username ubuntu on my test systems, I find it confusing as I mainly test on non-ubuntu systems. I have my teuthology configuration use something like cephtest. However, a few places in the code did not honor that existing configuration parameter.
The function canonicalize_hostname does not treat None as meaning "use the default user from the config" it is something more like "don't include a username component in the resulting hostname string".
I did not want to change this behavior because I didn't want to have to find all the existing callers and potentially change them.
I did consider making a sentinel object - like _deafult_user = object() but that would require a change to the type annotation and I thought that'd be ugly. I thought '.' seems like an obvious and safe sentinel value as it's not a real username and is akin to "here" in unix file systems and some other tools.
There was a problem hiding this comment.
From what I understood, the cannonicalize_hostname takes user name defaults = 'ubuntu', when user is None, it should return hostname without any user "@" suffix.
I don't think we ever used user=None. However you're saying that it would
be still useful to keep this possibility to suppress any user.
I would suggest to drop the comment above, and instead add doctoring for the canonicalize_hostname adding description of what is expected to be done and parameters and return value.
And as soon as we're touching the function signature, could you please add return typing for it, like -> str:
There was a problem hiding this comment.
And instead of confusing _default_user it would be better to user self descriptive naming. Since _canonicalize_hostname_default_user_sentinel is too long, maybe just use _default_user_sentinel or _user_sentinel.
There was a problem hiding this comment.
From what I understand about sentinel pattern the default value is just an object(), but I am not strictly against longly . character, it just looks strange, and introduce hidden behavior, in case someone call the function user='.'.
| _default_user = '.' | ||
|
|
||
| def canonicalize_hostname(hostname, user: Optional[str] = _default_user): | ||
| user = get_test_user() if user == _default_user else user |
There was a problem hiding this comment.
The get_test_user already has default value 'ubuntu' for 'test_user' config parameter.
|
@zmc ping |
phlogistonjohn
force-pushed
the
jjm-hax-2025
branch
from
a73e32f to
094179b
Compare
|
I guess the new behavior can be covered by a unit test, like adding a case to TestHostnames by https://github.com/ceph/teuthology/blob/main/teuthology/test/test_misc.py#L252-L313 |
A few places in the code try to use a configurable name but others effectively hard-code "ubuntu". Fix these cases to support more flexible user naming. Signed-off-by: John Mulligan <[email protected]>
phlogistonjohn
force-pushed
the
jjm-hax-2025
branch
from
094179b to
21b21c9
Compare
Also more places can be updated to respect |
|
@zmc as per our call yesterday: I have apparently been doing tests with this patch applied for weeks. I'm doing one more run today with it today after having rebased on teuthology main. |
|
Today's test run showed no regressions, FWIW. |
|
Also, I'd like to keep the scope of this change fairly small - I only really make use of it in my local teuthology setup. It's one where I don't generally test ubuntu based systems at all and feels weird to create a ubuntu user. I don't really expect to change the sepia lab values and start confusing everyone who is already used to this "quirk". Ultimately, I am just trying to carry fewer custom patches for that setup by up-streaming this. Thanks! |
|
@phlogistonjohn can you link the run here please? |
A few places in the code try to use a configurable name but others effectively hard-code "ubuntu". Fix these cases to support more flexible user naming.