Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

ci/openshift-ci: Use loop instead of curl --retry-all-errors #5806

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

ldoktor
Copy link
Contributor

@ldoktor ldoktor commented Jan 20, 2024

The --retry-all-errors argument is not supported on the CI runners, use a busy loop instead.

@katacontainersbot katacontainersbot added the size/tiny Smallest and simplest task label Jan 20, 2024
@ldoktor
Copy link
Contributor Author

ldoktor commented Jan 20, 2024

@wainersm @gkurz the selinux enablement PR used curl --retry-all-errors which is not supported on the CI runners, this should work.

@@ -56,7 +56,11 @@ else
port=$(oc get service/http-server-service -o jsonpath='{.spec.ports[0].nodePort}')
fi

curl "${host}:${port}${hello_file}" -s -o hello_msg.txt --retry 60 --retry-delay 1 --retry-all-errors
rm -f hello_msg.txt
for i in $(seq 60); do
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @ldoktor , good catch.

One suggestion is to use the waitForProcess (e.g. waitForProcess $wait_time $sleep_time "$cmd") utility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@ldoktor
Copy link
Contributor Author

ldoktor commented Jan 22, 2024

Changes:

  • use waitForProcess
  • new commit to remove the pre-existing file still consider a hot-fix, I'd like to remove the file completely just wait for the pipeline to stabilize first
  • new commit to avoid failures as by default it raises exception on non-zero return unless treated which prevents the cleanup execution (which is something I'd also want to properly address later)

Copy link
Contributor

@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 !

@wainersm
Copy link
Contributor

@beraldoleal mind to review this one?

The --retry-all-errors argument is not supported on the CI runners, use
a busy loop instead.

Fixes: kata-containers#5802

Signed-off-by: Lukáš Doktor <[email protected]>
To avoid using pre-existing file ensure it's not present by deleting it.

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

ldoktor commented Jan 23, 2024

Changes: Added the fixes to commit a message

@wainersm
Copy link
Contributor

/test

@ldoktor
Copy link
Contributor Author

ldoktor commented Jan 23, 2024

Changes: Removed the controversial commit that treats grep exception as it's not really related to the changes that are required no.

Copy link
Member

@gkurz gkurz left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ldoktor !

@gkurz
Copy link
Member

gkurz commented Jan 23, 2024

/test

@wainersm wainersm merged commit f15be37 into kata-containers:main Jan 23, 2024
7 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/tiny Smallest and simplest task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants