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

chore(ci): make libbpf as the default attacher #1077

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

vimalk78
Copy link
Collaborator

@vimalk78 vimalk78 commented Nov 24, 2023

  • tag existing 'latest' image as 'latest-bcc'
  • tag existing 'latest-libbpf' as 'latest'
  • make libbpf image as default released image
  • use libbpf as attacher for PR image
  • make unit_test run with libbpf attacher
  • build kepler with libbpf attacher for rpm build
  • rename integration_test to integration_test_bcc
  • rename integration_test_libbpf to integration_test

@vimalk78 vimalk78 force-pushed the libbpf-default-image branch 2 times, most recently from cceecea to 0eef904 Compare November 24, 2023 04:40
@vimalk78
Copy link
Collaborator Author

Cc: @rootfs @SamYuan1990

@vimalk78 vimalk78 changed the title chore(ci): make libbpf image as the default image chore(ci): make libbpf as the default attacher Nov 24, 2023
.github/workflows/image.yml Outdated Show resolved Hide resolved
.github/workflows/image.yml Outdated Show resolved Hide resolved
@sthaha sthaha marked this pull request as draft November 24, 2023 05:34
@vimalk78 vimalk78 force-pushed the libbpf-default-image branch 2 times, most recently from 6e77fa4 to 08227c7 Compare November 24, 2023 06:19
@vimalk78 vimalk78 marked this pull request as ready for review November 24, 2023 06:19
@vimalk78
Copy link
Collaborator Author

@rootfs @marceloamaral can i pls get rights to request a PR review?

Copy link
Collaborator Author

@vimalk78 vimalk78 Nov 24, 2023

Choose a reason for hiding this comment

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

this file is renamed
old name: integration_test_libbpf.yml
new name: integration_test.yml
there are no other changes in file

Copy link
Collaborator Author

@vimalk78 vimalk78 Nov 24, 2023

Choose a reason for hiding this comment

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

