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

[Active Directory] Bind mounts in the joining context manager #1627

Merged
merged 2 commits into from
Mar 31, 2023
Merged

[Active Directory] Bind mounts in the joining context manager #1627

merged 2 commits into from
Mar 31, 2023

Conversation

CarlosNihelton
Copy link
Collaborator

This concludes LP: 2013079.

realm --install <target> requires system bind-mounts to work properly.

realm --install <target> requires system bind-mounts to work properly.
@CarlosNihelton CarlosNihelton marked this pull request as ready for review March 30, 2023 15:34
@dbungert
Copy link
Collaborator

What problem were you encountering when you were running this through curtin? If humanely possible I don't want new mount code. Can you tell me more about this pam-auth-update thing? I'm worried about a more fundamental problem and want to understand that better.

@CarlosNihelton
Copy link
Collaborator Author

What problem were you encountering when you were running this through curtin? If humanely possible I don't want new mount code. Can you tell me more about this pam-auth-update thing? I'm worried about a more fundamental problem and want to understand that better.

Sure. This is more related to realm, though. When running it with the --install option, it temporarily chroots into the target system. I guess I didn't notice that before because I was chrooting (manually, with bind mounts) very often to monitor the target system while post-installation was progressing, like running apt commands to check what packages has been installed.

I couldn't reproduce the pam-auth-update issue I've seen before, so I took a break from it and focused on this one.

Without the bind-mounts, realm join --install should fail with a very unhelpful error message.

yield hostname_process
finally:
# Restoring the live session hostname.
hostname_process = run_command(['hostname', hostname_current])
if hostname_process.returncode:
log.info("Failed to restore live session hostname")
for bind in binds:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for bind in binds:
for bind in reversed(binds):

I think for the mounts your doing the order won't matter, but it does sometimes. Let's reverse the order in case we add more to the list later.

Sometimes unmounting order matters. Let's take the defensive approach.
@CarlosNihelton CarlosNihelton merged commit e3f7b38 into canonical:main Mar 31, 2023
@CarlosNihelton CarlosNihelton deleted the fix-ad-bind-mounts branch March 31, 2023 14:35
@ogayot ogayot mentioned this pull request Apr 3, 2023
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.

2 participants