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

Update GitHub runners to use ubuntu-latest since they have nested virt #811

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

jeefy
Copy link
Contributor

@jeefy jeefy commented Aug 5, 2024

@ahrtr
Copy link
Member

ahrtr commented Aug 5, 2024

@fuweid @ivanvc are you ok to use the default runner ( 4 cores)?

@fuweid note that the default runner supports nested virtualization.

@ivanvc I recall the may concern on the benchmark workflow is that it's too long to finish the workflow. Using the default runner (4 cores) may take even longer.

default: "['ubuntu-latest-8-cores']"

Signed-off-by: Jeffrey Sica <[email protected]>
@ivanvc
Copy link
Member

ivanvc commented Aug 5, 2024

@fuweid @ivanvc are you ok to use the default runner ( 4 cores)?

I compared the execution times for test-linux-amd64 and test-linux-amd64-race and they look the same. I think it's because we never used the 8 cores. Our tests use up to 4 cores. I'll check again when the robustness tests finish running.

@ivanvc I recall the may concern on the benchmark workflow is that it's too long to finish the workflow. Using the default runner (4 cores) may take even longer.

It took a long time to run the workflow, but when we were testing with benchstat. We're now using our bench, I never tested the performance with the default 4-core runners. It may be ok, the best would be to check the execution times.

@jeefy, do you have more context for the motivation for switching to the default runners? I'm not sure if I understand the pull request title.

@jeefy
Copy link
Contributor Author

jeefy commented Aug 5, 2024

Glancing at the current robustness tests, they take less than 3 hours to complete so I'd like to at least test that these workflows do-or-do-not fit in the six-hour window.

If they don't, I'd then like to try putting them on runners that the CNCF hosts. :)

@ivanvc
Copy link
Member

ivanvc commented Aug 5, 2024

/ok-to-test

@ivanvc
Copy link
Member

ivanvc commented Aug 5, 2024

Robustness tests take about the same amount of time.

@jeefy, feel free to also change the benchmark runner. I don't think we're saturating the 8 cores.

Did you have a chance to read my previous comment?

@jeefy, do you have more context for the motivation for switching to the default runners? I'm not sure if I understand the pull request title.

@ahrtr
Copy link
Member

ahrtr commented Aug 6, 2024

@jeefy, do you have more context for the motivation for switching to the default runners? I'm not sure if I understand the pull request title.

I think that the motivation is to save cost. CNCF is billed for the large runners, while the regular runners are free.

@ivanvc
Copy link
Member

ivanvc commented Aug 6, 2024

I got it; thanks. The jobs seem to be running fine with the 4 cores. We could also try updating the benchmarking job.

@ahrtr
Copy link
Member

ahrtr commented Aug 6, 2024

@jeefy Please update the following runner to ubuntu-latest as well,

default: "['ubuntu-latest-8-cores']"

@@ -14,7 +14,7 @@ jobs:
with:
count: 100
testTimeout: 200m
runs-on: "['ubuntu-latest-8-cores']"
Copy link
Member

Choose a reason for hiding this comment

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

REF: #612

The original motivation is not for nested virt. The free-tier runner has limitation. It seems that there was runner agent monitoring memory or cpu usage. If the resource usage is higher than expected, the agent will kill the process actions/runner-images#6680 (comment)

By default, the free-tier runner only has 3 cores. I'm not sure that it can do nested virt.

Copy link

@mrbobbytables mrbobbytables Aug 6, 2024

Choose a reason for hiding this comment

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

The free tier has been upgraded 👍 They are 4 core with 16gb of ram now.

EDIT: They now support nested virtualization as well.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

There's an if condition in .github/workflows/tests-template.yml, which can be simplified:

if: ${{ github.repository == 'etcd-io/bbolt' || inputs.runs-on == 'ubuntu-latest' }}

But I can do that in a follow-up PR.

LGTM. Thanks, @jeefy.

@ahrtr
Copy link
Member

ahrtr commented Aug 6, 2024

@jeefy Please squash the commits.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

I will squash the commits this time.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ivanvc, jeefy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahrtr ahrtr merged commit 4272a9c into etcd-io:main Aug 7, 2024
19 checks passed
@jeefy
Copy link
Contributor Author

jeefy commented Aug 7, 2024

Sorry didn't see the comment re: squashing 😅

Thanks!!!

samuelbartels20 pushed a commit to samuelbartels20/bbolt that referenced this pull request Nov 10, 2024
etcd-io#811)

* update github runners to use ubuntu-latest since they are 4c and have nested virt

Signed-off-by: Jeffrey Sica <[email protected]>
Signed-off-by: samuelbartels20 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants