Skip to content

Conversation

yeikel
Copy link
Collaborator

@yeikel yeikel commented Dec 12, 2024

What changes did you make? (Give an overview)

The existing logic was unnecessary because the confluentinc/cp-kafka image is a multi-platform image and the docker engine will pick the appropriate version as needed

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

A picture of a cute animal (not mandatory but encouraged)

1672588750

@yeikel yeikel requested a review from a team as a code owner December 12, 2024 01:06
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels Dec 12, 2024
@yeikel yeikel changed the title refactor: remove unnecessary architecture check in unit test BE: Chore: remove unnecessary architecture check in unit test Dec 12, 2024
@yeikel yeikel force-pushed the refactor/remove-logic branch from 9638ce4 to a92c50d Compare December 12, 2024 01:12
@Haarolean
Copy link
Member

This didn't work previously for some reason, have you tried running it on arm?

@yeikel yeikel requested a review from a team as a code owner December 12, 2024 15:19
@yeikel yeikel force-pushed the refactor/remove-logic branch from d6b3689 to a92c50d Compare December 12, 2024 15:21
@Haarolean
Copy link
Member

This doesn't work, running tests brings up non-arm 7.2.1:

$  docker ps -a | grep cp-kafka:
766e0ae73874   confluentinc/cp-kafka:7.2.1                   "sh -c 'while [ ! -f…"    20 seconds ago   Up 20 seconds               9092/tcp, 0.0.0.0:32884->2181/tcp, [::]:32884->2181/tcp, 0.0.0.0:32885->9093/tcp, [::]:32885->9093/tcp   adoring_driscoll
49967d09abed   confluentinc/cp-kafka:7.6.0.arm64             "bash -c 'echo Waiti…"    6 hours ago      Exited (0) 6 hours ago                                                                                                               kafbat-ui-dev-kafka-init-topics-1
01363d50167f   confluentinc/cp-kafka:7.6.0.arm64             "/etc/confluent/dock…"    6 hours ago      Up 6 hours                  0.0.0.0:9092->9092/tcp, :::9092->9092/tcp, 0.0.0.0:9997->9997/tcp, :::9997->9997/tcp                     kafka0
d13dfd350410   confluentinc/cp-kafka:7.5.2                   "sh -c '#!/bin/bash\n…"   7 weeks ago      Exited (255) 7 weeks ago    9092/tcp, 0.0.0.0:32825->2181/tcp, [::]:32825->2181/tcp, 0.0.0.0:32826->9093/tcp, [::]:32826->9093/tcp   quirky_borg

@yeikel yeikel marked this pull request as draft December 13, 2024 05:10
@yeikel yeikel force-pushed the refactor/remove-logic branch from a92c50d to dc79429 Compare December 16, 2024 01:58
@yeikel
Copy link
Collaborator Author

yeikel commented Dec 23, 2024

@Haarolean I am surprised that this happened because I had success in another M1 Mac with this. When I tested this I used the multi-platform hash instead of a tag and I am not sure if that matters

Could you please share the output of

docker pull docker.io/confluentinc/cp-kafka:7.8.0
docker inspect docker.io/confluentinc/cp-kafka:7.8.0 | grep "Architecture"

After that, I am curious what you see if you run using the Multi-platform digest instead:

docker pull docker.io/confluentinc/cp-kafka@sha256:adc392d28a1e99e8c9a1ec7f087e9e91041837b35b8b7cc8b8a691b82dd581b0
docker inspect docker.io/confluentinc/cp-kafka@sha256:adc392d28a1e99e8c9a1ec7f087e9e91041837b35b8b7cc8b8a691b82dd581b0 | grep "Architecture"

Also, could please confirm what version of docker are you using?

@Haarolean
Copy link
Member

pull:

$  docker pull confluentinc/cp-kafka:7.8.0
7.8.0: Pulling from confluentinc/cp-kafka
[...]
Digest: sha256:adc392d28a1e99e8c9a1ec7f087e9e91041837b35b8b7cc8b8a691b82dd581b0
Status: Downloaded newer image for confluentinc/cp-kafka:7.8.0
docker.io/confluentinc/cp-kafka:7.8.0

$  docker images | grep cp-kafka | grep 7.8.0
confluentinc/cp-kafka                                                        7.8.0                  4c8c16982ba4   4 weeks ago     1.11GB
$  docker inspect 4c8c16982ba4 | grep "Arch"
        "Architecture": "arm64",

Next I removed all the related images and ran tests:

21:33:49.030 [main] INFO tc.confluentinc/cp-kafka:7.8.0 -- Pulling docker image: confluentinc/cp-kafka:7.8.0. Please be patient; this may take some time but only needs to be done once.

got pulled a proper version:

$  docker images | grep cp-kafka
confluentinc/cp-kafka                                                        7.8.0                  4c8c16982ba4   4 weeks ago     1.11GB
$  docker inspect 4c8c16982ba4 | grep "Arch"
        "Architecture": "arm64",

Not sure what's happening here, the only wild guesses are that there could be a dependency bump for testcontainers or something that caused this to get fixed or the image version bump affected this (previous logs are about 7.2.1).

@yeikel makes sense to merge I guess?

@yeikel yeikel marked this pull request as ready for review January 13, 2025 14:17
@yeikel
Copy link
Collaborator Author

yeikel commented Jan 13, 2025

pull:

$  docker pull confluentinc/cp-kafka:7.8.0
7.8.0: Pulling from confluentinc/cp-kafka
[...]
Digest: sha256:adc392d28a1e99e8c9a1ec7f087e9e91041837b35b8b7cc8b8a691b82dd581b0
Status: Downloaded newer image for confluentinc/cp-kafka:7.8.0
docker.io/confluentinc/cp-kafka:7.8.0

$  docker images | grep cp-kafka | grep 7.8.0
confluentinc/cp-kafka                                                        7.8.0                  4c8c16982ba4   4 weeks ago     1.11GB
$  docker inspect 4c8c16982ba4 | grep "Arch"
        "Architecture": "arm64",

Next I removed all the related images and ran tests:

21:33:49.030 [main] INFO tc.confluentinc/cp-kafka:7.8.0 -- Pulling docker image: confluentinc/cp-kafka:7.8.0. Please be patient; this may take some time but only needs to be done once.

got pulled a proper version:

$  docker images | grep cp-kafka
confluentinc/cp-kafka                                                        7.8.0                  4c8c16982ba4   4 weeks ago     1.11GB
$  docker inspect 4c8c16982ba4 | grep "Arch"
        "Architecture": "arm64",

Not sure what's happening here, the only wild guesses are that there could be a dependency bump for testcontainers or something that caused this to get fixed or the image version bump affected this (previous logs are about 7.2.1).

@yeikel makes sense to merge I guess?

I believe so.

I am not sure what the actual issue was in the past, but I am certain that this works as long as the image is a multi-platform image which in this case it is

@Haarolean Haarolean added scope/backend Related to backend changes type/chore Boring stuff, could be refactoring or tech debt and removed status/triage/manual Manual triage in progress labels Jan 13, 2025
@Haarolean Haarolean added this to the 1.2 milestone Jan 13, 2025
@Haarolean Haarolean merged commit 2b3abd2 into kafbat:main Jan 13, 2025
6 checks passed
@Haarolean
Copy link
Member

@yeikel thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/backend Related to backend changes status/triage/completed Automatic triage completed type/chore Boring stuff, could be refactoring or tech debt
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants