Run jaeger-es-index-cleaner and jaeger-es-rollover locally #5714

hellspawn679 · 2024-07-07T11:31:45Z

Which problem is this PR solving?

Use current code, not published images, in ES integration tests #5709

Description of the changes

added a function build_local_img

How was this change tested?

Checklist

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

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

Signed-off-by: mehul gautam <mehulsharma4786@gmail.com>

codecov · 2024-07-07T11:41:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.87%. Comparing base (13c715b) to head (85d373c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5714      +/-   ##
==========================================
- Coverage   96.89%   96.87%   -0.02%     
==========================================
  Files         334      334              
  Lines       16146    16146              
==========================================
- Hits        15644    15642       -2     
- Misses        333      334       +1     
- Partials      169      170       +1

Flag	Coverage Δ
badger_v1	`8.06% <ø> (ø)`
badger_v2	`1.90% <ø> (ø)`
cassandra-3.x-v1	`16.62% <ø> (ø)`
cassandra-3.x-v2	`1.82% <ø> (ø)`
cassandra-4.x-v1	`16.62% <ø> (ø)`
cassandra-4.x-v2	`1.82% <ø> (ø)`
elasticsearch-6.x-v1	`18.80% <ø> (+0.01%)`	⬆️
elasticsearch-7.x-v1	`18.86% <ø> (ø)`
elasticsearch-8.x-v1	`19.04% <ø> (-0.02%)`	⬇️
elasticsearch-8.x-v2	`1.89% <ø> (ø)`
grpc_v1	`9.46% <ø> (ø)`
grpc_v2	`7.39% <ø> (-0.02%)`	⬇️
kafka	`9.75% <ø> (ø)`
opensearch-1.x-v1	`18.90% <ø> (ø)`
opensearch-2.x-v1	`18.90% <ø> (ø)`
opensearch-2.x-v2	`1.90% <ø> (ø)`
unittests	`95.26% <ø> (-0.02%)`	⬇️

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.

hellspawn679 · 2024-07-07T11:42:14Z

@yurishkuro have a look if this implementation looks good enough then after i will make similar changes for jaeger-es-rollover

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

yurishkuro · 2024-07-07T15:57:44Z

scripts/es-integration-test.sh

+    # Loop through each platform (separated by commas)
+    for platform in $(echo "$platforms" | tr ',' ' '); do
+      # Extract the architecture from the platform string
+      arch=${platform##*/}  # Remove everything before the last slash
+      make "build-binaries-$arch"
+    done


Suggested change

# Loop through each platform (separated by commas)

for platform in $(echo "$platforms" | tr ',' ' '); do

# Extract the architecture from the platform string

arch=${platform##*/} # Remove everything before the last slash

make "build-binaries-$arch"

done

make "build-binaries-linux"

Is that what you are trying to do?

yes but i let i be there since i wasn't sure if we have to test only on linux/amd64 i will make the changes

yurishkuro · 2024-07-07T15:59:20Z

scripts/es-integration-test.sh

+      make "build-binaries-$arch"
+    done
+    make create-baseimg
+    #bash scripts/build-upload-a-docker-image.sh ${LOCAL_FLAG} -b -c jaeger-es-index-cleaner -d cmd/es-index-cleaner -p "${platforms}" -t release -l Y


Why can't we use this?

well i tried it gives me a log message of Unable to find image
'localhost:5000/jaegertracing/jaeger-es-index-cleaner:local-test' locally
although it is able to pull the image locally and run fine but i wanted to ask you first about it

yurishkuro · 2024-07-07T15:59:33Z

plugin/storage/integration/es_index_cleaner_test.go

@@ -32,7 +32,7 @@ const (
 	samplingIndexName     = "jaeger-sampling-2019-01-01"
 	spanIndexName         = "jaeger-span-2019-01-01"
 	serviceIndexName      = "jaeger-service-2019-01-01"
-	indexCleanerImage     = "jaegertracing/jaeger-es-index-cleaner:latest"
+	indexCleanerImage     = "jaegertracing/jaeger-es-index-cleaner:local-test"
 	rolloverImage         = "jaegertracing/jaeger-es-rollover:1.57.0"


The jaeger-es-index-cleaner is now pulling the local image, and there are no longer any log messages for this process.

Rollover image is also pulled by published version instead of being built locally

#5714 (comment)
so is the current implementation for es-index-cleaner is good enough

i will make appropriate changes

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

yurishkuro · 2024-07-07T20:59:32Z

why is pkg/version/build_test.go deleted in this PR?

yurishkuro · 2024-07-07T21:05:14Z

scripts/es-integration-test.sh

@@ -96,14 +96,32 @@ teardown_storage() {
  docker compose -f "${compose_file}" down
 }

+build_local_img(){
+    make "build-binaries-linux"


you don't need to build ALL binaries, only the 2 specific ones

I think you want

make build-es-index-cleaner GOOS=linux make build-es-rollover GOOS=linux

yurishkuro · 2024-07-07T23:47:34Z

scripts/es-integration-test.sh

@@ -96,14 +96,32 @@ teardown_storage() {
  docker compose -f "${compose_file}" down
 }

+build_local_img(){
+    make "build-binaries-linux"
+    make create-baseimg


Suggested change

make create-baseimg

make create-baseimg PLATFORMS=linux/$(go env GOARCH)

yurishkuro · 2024-07-08T00:05:45Z

scripts/es-integration-test.sh

+    make "build-binaries-linux"
+    make create-baseimg
+    #build es-index-cleaner
+    docker build \


This worked for me:

Build with a fake GITHUB_SHA/BRANCH=local-test variables

GITHUB_SHA=local-test BRANCH=local-test bash scripts/build-upload-a-docker-image.sh -l -b -c jaeger-es-index-cleaner -d cmd/es-index-cleaner -t release -p linux/$(go env GOARCH)

Run as local-test from local registry localhost:5000

docker run localhost:5000/jaegertracing/jaeger-es-index-cleaner:local-test

yurishkuro · 2024-07-08T00:06:50Z

plugin/storage/integration/es_index_cleaner_test.go

+	indexCleanerImage     = "jaegertracing/jaeger-es-index-cleaner:local-test"
+	rolloverImage         = "jaegertracing/jaeger-es-rollover:local-test"


Suggested change

indexCleanerImage = "jaegertracing/jaeger-es-index-cleaner:local-test"

rolloverImage = "jaegertracing/jaeger-es-rollover:local-test"

indexCleanerImage = "localhost:5000/jaegertracing/jaeger-es-index-cleaner:local-test"

rolloverImage = "localhost:5000/jaegertracing/jaeger-es-rollover:local-test"

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

yurishkuro

Thanks!

mehul gautam added 4 commits July 7, 2024 09:30

run locally

8420368

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

fix

808d331

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

testing

ec5ae1b

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

fixed

8f5f4a2

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

hellspawn679 requested a review from a team as a code owner July 7, 2024 11:31

hellspawn679 requested a review from jkowall July 7, 2024 11:31

dosubot bot added area/storage changelog:new-feature Change that should be called out as new feature in CHANGELOG storage/elasticsearch labels Jul 7, 2024

Merge branch 'main' into node

45730e3

Signed-off-by: mehul gautam <mehulsharma4786@gmail.com>

hellspawn679 changed the title ~~Node~~ Run jaeger-es-index-cleaner and jaeger-es-rollover locally Jul 7, 2024

mehul gautam added 7 commits July 7, 2024 12:48

cleaning up

289b0ab

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

cleaning up

c51a745

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

testing

ebd8dbe

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

back

5b9dda7

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

fixed

57dd378

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

removed extra files

3f6940f

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

fixed

aeb00f7

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

yurishkuro reviewed Jul 7, 2024

View reviewed changes

mehul gautam added 2 commits July 7, 2024 16:10

fixed

913f952

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

fixed

387dd49

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

hellspawn679 requested a review from yurishkuro July 7, 2024 16:53

yurishkuro reviewed Jul 7, 2024

View reviewed changes

yurishkuro reviewed Jul 8, 2024

View reviewed changes

testing

c918928

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

mehul gautam and others added 3 commits July 8, 2024 03:46

testing

b475796

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

Merge branch 'main' into node

46c7928

fixed

85d373c

Signed-off-by: mehul gautam <mehulsharma4786@gamil.com>

hellspawn679 requested a review from yurishkuro July 8, 2024 04:29

yurishkuro approved these changes Jul 8, 2024

View reviewed changes

yurishkuro merged commit ea2ec18 into jaegertracing:main Jul 8, 2024
42 checks passed

yurishkuro mentioned this pull request Jul 8, 2024

Use local images for ES integration tests #5715

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run jaeger-es-index-cleaner and jaeger-es-rollover locally #5714

Run jaeger-es-index-cleaner and jaeger-es-rollover locally #5714

hellspawn679 commented Jul 7, 2024

codecov bot commented Jul 7, 2024 •

edited

Loading

hellspawn679 commented Jul 7, 2024

yurishkuro Jul 7, 2024

hellspawn679 Jul 7, 2024

yurishkuro Jul 7, 2024

hellspawn679 Jul 7, 2024

yurishkuro Jul 7, 2024

hellspawn679 Jul 7, 2024

yurishkuro Jul 7, 2024

hellspawn679 Jul 7, 2024

hellspawn679 Jul 7, 2024

yurishkuro commented Jul 7, 2024

yurishkuro Jul 7, 2024

yurishkuro Jul 7, 2024

yurishkuro Jul 7, 2024 •

edited

Loading

yurishkuro Jul 8, 2024 •

edited

Loading

yurishkuro Jul 8, 2024

yurishkuro left a comment

	make create-baseimg
	make create-baseimg PLATFORMS=linux/$(go env GOARCH)

		indexCleanerImage = "jaegertracing/jaeger-es-index-cleaner:local-test"
		rolloverImage = "jaegertracing/jaeger-es-rollover:local-test"

Run jaeger-es-index-cleaner and jaeger-es-rollover locally #5714

Run jaeger-es-index-cleaner and jaeger-es-rollover locally #5714

Conversation

hellspawn679 commented Jul 7, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Jul 7, 2024 • edited Loading

Codecov Report

hellspawn679 commented Jul 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jul 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

yurishkuro Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 7, 2024 •

edited

Loading

yurishkuro Jul 7, 2024 •

edited

Loading

yurishkuro Jul 8, 2024 •

edited

Loading