-
Notifications
You must be signed in to change notification settings - Fork 661
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
[sonic_installer] don't print errors when installing an image not sup… #1719
[sonic_installer] don't print errors when installing an image not sup… #1719
Conversation
…porting app ext Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
/azpw run |
1 similar comment
/azpw run |
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
…porting app ext (#1719) FIXES: sonic-net/sonic-buildimage#8149 #### What I did Don't print errors if installing app.ext incompatible image. #### How I did it Check for docker.sh existance and take an assumption that if it exists then it is app.ext compatible. #### How to verify it From master image install 202012 image and verify no errors in logs.
…porting app ext
Signed-off-by: Stepan Blyschak stepanb@nvidia.com
FIXES: sonic-net/sonic-buildimage#8149
What I did
Don't print errors if installing app.ext incompatible image.
How I did it
Check for docker.sh existance and take an assumption that if it exists then it is app.ext compatible.
How to verify it
From master image install 202012 image and verify no errors in logs.
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)