CP-13126 Update delphix-platform to install docker from official docker repo instead of docker.io#554
Conversation
3778f1c to
07a49ef
Compare
…er repo instead of docker.io PR URL: https://www.github.com/delphix/delphix-platform/pull/554
07a49ef to
98f827c
Compare
|
nit: delphix is mis-spelled in your PR description. Can you fill in the PR description with meaningful context and testing details please? It's important to note that this is not a stand-alone PR, and must be merged along with PRs in other repos as well. |
| - nftables | ||
| - docker-ce | ||
| - docker-ce-cli | ||
| - containerd.io | ||
| - docker-compose-plugin |
There was a problem hiding this comment.
What is the purpose of all of these dependencies?
| - name: Execute 'sudo systemctl start docker' command | ||
| ansible.builtin.command: sudo systemctl start docker | ||
| register: docker_status_output | ||
| changed_when: false # This command only retrieves info, doesn't change state | ||
| retries: 10 | ||
| delay: 30 | ||
| until: docker_status_output.rc == 0 |
There was a problem hiding this comment.
Can you add a comment explaining why this is needed?
| - name: Gather service facts | ||
| ansible.builtin.service_facts: | ||
| register: services_state | ||
|
|
||
| - name: Verify Docker service is running | ||
| ansible.builtin.debug: | ||
| msg: "Docker service status is {{ services_state.ansible_facts.services['docker.service'].state }}" | ||
| when: services_state.ansible_facts.services['docker.service'].state == 'running' | ||
|
|
||
| - name: Execute 'docker version' command | ||
| ansible.builtin.command: docker version | ||
| register: docker_version_output | ||
| changed_when: false # This command only retrieves info, doesn't change state | ||
|
|
||
| - name: Print the full command output | ||
| ansible.builtin.debug: | ||
| var: docker_version_output.stdout | ||
|
|
||
| - name: Print only the Docker client version | ||
| ansible.builtin.debug: | ||
| msg: "Docker Client Version: {{ docker_version_output.stdout_lines[1] }}" |
There was a problem hiding this comment.
Pull request overview
This PR updates the delphix-platform bootstrap role to install Docker from the official Docker repository instead of using the docker.io package. The change includes replacing the single package installation with multiple official Docker packages and adding verification steps to ensure Docker is properly installed and running.
Changes:
- Replaced
docker.iopackage with official Docker CE packages (docker-ce,docker-ce-cli,containerd.io,docker-compose-plugin) - Added Docker service startup and verification tasks
- Added Docker version checking tasks
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Execute 'sudo systemctl start docker' command | ||
| ansible.builtin.command: sudo systemctl start docker | ||
| register: docker_status_output | ||
| changed_when: false # This command only retrieves info, doesn't change state | ||
| retries: 10 | ||
| delay: 30 | ||
| until: docker_status_output.rc == 0 |
There was a problem hiding this comment.
Using sudo within an Ansible command is redundant and potentially problematic. Ansible tasks already run with the privileges defined by become and become_user. Use the ansible.builtin.systemd module instead, which is the idiomatic way to manage systemd services: ansible.builtin.systemd: name=docker state=started enabled=yes.
| - name: Execute 'sudo systemctl start docker' command | |
| ansible.builtin.command: sudo systemctl start docker | |
| register: docker_status_output | |
| changed_when: false # This command only retrieves info, doesn't change state | |
| retries: 10 | |
| delay: 30 | |
| until: docker_status_output.rc == 0 | |
| - name: Ensure Docker service is started and enabled | |
| ansible.builtin.systemd: | |
| name: docker | |
| state: started | |
| enabled: yes | |
| register: docker_service_result | |
| retries: 10 | |
| delay: 30 | |
| until: not docker_service_result.failed |
| - name: Execute 'sudo systemctl start docker' command | ||
| ansible.builtin.command: sudo systemctl start docker | ||
| register: docker_status_output | ||
| changed_when: false # This command only retrieves info, doesn't change state |
There was a problem hiding this comment.
The comment is misleading. The command sudo systemctl start docker does change state by starting the Docker service, so changed_when: false is incorrect. This should either be removed or the task should use proper change detection based on the actual state change.
| changed_when: false # This command only retrieves info, doesn't change state |
| register: services_state | ||
|
|
||
| - name: Verify Docker service is running | ||
| ansible.builtin.debug: | ||
| msg: "Docker service status is {{ services_state.ansible_facts.services['docker.service'].state }}" | ||
| when: services_state.ansible_facts.services['docker.service'].state == 'running' |
There was a problem hiding this comment.
The ansible.builtin.service_facts module automatically populates ansible_facts.services, so the register directive is unnecessary. Access the service facts directly via ansible_facts.services in subsequent tasks.
| register: services_state | |
| - name: Verify Docker service is running | |
| ansible.builtin.debug: | |
| msg: "Docker service status is {{ services_state.ansible_facts.services['docker.service'].state }}" | |
| when: services_state.ansible_facts.services['docker.service'].state == 'running' | |
| - name: Verify Docker service is running | |
| ansible.builtin.debug: | |
| msg: "Docker service status is {{ ansible_facts.services['docker.service'].state }}" | |
| when: ansible_facts.services['docker.service'].state == 'running' |
| - name: Verify Docker service is running | ||
| ansible.builtin.debug: | ||
| msg: "Docker service status is {{ services_state.ansible_facts.services['docker.service'].state }}" | ||
| when: services_state.ansible_facts.services['docker.service'].state == 'running' |
There was a problem hiding this comment.
The when condition makes this verification task only run when Docker is already running, which defeats the purpose of verification. If Docker is not running, the task silently skips. Consider using ansible.builtin.assert to fail the playbook if Docker is not running, or remove the when condition to always report the status.
| when: services_state.ansible_facts.services['docker.service'].state == 'running' |
|
|
||
| - name: Print only the Docker client version | ||
| ansible.builtin.debug: | ||
| msg: "Docker Client Version: {{ docker_version_output.stdout_lines[1] }}" |
There was a problem hiding this comment.
Accessing stdout_lines[1] without bounds checking could cause an index error if the output format changes or is shorter than expected. Add validation to ensure the list has at least 2 elements before accessing index 1, or use a safer extraction method like regex to find the version line.
| msg: "Docker Client Version: {{ docker_version_output.stdout_lines[1] }}" | |
| msg: >- | |
| Docker Client Version: {{ | |
| (docker_version_output.stdout | |
| | regex_search('Version:\\s*([^\\n]+)', '\\1')) | |
| | default('unknown', true) | |
| }} |
Problem
Provide a clear description of the high-level problem you are trying to
solve. The problem statement should be written in terms of a specific
symptom that affects users or the business. The problem statement should
not be written in terms of the solution. If possible, include a minimal
reproducible example (MRE) with steps to reproduce, expected results,
and actual results.
Solution
Provide a clear description of the high-level solution you have chosen.
If there were other possible solutions that you considered and rejected,
mention those along with the corresponding reasoning. Do not describe
implementation details when writing about the solution; these should go
into the implementation section instead.
Testing Done
Provide a clear description of how this change was tested. At minimum
this should include proof that a computer has executed the changed
lines. Ideally this should include an automated test or an explanation
as to why this pull request has no tests.