bpfman / bpfman-operator

Kubernetes operator for bpfman
https://bpfman.io
Apache License 2.0
10 stars 12 forks source link

build: BPFMAN_IMG & BPFMAN_AGENT_IMG to overwrite image #18

Closed Billy99 closed 4 months ago

Billy99 commented 4 months ago

The bpfman-operator is setup to allow BPFMAN_IMG to overwrite the default bpfman image, and BPFMAN_AGENT_IMG to overwrite the bpfman-agent image. However, the Makefile is leveraging kustomize. kustomize can replace an image string in a yaml when it knows the k8s object layout, but these images are passed via a ConfigMap, which is opaque data. So the current implementation doesn't work. kustomize does have a ConfigMapGenrator, which can replace the contents of a ConfigMap.

The change is to:

Billy99 commented 4 months ago

To reproduce, run something like:

BPFMAN_IMG=quay.io/billy99/bpfman-test:pr-1150 make run-on-kind

Then use kubectl describe pod -n bpfman bpfman-daemon-xxxxx to view the container image that was loaded.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 26.38%. Comparing base (768a235) to head (aa19fa7). Report is 3 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #18 +/- ## ======================================= Coverage 26.38% 26.38% ======================================= Files 75 75 Lines 5021 5021 ======================================= Hits 1325 1325 Misses 3532 3532 Partials 164 164 ``` | [Flag](https://app.codecov.io/gh/bpfman/bpfman-operator/pull/18/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bpfman) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/bpfman/bpfman-operator/pull/18/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bpfman) | `26.38% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bpfman#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Billy99 commented 4 months ago

NOTE: make run-on-kind is currently not working. As a temp workaround, the BPFMAN_AGENT_IMG and BPFMAN_OPERATOR_IMG in Makefile need a -amd64 appended to them. I didn't fix that because I think it needs a little more discussion on the correct fix.

msherif1234 commented 4 months ago

NOTE: make run-on-kind is currently not working. As a temp workaround, the BPFMAN_AGENT_IMG and BPFMAN_OPERATOR_IMG in Makefile need a -amd64 appended to them. I didn't fix that because I think it needs a little more discussion on the correct fix.

WDYT about the following to fix -amd64 tag issue ?

.PHONY: load-images-kind
 load-images-kind: ## Load bpfman-agent, and bpfman-operator images into the running local kind devel cluster.
-       kind load docker-image ${BPFMAN_OPERATOR_IMG} ${BPFMAN_AGENT_IMG} --name ${KIND_CLUSTER_NAME}
+       kind load docker-image ${BPFMAN_OPERATOR_IMG}-${MULTIARCH_TARGETS} ${BPFMAN_AGENT_IMG}-${MULTIARCH_TARGETS} --name ${KIND_CLUSTER_NAME}
Billy99 commented 4 months ago

NOTE: make run-on-kind is currently not working. As a temp workaround, the BPFMAN_AGENT_IMG and BPFMAN_OPERATOR_IMG in Makefile need a -amd64 appended to them. I didn't fix that because I think it needs a little more discussion on the correct fix.

WDYT about the following to fix -amd64 tag issue ?

.PHONY: load-images-kind
 load-images-kind: ## Load bpfman-agent, and bpfman-operator images into the running local kind devel cluster.
-       kind load docker-image ${BPFMAN_OPERATOR_IMG} ${BPFMAN_AGENT_IMG} --name ${KIND_CLUSTER_NAME}
+       kind load docker-image ${BPFMAN_OPERATOR_IMG}-${MULTIARCH_TARGETS} ${BPFMAN_AGENT_IMG}-${MULTIARCH_TARGETS} --name ${KIND_CLUSTER_NAME}

That's probably fine as long as that's the only place it is needed.

msherif1234 commented 4 months ago

/LGTM