Skip to content

teuthology: use test user name from the config #2071

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

teuthology: use test user name from the config #2071

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
10 changes: 9 additions & 1 deletion teuthology/misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,15 @@ def host_shortname(hostname):
else:
return hostname.split('.', 1)[0]

def canonicalize_hostname(hostname, user: Optional[str] ='ubuntu'):
# 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 = '.'
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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='.'.


def canonicalize_hostname(
hostname: str, user: Optional[str] = _default_user
) -> str:
user = get_test_user() if user == _default_user else user
Copy link
Contributor

Choose a reason for hiding this comment

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

The get_test_user already has default value 'ubuntu' for 'test_user' config parameter.

hostname_expr = hostname_expr_templ.format(
lab_domain=config.lab_domain.replace('.', r'\.'))
match = re.match(hostname_expr, hostname)
Expand Down
9 changes: 5 additions & 4 deletions teuthology/task/ssh_keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,21 +93,22 @@ def tweak_ssh_config(ctx, config):
"""
Turn off StrictHostKeyChecking
"""
user = misc.get_test_user()
run.wait(
ctx.cluster.run(
args=[
'echo',
'StrictHostKeyChecking no\n',
run.Raw('>'),
run.Raw('/home/ubuntu/.ssh/config'),
run.Raw(f'/home/{user}/.ssh/config'),
run.Raw('&&'),
'echo',
'UserKnownHostsFile ',
run.Raw('/dev/null'),
run.Raw('>>'),
run.Raw('/home/ubuntu/.ssh/config'),
run.Raw(f'/home/{user}/.ssh/config'),
run.Raw('&&'),
run.Raw('chmod 600 /home/ubuntu/.ssh/config'),
run.Raw(f'chmod 600 /home/{user}/.ssh/config'),
],
wait=False,
)
Expand All @@ -119,7 +120,7 @@ def tweak_ssh_config(ctx, config):
finally:
run.wait(
ctx.cluster.run(
args=['rm',run.Raw('/home/ubuntu/.ssh/config')],
args=['rm',run.Raw(f'/home/{user}/.ssh/config')],
wait=False
),
)
Expand Down
Loading