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

ci: Improve reliability of ansible tasks #345

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

ldoktor
Copy link
Contributor

@ldoktor ldoktor commented Jan 31, 2024

Let's allow to re-try tasks that make sense and a few cleanups.

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Apart from a minor typo in the header of 7f2271d this looks good to me and hopefully the retries do the trick. Thanks!

@ldoktor
Copy link
Contributor Author

ldoktor commented Jan 31, 2024

Changes:

  • Fixed typo in a commit message, no code changes

Copy link
Member

@portersrc portersrc left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -46,6 +46,8 @@
url: https://storage.googleapis.com/kubernetes-release/release/{{ k8s_version }}/bin/linux/{{ target_arch }}/{{ item }}
dest: /usr/local/bin
mode: '+x'
retries: 3
Copy link
Member

Choose a reason for hiding this comment

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

hi @ldoktor ! Good to know what ansible has now retry for tasks (or exists a long time and I never noticed:(

I noticed that on this file you didn't retry unarchive tasks. Isn't it idempotent?

@@ -20,6 +20,8 @@
repo: https://github.com/bats-core/bats-core.git
dest: bats-core
force: yes
retries: 3
Copy link
Member

Choose a reason for hiding this comment

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

Install test dependencies can be retried too I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@@ -13,10 +13,16 @@
name: "{{ build_pkgs[ansible_distribution | lower] }}"
state: present
- block:
- name: Download and extract Go tarball
- name: Download Go tarball
Copy link
Member

Choose a reason for hiding this comment

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

This answers one of my questions on the previous commit :D

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps run get_url with force: true to update the tgz in case it exists and the content changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand the workers should be always clean and locally we might benefit of the caching, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Caching is nice. Then rename go.tar.gz -> go{{ go_version }}.linux-{{ target_arch }}.tar.gz so that a version update will force the download?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that sounds good

@@ -37,7 +37,7 @@
ignore_errors: yes
- name: Install kustomize
shell: |
curl -s "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash
curl -s --retry 3 --retry-delay 10 "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash
Copy link
Member

Choose a reason for hiding this comment

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

Luckily the curl version installed on CI supports --retry? I think it was a patch to kata that it didn't work because it has used an old curl, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The --retry option is fairly old, the problem in kata was the --retry-on-all-errors

@wainersm
Copy link
Member

wainersm commented Feb 2, 2024

hi @ldoktor ! Good idea using "retries everywhere"; I should make it a habit on my codes for sure.

@ldoktor
Copy link
Contributor Author

ldoktor commented Feb 5, 2024

Changes:

  • Added retries to Install test dependencies (under tests/e2e/ansible/install_test_deps.yml)

@ldoktor
Copy link
Contributor Author

ldoktor commented Feb 7, 2024

Changes:

  • go.tar.gz -> go{{ go_version }}.linux-{{ target_arch }}.tar.gz filename to ensure we don't use old version in install_test_deps.yml and install_build_deps.yml

Copy link
Member

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

Thanks @ldoktor !

@ldoktor
Copy link
Contributor Author

ldoktor commented Feb 15, 2024

@wainersm do you find this ready to be merged (after rebase) or is there something I should improve to avoid unnecessary extra rebases?

the network in CI environment tends to break from time to time, let's
allow up to 3 retries for tasks that support it and that use external
sources.

Fixes: confidential-containers#339

Signed-off-by: Lukáš Doktor <[email protected]>
Network tends to be flaky, let's use get_url module with retries to
ensure external resources are fetched and unpack them afterwards
(unfortunatelly the unarchive module does not supports retries).

Signed-off-by: Lukáš Doktor <[email protected]>
as network tends to be flaky, allow curl to retry in case it fails to
fetch the kustomize.

Signed-off-by: Lukáš Doktor <[email protected]>
we do want to do multiple steps and doing that in shell makes sense in
those cases, ignore the ansible warning explicitly.

Signed-off-by: Lukáš Doktor <[email protected]>
@ldoktor
Copy link
Contributor Author

ldoktor commented Feb 16, 2024

No changes, just rebased

@wainersm wainersm merged commit 31a4efa into confidential-containers:main Feb 19, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants