Warm reboot: Add support for docker upgrade#292
Conversation
Signed-off-by: Jipan Yang <[email protected]>
Signed-off-by: Jipan Yang <[email protected]>
sonic_installer/main.py
Outdated
| @click.option('-y', '--yes', is_flag=True, callback=abort_if_false, | ||
| expose_value=False, prompt='New docker image will be installed, continue?') | ||
| @click.option('--cleanup_image', is_flag=True, help="Clean up old docker images") | ||
| @click.argument('container_name', metavar='<container_name>', required=True, type=click.Choice(["swss", "snmp", "lldp"])) |
There was a problem hiding this comment.
add bgp, pmon, dhcp_relay,telemetry,teamd?
sonic_installer/main.py
Outdated
|
|
||
| if url.startswith('http://') or url.startswith('https://'): | ||
| click.echo('Downloading image...') | ||
| urllib.urlretrieve(url, DEFAULT_IMAGE_PATH, reporthook) |
There was a problem hiding this comment.
We should handle failure cases of this API call here.
sonic_installer/main.py
Outdated
| cmd = "docker exec -it " + container_name + " orchagent_restart_check" | ||
| proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, shell=True) | ||
| result = proc.stdout.read().rstrip() | ||
| if result != "RESTARTCHECK succeeded": |
There was a problem hiding this comment.
I guess this PR is relying on sonic-net/sonic-swss#562
Can we check the orchagent_restart_check return code instead of string?
There was a problem hiding this comment.
It could be changed to check return code. What is the benefit of doing that?
The concern here is that, theoretically, the success of running orchagent_restart_check program itself doesn't indicate the warm restart readiness of orchagent.
There was a problem hiding this comment.
The benefit is if any result string from "orchagent_restart_check" changed for some reason, we don't have to change code here.
orchagent_restart_check should return EXIT_SUCCESS code if check is successful and return EXIT_FAILURE if check is failed or some Error code if the program is not running due to other reasons.
There was a problem hiding this comment.
Both the result string and exit code are from orchagent_restart_check, but exit code may also indicate status of orchagent_restart_check itself. The result strings are designed for the result check purposely. Anyway, don't want to spend too much time debating this kind of issues. Either one works for now, will change to use exit code to settle the issue.
|
It would be nice if we can add the docker rollback CLI if the previous images was not removed from the system. |
jipanyang
left a comment
jipanyang
left a comment
There was a problem hiding this comment.
Yes, rollback CLI is good. Will focus on docker upgrade for this PR.
sonic_installer/main.py
Outdated
|
|
||
| if url.startswith('http://') or url.startswith('https://'): | ||
| click.echo('Downloading image...') | ||
| urllib.urlretrieve(url, DEFAULT_IMAGE_PATH, reporthook) |
sonic_installer/main.py
Outdated
| cmd = "docker exec -it " + container_name + " orchagent_restart_check" | ||
| proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, shell=True) | ||
| result = proc.stdout.read().rstrip() | ||
| if result != "RESTARTCHECK succeeded": |
There was a problem hiding this comment.
It could be changed to check return code. What is the benefit of doing that?
The concern here is that, theoretically, the success of running orchagent_restart_check program itself doesn't indicate the warm restart readiness of orchagent.
…er tag from docker meta label Signed-off-by: Jipan Yang <[email protected]>
| run_command("docker load < %s" % image_path) | ||
| if tag == None: | ||
| # example image: docker-lldp-sv2:latest | ||
| tag = get_docker_tag_name(image_latest) |
There was a problem hiding this comment.
Do we want to check return tag value, e,g, if it is "unknown" and then we do things differently here?
There was a problem hiding this comment.
Some tag is needed. If no tag is provided, default is "unknown". Before tag is built into docker meta label, I don't want to enforce the existence of tag.
Signed-off-by: Jipan Yang <[email protected]>
|
@jleveque do you have more comments for the changes? |
Signed-off-by: Jipan Yang <[email protected]>
Description Add unit test cases to test eeprom_tlvinfo.py, now code coverage is 80% Motivation and Context There is no UT available for eeprom_tlvinfo.py previously. Using a HEX file to mock EEPROM content. Take this mocked EEPROM as input and test the functions. Signed-off-by: Kebo Liu <[email protected]>
5 participants
Signed-off-by: Jipan Yang [email protected]
- What I did
Add sonic_installer upgrade_docker command to support docker upgrade
- How I did it
- How to verify it
sonic_installer upgrade_docker
Before docker upgrade
docker upgrade
- 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)