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: Automate colima delete command with confirmation #3061

Closed
wants to merge 4 commits into from

Conversation

SmartManoj
Copy link
Contributor

What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?

image

Action link


Give a summary of what the PR does, explaining any non-trivial design decisions

Increased ATTEMPT_LIMIT to 4 to check the inference ( QEMU works in 3rd attempt) lima-vm/lima#2455 (comment)


Other references

Copy link
Contributor

@neubig neubig left a comment

Choose a reason for hiding this comment

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

Thanks for attempting to fix the Colima problems! But sorry, could you explain more?

Why do you think that 3 attempts may be insufficient but 4 would be better?

Also, why delete after 3 attempts when the maximum number of attempts is 4?

@SmartManoj
Copy link
Contributor Author

SmartManoj commented Jul 23, 2024

Increased ATTEMPT_LIMIT to 4 to check the inference ( QEMU works in 3rd attempt) lima-vm/lima#2455 (comment)

first two attempts failed with msg="[hostagent] QEMU did not exit in 3m0s, forcibly killing QEMU"
third one with msg="error starting docker: cannot restart, VM not previously started" (inference => 1st essential requirement is satisfied in ~7mins (3+3+1))

Action link


Why do you think that 3 attempts may be insufficient but 4 would be better?

The timeout issue persists for 2 attempts. So, need to check colima stop or colima delete is efficient.

So on 3rd attempt, it will stop, and 4th attempt it will delete.

Also, why delete after 3 attempts when the maximum number of attempts is 4?

a fall back method

@SmartManoj
Copy link
Contributor Author

Another failed action

@neubig
Copy link
Contributor

neubig commented Jul 24, 2024

Thanks! But do we know why colima is failing? Do we have any evidence that this actually fixes the problem? It seems like this is trying things out without any guarantee that this actually fixes the problem...

@SmartManoj
Copy link
Contributor Author

do we know why colima is failing?

Due to Github actions resource contention? lima-vm/lima#2455 (comment)

@neubig
Copy link
Contributor

neubig commented Jul 24, 2024

I think this is probably fixed by #3097 , so let's close this for now. If we still end up getting flaky tests we could reconsider!

@neubig neubig closed this Jul 24, 2024
@SmartManoj
Copy link
Contributor Author

I think this is probably fixed by #3097

It didn't. #3097 (comment)

@@ -99,8 +99,8 @@ jobs:
break
else
echo "Failed to start Colima. Attempt $i/$ATTEMPT_LIMIT."
if [ $i -eq $ATTEMPT_LIMIT ]; then
colima delete
if [ $i -eq $(($ATTEMPT_LIMIT - 1)) ]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC: @tobitege

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it delete after every failed attempt (except the last one), like this?

if [ $i -lt $ATTEMPT_LIMIT ]; then

Copy link
Contributor Author

@SmartManoj SmartManoj Jul 25, 2024

Choose a reason for hiding this comment

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

Yes but if [ $i -lt $ATTEMPT_LIMIT ]; then it will delete.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I'll change that in my PR then

Copy link
Contributor Author

@SmartManoj SmartManoj Jul 25, 2024

Choose a reason for hiding this comment

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

, like this?

Did you mean unlike this?

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.

3 participants