this file is renamed
old name: integration_test.yml
new name: integration_test_bcc.yml
there are no other changes in file

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be changed to Build and push kepler latest (bcc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@vimalk78 vimalk78 force-pushed the libbpf-default-image branch 4 times, most recently from 4aaf4d1 to e6db42a Compare November 24, 2023 07:20

on:
workflow_call:

env:
OUTPUT_DIR: "_output/"
FILE_NAME: "kepler_libbpf.tar.gz"
FILE_NAME: "kepler.tar.gz"
Copy link
Collaborator

Choose a reason for hiding this comment

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

kepe

Suggested change
FILE_NAME: "kepler.tar.gz"
FILE_NAME: "kepler_bcc.tar.gz"

@@ -24,30 +24,30 @@ jobs:
- name: install libbpf
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- name: install libbpf
- name: install bcc

@@ -130,17 +130,17 @@ jobs:
context: .
platforms: linux/amd64
push: true
tags: quay.io/sustainable_computing_io/kepler:latest, quay.io/sustainable_computing_io/kepler:${{ github.event.inputs.release }}
tags: quay.io/sustainable_computing_io/kepler:latest, quay.io/sustainable_computing_io/kepler:${{ github.event.inputs.release }}-bcc
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we have the latest tag here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lets take this question in some other PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason I ask is because I am assuming that the previous release tagged bcc as latest and now we want to switch that to libbpf. Would that be the case if we add latest here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tags field for both images has quay.io/sustainable_computing_io/kepler:latest

@vimalk78 vimalk78 force-pushed the libbpf-default-image branch 2 times, most recently from 9fb5b58 to b71c37a Compare November 24, 2023 10:12
@vimalk78
Copy link
Collaborator Author

vimalk78 commented Nov 24, 2023

@rootfs we wanted this PR to merge before we merged xgboost PR #1052
now can kepler compile without xgboost ? kepler needs #include <xgboost/c_api.h> to build.

@sthaha
Copy link
Collaborator

sthaha commented Nov 24, 2023

@rootfs could we please revert #1052 , wait until this PR gets merged so that a release without xgboost can be made?

@vimalk78 vimalk78 force-pushed the libbpf-default-image branch 11 times, most recently from e4d38f5 to 9b991e5 Compare November 27, 2023 11:44
  - tag existing 'latest' image as 'latest-bcc'
  - tag existing 'latest-libbpf' as 'latest'
  - make libbpf image as default released image
  - use libbpf as attacher for PR image
  - make unit_test run with libbpf attacher
  - updated unit_test workflow to install libbpf from source
  - build kepler with libbpf attacher for rpm build
  - rename integration_test to integration_test_bcc
  - rename integration_test_libbpf to integration_test

Signed-off-by: Vimal Kumar <[email protected]>
  Updated following files to not use normalized_cpu_arch.csv, and use
  cpus.yaml
  - updated build/Dockerfile
  - doc/dev/prepare_dev_env.sh

Signed-off-by: Vimal Kumar <[email protected]>
@vimalk78 vimalk78 force-pushed the libbpf-default-image branch from 9b991e5 to e04259c Compare November 27, 2023 11:49
@rootfs
Copy link
Contributor

rootfs commented Nov 27, 2023

Per @SamYuan1990 comment, the bcc code will held in the source and be removed in the next release.
Following this PR, the libbpf ebpf module should be also built on the CI. It is currently updated manually.

@rootfs rootfs merged commit e9e1c8b into sustainable-computing-io:main Nov 27, 2023
@SamYuan1990
Copy link
Collaborator

Per @SamYuan1990 comment, the bcc code will held in the source and be removed in the next release. Following this PR, the libbpf ebpf module should be also built on the CI. It is currently updated manually.

so we will remove bcc code after 0.8 release?

@rootfs
Copy link
Contributor

rootfs commented Nov 27, 2023

@vimalk78 we need a CI step to make genlibbpf

@SamYuan1990 yes, let's remove all the bcc code in 0.8 once we are fully confident on libbpf.

- name: Prepare environment
run: |
sudo apt-get install -y cpuid
cd doc/ && sudo ./dev/prepare_dev_env.sh && cd -
git config --global --add safe.directory /kepler
- name: Run
run: |
make test-verbose
sudo apt remove libbpf-dev
Copy link
Collaborator

Choose a reason for hiding this comment

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

if libbpf-dev is not enough of our libbpf driver, we should make a deb package at https://github.com/sustainable-computing-io/kepler-ci-artifacts and update our kepler-action.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rootfs / @vimalk78 / @sthaha could you please help double confirm if we need sustainable-computing-io/kepler-action#87 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

new issue related to this PR as #1086

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if libbpf-dev is not enough of our libbpf driver, we should make a deb package at

yes @SamYuan1990, opening a PR for the same. in this PR i wanted to test what works. will update kepler-action, and then we dont needs this setup in CI

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vimalk78 I am going to sleep, please take a look at #1086 briefly, the libbpf-dev ubuntu distribution has the static link file but it seems the version for ubuntu distribution is too old. or mismatch with the golang code. hence I am worry about if libbpf is ready for use? as if we build it from source each time.... dependence maintain as OpenSSF or SBOM sounds like disaster for us.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -277,6 +277,7 @@ test: ginkgo-set tidy-vendor

test-verbose: ginkgo-set tidy-vendor
@echo TAGS=$(GO_BUILD_TAGS)
@echo GOENV=$(GOENV)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need echo this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the information echoed in this line is helpful in checking errors we get from go build, if any.

@SamYuan1990
Copy link
Collaborator

some further comments been added related pr code but not release process.

vprashar2929 pushed a commit to vprashar2929/kepler that referenced this pull request Dec 19, 2023
…-io#1077)

* chore(ci): make libbpf image as the default image

  - tag existing 'latest' image as 'latest-bcc'
  - tag existing 'latest-libbpf' as 'latest'
  - make libbpf image as default released image
  - use libbpf as attacher for PR image
  - make unit_test run with libbpf attacher
  - updated unit_test workflow to install libbpf from source
  - build kepler with libbpf attacher for rpm build
  - rename integration_test to integration_test_bcc
  - rename integration_test_libbpf to integration_test

Signed-off-by: Vimal Kumar <[email protected]>

* fix(build): use correct cpus data file

  Updated following files to not use normalized_cpu_arch.csv, and use
  cpus.yaml
  - updated build/Dockerfile
  - doc/dev/prepare_dev_env.sh

Signed-off-by: Vimal Kumar <[email protected]>

---------

Signed-off-by: Vimal Kumar <[email protected]>
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.

5 participants