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

Jump to bottom

Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker #2278

Merged
merged 4 commits into from
Aug 8, 2022
Merged

Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker #2278

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a3bbf46
Fix issues for sonic_installer upgrade-docker and sonic_installer rol…
Junchao-Mellanox Jul 20, 2022
58b1cec
Allow mgmt-framework to be warm restart
Junchao-Mellanox Jul 21, 2022
b4a0720
Fix review comment and add unit test
Junchao-Mellanox Aug 5, 2022
d690b7b
Improve coverage
Junchao-Mellanox Aug 8, 2022
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
2 changes: 2 additions & 0 deletions sonic_installer/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ def run_command(command):
if proc.returncode != 0:
sys.exit(proc.returncode)

return out.rstrip("\n")
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

return out.rstrip("\n")

To make coverage easier, do you really need this change? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since I will rollback line 942, I don't need this change any more.


# Run bash command and return output, raise if it fails
def run_command_or_raise(argv, raise_exception=True):
click.echo(click.style("Command: ", fg='cyan') + click.style(' '.join(argv), fg='green'))
Expand Down
24 changes: 13 additions & 11 deletions sonic_installer/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def get_docker_tag_name(image):


def echo_and_log(msg, priority=LOG_NOTICE, fg=None):
if priority >= LOG_ERR:
if priority == LOG_ERR:
# Print to stderr if priority is error
click.secho(msg, fg=fg, err=True)
else:
Expand Down Expand Up @@ -647,7 +647,7 @@ def set_fips(image, enable_fips):
bootloader = get_bootloader()
if not image:
image = bootloader.get_next_image()
if image not in bootloader.get_installed_images():
if image not in bootloader.get_installed_images():
echo_and_log('Error: Image does not exist', LOG_ERR)
sys.exit(1)
bootloader.set_fips(image, enable=enable_fips)
Expand Down Expand Up @@ -743,7 +743,8 @@ def cleanup():
"swss",
"syncd",
"teamd",
"telemetry"
"telemetry",
"mgmt-framework"
]

# Upgrade docker image
Expand Down Expand Up @@ -872,17 +873,17 @@ def upgrade_docker(container_name, url, cleanup_image, skip_check, tag, warm):
# this is image_id for image with "latest" tag
image_id_latest = get_container_image_id(image_latest)

for id in image_id_all:
if id != image_id_latest:
# Unless requested, the previoud docker image will be preserved
if not cleanup_image and id == image_id_previous:
continue
run_command("docker rmi -f %s" % id)
if cleanup_image:
# Unless requested, the previoud docker image will be preserved
for id in image_id_all:
if id != image_id_latest and id == image_id_previous:
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

id != image_id_latest and id == image_id_previous

This check is weird, seems like image_id_latest != image_id_previous, and not related to the loop var. Could you double check? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will fix

run_command("docker rmi -f %s" % id)
break

exp_state = "reconciled"
state = ""
# post warm restart specific procssing for swss, bgp and teamd dockers, wait for reconciliation state.
if warm_configured is True or warm:
if warm_app_names and (warm_configured is True or warm):
count = 0
for warm_app_name in warm_app_names:
state = ""
Expand Down Expand Up @@ -938,7 +939,8 @@ def rollback_docker(container_name):
version_tag = ""
for id in image_id_all:
if id != image_id_previous:
version_tag = get_docker_tag_name(id)
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

get_docker_tag_name

you can change the implementation of get_docker_tag_name(), and this function should be easy to unit test, you still need some mock. #Closed

Copy link
Collaborator Author

@Junchao-Mellanox Junchao-Mellanox Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi qi, thanks for the suggestion. I would to rollback this change because I found the real issue is in sonic build system. Here is my analyze:

During my previous manual test with a local image, I found get_docker_tag_name() cannot return the correct tag name of docker. The return value of get_docker_tag_name() does not equal to output of docker images.

However, I re-visit the logic today, and I found:

  1. get_docker_tag_name() tries to get the docker tag by querying a docker label specified by sonic make file.
cmd = "docker inspect --format '{{.ContainerConfig.Labels.Tag}}' " + image
  1. This label is added in slave.mk. See https://github.com/sonic-net/sonic-buildimage/blob/736c739bf492a73c60cfca000b853c040f53a104/slave.mk#L895
	docker build --squash --no-cache \
...
		--label Tag=$(SONIC_IMAGE_VERSION) \
  1. The real docker tag is added in build_debian.sh. See https://github.com/sonic-net/sonic-buildimage/blob/736c739bf492a73c60cfca000b853c040f53a104/files/build_templates/sonic_debian_extension.j2#L697
sudo LANG=C DOCKER_HOST="$DOCKER_HOST" chroot $FILESYSTEM_ROOT docker tag {{imagename}}:latest {{imagename}}:"${SONIC_IMAGE_VERSION}"

In theory, item#2 and item#3 shall get the same value of ${SONIC_IMAGE_VERSION}. But it doesn't. I got from build log:

  1. For item#2, it prints:
docker-database.gz.log:Step 23/25 : LABEL Tag=syslog-rate-limit.0-dirty-20220804.115836
  1. For item#3, it prints:
sonic-mellanox.bin.log:+ sudo LANG=C DOCKER_HOST= chroot ./fsroot-mellanox docker tag docker-database:latest docker-database:syslog-rate-limit.0-dirty-20220804.165951

As you can see, value of SONIC_IMAGE_VERSION is different in the same build. I suppose SONIC_IMAGE_VERSION value shall always be the same in the same build.

The issue is probably related to the timestamp in the SONIC_IMAGE_VERSION. However, I found timestamp is not part of SONIC_IMAGE_VERSION in official image. So, maybe we can ignore this issue.

version_tag = run_command("docker images --format '{{{{.ID}}}} {{{{.Tag}}}}' | grep {} | awk '{{print $2}}'".format(id))
break

# make previous image as latest
run_command("docker tag %s:%s %s:latest" % (image_name, version_tag, image_name))
Expand Down