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

[ci] Make Kafka integration test more self-siufficient #4982

Closed
yurishkuro opened this issue Dec 1, 2023 · 3 comments · Fixed by #4989
Closed

[ci] Make Kafka integration test more self-siufficient #4982

yurishkuro opened this issue Dec 1, 2023 · 3 comments · Fixed by #4989
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

The test script scripts/kafka-integration-test.sh is currently very simple, it could use some improvements:

  • Instead of starting Kafka in the CI it could be started from the script when an extra -k parameter is passed. This makes it easier to run the test locally. Example of starting Kafka manually in [test]: make Kafka consumer read from the earliest messages #4981
  • The CI workflow .github/workflows/ci-kafka.yml starts both Kafka and ZK. Is ZK actually needed with the latest versions of Kafka?
  • The script has an infinite while true loop waiting for Kafka to start - if should instead use a loop counter and be time bound, say no more than 3min, with checks every 5sec or so
@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Dec 1, 2023
@RipulHandoo
Copy link
Contributor

RipulHandoo commented Dec 2, 2023

Hey @yurishkuro, I've made some improvements to the Kafka integration test script. Now, you can start Kafka from within the script using the -k parameter, making it easier to run tests locally. Here's the updated script:


#!/bin/bash

set -e

export STORAGE=kafka

# Function to start Kafka
start_kafka() {
    echo "Starting Kafka..."
    
    docker run --name kafka \
    --network jaeger \
    -p 9092:9092 \
    -e KAFKA_CFG_NODE_ID=0 \
    -e KAFKA_CFG_PROCESS_ROLES=controller,broker \
    -e KAFKA_CFG_CONTROLLER_QUORUM_VOTERS=0@kafka:9093 \
    -e KAFKA_CFG_LISTENERS=PLAINTEXT://:9092,CONTROLLER://:9093 \
    -e KAFKA_CFG_ADVERTISED_LISTENERS=PLAINTEXT://localhost:9092 \
    -e KAFKA_CFG_LISTENER_SECURITY_PROTOCOL_MAP=CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT \
    -e KAFKA_CFG_CONTROLLER_LISTENER_NAMES=CONTROLLER \
    -e KAFKA_CFG_INTER_BROKER_LISTENER_NAME=PLAINTEXT \
    bitnami/kafka:3.6
}

# Check if the -k parameter is provided
if [ "$1" == "-k" ]; then
    start_kafka
fi

# Set the timeout in seconds
timeout=180
# Set the interval between checks in seconds
interval=5

# Calculate the end time
end_time=$((SECONDS + timeout))

while [ $SECONDS -lt $end_time ]; do
    if nc -z localhost 9092; then
        break
    fi
    sleep $interval
done

# Check if Kafka is still not available after the timeout
if ! nc -z localhost 9092; then
    echo "Timed out waiting for Kafka to start"
    exit 1
fi

make storage-integration-test

Changes in ci_kafka.yml

- name: Run kafka integration tests
  run: bash scripts/kafka-integration-test.sh -k

Let me know if any further adjustments are needed. Thanks!

@yurishkuro
Copy link
Member Author

Looks good, please create a pull request

@RipulHandoo
Copy link
Contributor

@yurishkuro I've raised a PR.
PTAL.

yurishkuro pushed a commit that referenced this issue Dec 4, 2023
## Which problem is this PR solving?
Resolves #4982 

## Description of the changes
- Introduced a -k parameter to the kafka-integration-test.sh script.
- Modified the script to wait for Kafka availability before proceeding.

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: Ripul Handoo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
2 participants