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

Rewrite integration CI #117

Merged
merged 10 commits into from
Sep 8, 2023
Merged

Rewrite integration CI #117

merged 10 commits into from
Sep 8, 2023

Conversation

vyzigold
Copy link
Contributor

@vyzigold vyzigold commented Sep 6, 2023

As we discussed with Leif previously, I rewrote the CI to be closer to how we actually use sg-core.

  • The original metrics job is now split into 2. Ceilometer and Collectd have their own job, which makes it easier to see which one of them is failing.
  • I switched the transport for ceilometer from the amqp transport plugin to use sg-bridge and the socket transport plugin, just like we do in STF
  • I added a new job, which tests sg-core with ceilometer by using the TCP as transport

@vyzigold
Copy link
Contributor Author

vyzigold commented Sep 6, 2023

I think it might be a good idea to somehow make it, that the jobs don't get executed twice on PR. Because it's getting a little cluttered now with 16 jobs running instead of just 8. I thought it might be solved by changing the "on" parameter in the workflow file, but for example here: openstack-k8s-operators/telemetry-operator#172 The "on" parameter is also on: [push, pull_request], but the CI job runs only once.

@vyzigold vyzigold force-pushed the jwysogla-debug-ci branch 2 times, most recently from 8489ab5 to a8c18c2 Compare September 6, 2023 10:51
@vyzigold
Copy link
Contributor Author

vyzigold commented Sep 7, 2023

I tried to remove the mysql workaround from the workflow to see, if it's still required. I'll delete the commit if this fails.

@vyzigold
Copy link
Contributor Author

vyzigold commented Sep 7, 2023

I deleted the commit. Both of the workarounds (mysql, postgres) seem to be required to deploy devstack.

@vyzigold
Copy link
Contributor Author

vyzigold commented Sep 7, 2023

I'm done trying things, this is ready for review

.github/workflows/integration.yml Outdated Show resolved Hide resolved
$TEST_IMAGE bash $PROJECT_ROOT/ci/integration/metrics/run_bridge.sh
- name: Install collectd
run: |
sudo apt-get install collectd
Copy link
Member

Choose a reason for hiding this comment

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

What version of collectd does this install?

Copy link
Member

Choose a reason for hiding this comment

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

I see now you've just moved what was already happening up here. Still curious, but this seems fine :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have ubuntu on hand right now, but I found this: https://www.ubuntuupdates.org/package/core/jammy/universe/updates/collectd So I guess it could be 5.12?

--volume ${{ github.workspace }}:$PROJECT_ROOT:z --workdir $PROJECT_ROOT \
$TEST_IMAGE bash $PROJECT_ROOT/ci/integration/metrics/collectd/run_validation.sh
#-------------------------------------------------------------------------------
ceilometer-metrics-bridge:
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that further refactoring could happen here using composite actions (https://docs.github.com/en/actions/creating-actions/creating-a-composite-action) in order to remove some of this duplication.

I am not advocating doing it here though, just noting in case a future refactoring was warranted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree. For the metric jobs for example, the first ~5 steps are the same. I was looking into the composite actions but I decided to not implement them. As I understand the documentation, we would either need to create a new repository for the actions or we could have them in this repo, but we would probably need to tag the repository. So that we can use the uses: action@tag syntax. I haven't tried this, but after reading the docs and understanding it like this, I decided to not use the composite actions to not create some confusion in the repository tags just for CI deduplication.

Copy link
Member

@leifmadsen leifmadsen left a comment

Choose a reason for hiding this comment

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

Went through the code and some existing scripts etc to see how this looks, and see some of the interactions. Couple minor questions I was able to answer. Looks good. Thanks!

@leifmadsen leifmadsen added the testing-ci Testing or CI changes label Sep 7, 2023
@vyzigold
Copy link
Contributor Author

vyzigold commented Sep 8, 2023

Just FYI. The logging CI job just failed. I looked at the logs and it failed to install the golang package, so not our fault right now. I'll restart the job.

@leifmadsen
Copy link
Member

Looks good to me! Thanks for doing this!

Copy link
Contributor

@csibbitt csibbitt left a comment

Choose a reason for hiding this comment

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

LGTM

@leifmadsen leifmadsen merged commit e91a06b into master Sep 8, 2023
14 checks passed
@leifmadsen leifmadsen deleted the jwysogla-debug-ci branch September 8, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing-ci Testing or CI changes
Development

Successfully merging this pull request may close these issues.

3 participants