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.

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

Increase reliability of host network setup #310

Closed
wants to merge 1 commit into from
Closed

Increase reliability of host network setup #310

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
34 changes: 20 additions & 14 deletions suse_migration_services/units/setup_host_network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,6 @@ def main():
resolv_conf = os.sep.join(
[root_path, 'etc', 'resolv.conf']
)
if not os.path.exists(resolv_conf):
raise DistMigrationNameResolverException(
'Could not find {0} on migration host'.format(resolv_conf)
)
sysconfig_network_providers = os.sep.join(
[root_path, 'etc', 'sysconfig', 'network', 'providers']
)
Expand All @@ -64,15 +60,16 @@ def main():
)
try:
log.info('Running setup host network service')
Command.run(
[
'mount', '--bind', sysconfig_network_providers,
'/etc/sysconfig/network/providers'
]
)
system_mount.add_entry(
sysconfig_network_providers, '/etc/sysconfig/network/providers'
)
if os.path.exists(sysconfig_network_providers):
Command.run(
[
'mount', '--bind', sysconfig_network_providers,
'/etc/sysconfig/network/providers'
]
)
system_mount.add_entry(
sysconfig_network_providers, '/etc/sysconfig/network/providers'
)
for network_setup in glob.glob(sysconfig_network_setup):
if os.path.isfile(network_setup):
shutil.copy(
Expand All @@ -87,7 +84,14 @@ def main():
resolv_conf, '/etc/resolv.conf'
)
else:
log.info('Empty {0}, bind mounting /etc/resolv.conf to {0}'.format(resolv_conf))
# ensure resolv.conf won't be a symlink on migrated system, create an empty file instead to allow bind mount
if os.path.islink(resolv_conf):
log.info('{0} is a symlink, renaming it to {0}.pre-migration, bind mounting /etc/resolv.conf to {0}'.format(resolv_conf))
os.replace(resolv_conf,resolv_conf+'.pre-migration')
open(resolv_conf,'w').close()
Copy link
Member

Choose a reason for hiding this comment

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

But if resolv.conf is a symlink then the user could have pointed it to any location that has resolution information. If we stick an empty file in it's place there is a good chance that network access will not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default on SLES15, /etc/resolv.conf is a symlink to /run/netconfig/resolv.conf (in tmpfs), which is not available on SLES16. This is why we need to move the symlink away (we can't bind mount on top of a symlink).

On SLES16, /etc/resolv.conf is no longer a symlink but a file, which is managed by NetworkManager. If the file is empty, it will replaced by NetworkManager by the information from DHCP (or NM configuration).

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we at least check if the symlink points to the default location? If it doesn't we should probably not write an empty file and instead copy the content from the pointed to file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I'll add some checks to see if the symlink is resolvable (ie not pointing to /run). If it is, it should be left intact, if not, we could do the renaming.

WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

I think if the symlink points to the default location we can be reasonably certain that the user has not added any custom configuration and that a dhcp based setup will work. If it it points somewhere else we should read the content of the target and dump that in the new file being created.

Copy link
Collaborator

Choose a reason for hiding this comment

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

why should bind mounting a symlink be a problem ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the various issues the patch is trying to resolve are due to /etc/resolv.conf symlink to /run/netconfig/resolv.conf being broken on a SLES 16 system (ie the migration live image), since netconfig is not present anymore, nor wicked.

  • test on /etc/resolv.conf existence fails (os.path.exists will follow the link)
  • bind mount on /etc/resolv.conf symlink will fail too (symlink target doesn't exist)

I'm not super happy with touching the migration system either, before we start the migration process.

I'll test another approach, by creating a symlink /run/netconfig/resolv.conf in the sle16 migration image and reports if it works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fcrozat as we control the migration environment I would also be in favor to "fix" the migration live image and the container to provide a proper environment rather than adding code to do this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fcrozat something different. I see you work from forks and for whatever reason I cannot make the github actions to run from your contribution. Normally I have a button to "approve and run" the actions but I believe it's because the SUSE org is a private one this doesn't work. It would be better to add you to the project and you can work from branches which will then also run the actions and tests. I believe I don't have permissions to add members but maybe @rjschwei or @aosthof can do this ? Thanks much

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 didn't want to bother you with my initial tests but I'm fine following your regular workflow.

else:
log.info('Empty {0}, bind mounting /etc/resolv.conf to {0}'.format(resolv_conf))

Command.run(
[
'mount', '--bind', '/etc/resolv.conf',
Expand Down Expand Up @@ -154,6 +158,8 @@ def log_network_details():


def has_host_resolv_setup(resolv_conf_path):
if not os.path.exists(resolv_conf_path):
return False
with open(resolv_conf_path, 'r') as resolv:
for line in resolv:
# check there is useful information in the remaining lines
Expand Down