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.

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

Add rescue option for OS upgrade failure + test via docker build #121

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

compscidr
Copy link

A first pass at trying to solve: #117

I was able to reproduce the failure mode reliably with the added Dockerfile.

You can try it out by running:
docker build -f tests/Dockerifle . from the main folder of the repo.

The dockerfile uses multistage builds. In the first stage, it uses Ubuntu20.04, applies the playbook. Then in the next stage, it starts from Ubuntu22.04, and then copies the virtualenv from the previous stage, as if an upgrade had been done without cleaning this up. It then tries to run the playbook again and then fails.

I converted the failing task to a block and rescue, and then when the rescue happens, it clobbers the virtual env and then just tries to apply the playbook again. This appears to work succesfully.

The only hangup I had, was trying to apply the playbook during a docker build - there is no systemd and docker can't run in a service while building, so I just added a couple of new default variables that toggle off the restart handlers.

Might not be the best way to do it, but open to suggestions!

Copy link
Owner

@nickjj nickjj left a comment

Choose a reason for hiding this comment

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

Hi, thanks for taking a shot at this. I left in a few comments.

tasks/main.yml Outdated
rescue:
- name: Handle upgrade case
ansible.builtin.file:
path: /usr/local/lib/docker/virtualenv
Copy link
Owner

Choose a reason for hiding this comment

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

We should replace this hard coded path with: "{{ docker__pip_virtualenv }}"

tasks/main.yml Outdated
- name: Handle upgrade case
ansible.builtin.file:
path: /usr/local/lib/docker/virtualenv
state: absent
Copy link
Owner

Choose a reason for hiding this comment

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

Can you quote this?

tasks/main.yml Outdated
ansible.builtin.file:
path: /usr/local/lib/docker/virtualenv
state: absent
- include_tasks: main.yml
Copy link
Owner

Choose a reason for hiding this comment

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

Is this going to re-run everything? I haven't used this pattern in Ansible yet.

Ideally I wonder if we can detect an upgrade occurred before the task runs so we can delete the file before this task runs.

Then we wouldn't need to rescue anything and the whole feature becomes maybe a single task to detect if it an upgrade happened and delete the file, else it does nothing.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, it did re-run everything. Just pushed a new version that is able to detect in advance and not do this.

@compscidr compscidr requested a review from nickjj October 8, 2022 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants