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

Revert automatic bpfman image loading into KIND cluster #113

Conversation

frobware
Copy link
Contributor

@frobware frobware commented Aug 27, 2024

Previously, the Makefile was modified to load the bpfman image
unconditionally into the KIND cluster. This commit reverts that
change. To load the bpfman image into the KIND cluster, use the
./hack/kind-load-image.sh script as a discrete step.

For example:

$ export BPFMAN_IMG=docker.io/library/bpfman:latest

$ cd bpfman
$ docker build -f Containerfile.bpfman.local -t $BPFMAN_IMG . 

$ cd bpfman-operator
$ make run-on-kind

# Note the current image.
$ echo "$(kubectl get -n bpfman daemonsets.apps bpfman-daemon -o jsonpath='{.spec.template.spec.containers[?(@.name=="bpfman")].image}')"
quay.io/bpfman/bpfman:latest

$ ./hack/kind-load-image.sh -v bpfman-deployment $BPFMAN_IMG
docker save  --output "/tmp/bpfman-5G4T1G/image-1.tar" "docker.io/library/bpfman:latest"
kind load image-archive --name "bpfman-deployment" "/tmp/bpfman-5G4T1G/image-1.tar"

$ kubectl set image -n bpfman daemonset/bpfman-daemon bpfman=$BPFMAN_IMG
daemonset.apps/bpfman-daemon image updated

$ kubectl rollout status -n bpfman daemonset/bpfman-daemon
daemon set "bpfman-daemon" successfully rolled out

# Verify that we are running the new image.
$ echo "$(kubectl get -n bpfman daemonsets.apps bpfman-daemon -o jsonpath='{.spec.template.spec.containers[?(@.name=="bpfman")].image}')"
docker.io/library/bpfman:latest

# Notice the two bpfman images we now have in the KIND cluster
$ docker exec -it bpfman-deployment-control-plane /bin/bash
root@bpfman-deployment-control-plane:/# crictl images | grep bpfman
docker.io/library/bpfman-agent                  latest               b61f99a39fca8       501MB
docker.io/library/bpfman-operator               latest               521c4ad502091       180MB
docker.io/library/bpfman                        latest               b16a909169931       405MB
quay.io/bpfman/bpfman                           latest               b13146c64b45f       56.2MB
quay.io/bpfman/csi-node-driver-registrar        v2.9.0               50013f94a28d1       10.6MB

This reverts:

This reverts commit c6cafcf.

Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
This reverts commit 6729ecd.

Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.61%. Comparing base (c6cafcf) to head (3fe1275).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #113   +/-   ##
=======================================
  Coverage   27.61%   27.61%           
=======================================
  Files          81       81           
  Lines        6999     6999           
=======================================
  Hits         1933     1933           
  Misses       4877     4877           
  Partials      189      189           
Flag Coverage Δ
unittests 27.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@anfredette anfredette left a comment

Choose a reason for hiding this comment

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

With these changes reverted, does make run-on-kind build and load the local bpfman-operator and bpfman-agent images by default?

I was testing w/Mohammed's pr #102 yesterday, and that didn't seem to be happening. I had to explicitly load upstream images using:

BPFMAN_IMG="quay.io/afredette/bpfman:latest" BPFMAN_OPERATOR_IMG="quay.io/mmahmoud/bpfman-opertor:tcx-support" BPFMAN_AGENT_IMG="quay.io/mmahmoud/bpfman-agent:tcx-support" make run-on-kind

@anfredette
Copy link
Contributor

Also, can you spell out the steps needed for someone to use a locally built bpfman image as opposed to a remote image with the same name and tag?

@anfredette
Copy link
Contributor

anfredette commented Aug 28, 2024

I was thinking of something like the following:

By default, make run-on-kind

  • builds and uses local images for bpfman-operator and bpfman-agent
  • uses the remote version of quay.io/bpfman/bpfman:latest

BPFMAN_LOCAL=1 make run-on-kind

  • uses the local version of whatever BPFMAN_IMG is set to
  • by default, this is the local version of quay.io/bpfman/bpfman:latest

BPFMAN_REMOTE=1 make run-on-kind

  • use the remote versions of everything
  • This allows someone to quickly spin up a cluster w/bpfman without building anything for kicking the tires or testing.

UPDATE: I added issue #118 for these suggestions to be considered separately.

Copy link
Contributor

@anfredette anfredette left a comment

Choose a reason for hiding this comment

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

Looks good other than the one comment.

Makefile Outdated Show resolved Hide resolved
@anfredette
Copy link
Contributor

anfredette commented Aug 30, 2024

With these changes reverted, does make run-on-kind build and load the local bpfman-operator and bpfman-agent images by default?

FYI, I tested this pr and it does after the suggested change is made to load the bpfman-agent image.

@frobware frobware force-pushed the revert-ensure-run-on-kind-works-without-local-images branch from 3fe1275 to 8650f28 Compare August 31, 2024 06:27
@frobware
Copy link
Contributor Author

UPDATE: I added issue #118 for these suggestions to be considered separately.

Noted. I dropped my [testing] commit.

Also, can you spell out the steps needed for someone to use a locally built bpfman image as opposed to a remote image with the same name and tag?

I updated the PR description, though perhaps the cited example should go in our docs.

@anfredette
Copy link
Contributor

I updated the PR description, though perhaps the cited example should go in our docs.

I'm going to approve this pr as is, but please add that info to the docs. Somewhere in the following area is probably a good place:

https://github.com/bpfman/bpfman/blob/9ae469f2073a45ba342af8a28220630b3672932d/docs/developer-guide/develop-operator.md#L252

Copy link
Contributor

@anfredette anfredette left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit eeb144f into bpfman:main Sep 3, 2024
20 checks passed
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