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

Migrate CircleCI workflows to GitHub Actions (2/3) #2298

Merged
merged 2 commits into from
Dec 18, 2020

Conversation

AzfaarQureshi
Copy link
Contributor

Which problem is this solving?

As part of issue, this pull request migrates the loadtest, correctness and build-package jobs to GitHub Actions. windows-msi and the publish jobs will be migrated in PR 3/3

Migration Plan

We suggest having CircleCI and GitHub Action jobs run in parallel for a few weeks. After the GitHub Actions jobs are running fine for a week or so and then remove the CircleCI workflows from config.yml
cc- @alolita, @shovnik

@AzfaarQureshi AzfaarQureshi requested a review from a team December 16, 2020 22:52
Comment on lines 135 to 141
loadtestpre:
runs-on: ubuntu-latest
outputs:
matrix: ${{ steps.splitloadtest.outputs.matrix }}
steps:
- name: Checkout Repo
uses: actions/checkout@v2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

loadtestpre does some pre-processing so we can simulate CircleCI testsharding on GitHub Actions. This cuts down the runtime of loadtests from 15mins to ~5mins
This job does the following:

  1. Get a list of all loadtest tests using make testbed-list-loadtest
  2. Split tests into groups of two (TestIdleMode|TestBallastMemory, TestLog10kDPS|TestMetric10kDPS etc)
  3. Create a JSON containing each test grouping. This JSON will be consumed by the strategy.matrix of the loadtest job using fromJSON(). The strategy.matrix is how we get to run the loadtests in parallel

Comment on lines +223 to +209
- name: Create test result archive # some test results have invalid characters
if: ${{ failure() || success() }}
continue-on-error: true
run: tar -cvf test_results.tar testbed/tests/results
- name: Upload test results
if: ${{ failure() || success() }}
continue-on-error: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • making a tarball of the test results because some tests have filenames with invalid characters (namely the 0*0bytes test) which causes the upload action to fail.

  • if: ${{ failure() || success() }} this ensures that test results are uploaded even if the loadtest fails which can be useful for debugging

  • continue-on-error: true Sometimes (rarely, but still) the upload action fails due to a network error on github's side. This makes sure the entire job doesnt fail if the test results fail to upload

@@ -60,7 +60,7 @@ type DataReceiverBase struct {
Port int
}

const DefaultHost = "localhost"
const DefaultHost = "127.0.0.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line was causing an error with the fluentbit log test since localhost could not be resolved on the github actions runners.

@@ -42,7 +42,7 @@ func TestIdleMode(t *testing.T) {
)
defer tc.Stop()

tc.SetResourceLimits(testbed.ResourceSpec{ExpectedMaxCPU: 4, ExpectedMaxRAM: 50})
tc.SetResourceLimits(testbed.ResourceSpec{ExpectedMaxCPU: 4, ExpectedMaxRAM: 55})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the GitHub Action runners the RAM utilization is consistently higher (probably due to some other background processes on the GitHub machine). Loadtests don't pass unless these values are adjusted a little 😞

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #2298 (00caf64) into master (723676e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2298   +/-   ##
=======================================
  Coverage   92.04%   92.04%           
=======================================
  Files         272      272           
  Lines       15345    15345           
=======================================
  Hits        14124    14124           
  Misses        840      840           
  Partials      381      381           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 723676e...00caf64. Read the comment docs.

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
Comment on lines +186 to +190
steps:
- name: Checkout Repo
uses: actions/checkout@v2
- name: Setup Go
uses: actions/[email protected]
with:
go-version: 1.15
- name: Download tool binaries
uses: actions/download-artifact@v2
with:
name: tool-binaries
path: /home/runner/go/bin
- name: Add execute permissions to tool binaries
run: chmod -R +x /home/runner/go/bin
- name: Setup env
run: |
echo "GOPATH=$(go env GOPATH)" >> $GITHUB_ENV
echo "$(go env GOPATH)/bin" >> $GITHUB_PATH
- name: Restore module cache
uses: actions/cache@v2
env:
cache-name: cache-go-modules
with:
path: /home/runner/go/pkg/mod
key: go-pkg-mod-${{ runner.os }}-${{ hashFiles('./go.mod') }}
Copy link
Member

Choose a reason for hiding this comment

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

This code seem to be duplicated a lot between jobs, is there a way to avoid this and have a common step that all jobs will run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I dont think there is a way to reuse steps across jobs as we can't use YAML anchors. It seems like the GitHub team is aware this is a pain point though and working on an alternative feature soon :/

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

@AzfaarQureshi there is an error in the action

Error when evaluating 'strategy' for job 'loadtest'. (Line: 161, Col: 15): Error reading JToken from JsonReader. Path '', line 0, position 0.,(Line: 161, Col: 15): Unexpected value ''

@bogdandrutu
Copy link
Member

@AzfaarQureshi @shovnik I have couple more feedback about this, how can I give that to you? Is it ok for me to file issues and assign to you?

@AzfaarQureshi
Copy link
Contributor Author

AzfaarQureshi commented Dec 18, 2020

@bogdandrutu yup! that works 😄 or you can hit us up on gitter if thats easier

@AzfaarQureshi
Copy link
Contributor Author

AzfaarQureshi commented Dec 18, 2020

@bogdandrutu the workflow actually ran faster (~10 mins) when loadtest didnt depend on the binaries from cross-compile as opposed to now when its running in ~20 mins. Should we revert the change to what it was before?

@bogdandrutu
Copy link
Member

@AzfaarQureshi yes please revert :D

removing sleep from common.sh

removing windows-msi

updating expectd ram usage

removing caching

moving loadtestpre into setup environment

using underscore for setup-environment instead

adding collector binaries and fixing setup-pre job
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
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.

2 participants