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

[sonic_installer] don't print errors when installing an image not sup… #1719

Merged
merged 1 commit into from
Aug 3, 2021
Merged

[sonic_installer] don't print errors when installing an image not sup… #1719

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
13 changes: 9 additions & 4 deletions sonic_installer/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

SYSLOG_IDENTIFIER = "sonic-installer"
LOG_ERR = logger.Logger.LOG_PRIORITY_ERROR
LOG_WARN = logger.Logger.LOG_PRIORITY_WARNING
LOG_NOTICE = logger.Logger.LOG_PRIORITY_NOTICE

# Global Config object
Expand Down Expand Up @@ -332,6 +333,7 @@ def migrate_sonic_packages(bootloader, binary_image_version):
echo_and_log("Error: SONiC package migration cannot proceed due to missing docker folder", LOG_ERR, fg="red")
return

docker_started = False
with bootloader.get_rootfs_path(new_image_dir) as new_image_squashfs_path:
try:
mount_squash_fs(new_image_squashfs_path, new_image_mount)
Expand All @@ -342,22 +344,25 @@ def migrate_sonic_packages(bootloader, binary_image_version):
mount_bind(new_image_docker_dir, new_image_docker_mount)
mount_procfs_chroot(new_image_mount)
mount_sysfs_chroot(new_image_mount)
# Assume if docker.sh script exists we are installing Application Extension compatible image.
if not os.path.exists(os.path.join(new_image_mount, os.path.relpath(DOCKER_CTL_SCRIPT, os.path.abspath(os.sep)))):
echo_and_log("Warning: SONiC Application Extension is not supported in this image", LOG_WARN, fg="yellow")
return
run_command_or_raise(["chroot", new_image_mount, DOCKER_CTL_SCRIPT, "start"])
docker_started = True
run_command_or_raise(["cp", packages_path, os.path.join(new_image_mount, tmp_dir, packages_file)])
run_command_or_raise(["touch", os.path.join(new_image_mount, "tmp", DOCKERD_SOCK)])
run_command_or_raise(["mount", "--bind",
os.path.join(VAR_RUN_PATH, DOCKERD_SOCK),
os.path.join(new_image_mount, "tmp", DOCKERD_SOCK)])
run_command_or_raise(["chroot", new_image_mount, "sh", "-c", "command -v {}".format(SONIC_PACKAGE_MANAGER)])
except SonicRuntimeException as err:
echo_and_log("Warning: SONiC Application Extension is not supported in this image: {}".format(err), LOG_ERR, fg="red")
else:
run_command_or_raise(["chroot", new_image_mount, SONIC_PACKAGE_MANAGER, "migrate",
os.path.join("/", tmp_dir, packages_file),
"--dockerd-socket", os.path.join("/", tmp_dir, DOCKERD_SOCK),
"-y"])
finally:
run_command_or_raise(["chroot", new_image_mount, DOCKER_CTL_SCRIPT, "stop"], raise_exception=False)
if docker_started:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

docker_started

Why you need this check? If the DOCKER_CTL_SCRIPT could not be stop more than once, it's a bug. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiluo-msft This check prevents doing "stop" in case we haven't done "start" at line 351

Copy link
Contributor

Choose a reason for hiding this comment

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

I know your intention. My point here is that a service script in general should allow stop a stopped service and return 0. So I think this is a bug inside /usr/lib/docker/docker.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not like it cannot stop it if it is stopped already. Here we added a check - if DOCKER_CTL_SCRIPT exists or not. If it exists then we continue the flow and start docker, if not we return from the function but a cleanup is still required. In case DOCKER_CTL_SCRIPT is missing this line will fail and print error. So a check is added "if docker_started", if it was started DOCKER_CTL_SCRIPT exists and we can execute this line, if not - not even needed to execute stop.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not like it cannot stop it if it is stopped already.

I checked files/docker/docker, and it is using start-stop-daemon --stop --pidfile "$DOCKER_SSD_PIDFILE" --retry 10 in the stop branch. I think the option --oknodo will treat noop as success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiluo-msft It is not what I fix in this PR. The intention is to not call DOCKER_CTL_SCRIPT stop in case DOCKER_CTL_SCRIPT is missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification!

run_command_or_raise(["chroot", new_image_mount, DOCKER_CTL_SCRIPT, "stop"], raise_exception=False)
umount(new_image_mount, recursive=True, read_only=False, remove_dir=False, raise_exception=False)
umount(new_image_mount, raise_exception=False)

Expand Down