Use es-index-cleaner golang implementation

[pavolloffay]

Signed-off-by: Pavol Loffay [email protected]

Updates #3179

[codecov]

Codecov Report

Merging #3204 (10284ca) into master (8d2ac60) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3204   +/-   ##
=======================================
  Coverage   95.98%   95.99%           
=======================================
  Files         242      242           
  Lines       14794    14789    -5     
=======================================
- Hits        14200    14196    -4     
  Misses        514      514           
+ Partials       80       79    -1     

Impacted Files	Coverage Δ
cmd/es-index-cleaner/app/flags.go	100.00% <100.00%> (ø)
cmd/es-index-cleaner/app/index_filter.go	100.00% <100.00%> (ø)
...lugin/sampling/strategystore/adaptive/processor.go	99.35% <0.00%> (-0.65%)	⬇️
cmd/query/app/static_handler.go	97.60% <0.00%> (+1.79%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d2ac60...10284ca. Read the comment docs.

[jpkrohling]

LGTM, just want to hear from @Ashmita152 on the Dockerfile changes.

[jpkrohling]

@Ashmita152, are you interested in reviewing this?

[pavolloffay]

The index cleaer integration test is failing bc it cannot build the image via buildx. The buildx uses different CI runner.

CGO_ENABLED=0 installsuffix=cgo go build -trimpath -o ./cmd/es-index-cleaner/es-index-cleaner-linux-amd64 ./cmd/es-index-cleaner/main.go
make[1]: Leaving directory '/home/runner/work/jaeger/jaeger'
docker build --target release -t jaegertracing/jaeger-es-index-cleaner:latest --build-arg base_image=localhost:5000/baseimg_alpine:latest  --build-arg TARGETARCH=amd64 cmd/es-index-cleaner
Sending build context to Docker daemon  10.05MB

Step 1/5 : ARG base_image
Step 2/5 : FROM $base_image AS release
Get http://localhost:5000/v2/: dial tcp [::1]:5000: connect: connection refused
make: *** [Makefile:330: docker-images-elastic] Error 1

[Ashmita152]

@pavolloffay I think you would need to start registry container in elasticsearch integration tests i.e. add this section https://github.com/jaegertracing/jaeger/blob/master/.github/workflows/ci-hotrod.yml#L12-L16 in https://github.com/jaegertracing/jaeger/blob/master/.github/workflows/ci-elasticsearch.yml for fixing the CI failures.

[Ashmita152]

Curious why we are overwriting BASE_IMAGE here ? I would like to keep a single source of truth for all base images and debug images.

[pavolloffay]

I don't like that building the images requires running docker registry at localhost. This complicates local and CI setup.

[Ashmita152]

I think reverting this change back should fix the current CI failure.

Here it is trying to tag the image name as alpine:3.13

make BASE_IMAGE=alpine:3.13 docker-images-elastic
make[1]: Entering directory '/home/runner/work/jaeger/jaeger'
docker buildx build -t alpine:3.13 --push \
	--build-arg root_image=alpine:3.13 \
	--build-arg cert_image=alpine:3.13 \
	--platform=linux/amd64,linux/s390x,linux/ppc64le,linux/arm64 \
	docker/base

which it later tries to push in the official alpine image resulting in

error: failed to solve: authorization status: 401: authorization failed

[pavolloffay]

yes this was the problem. I will try to play with tit and see if I could run it locally.

Can you run the full build locally? I had some issues with running the registry locally.

[albertteoh]

@pavolloffay I had issues with local builds as well in the past; I managed to get it going I think. Maybe this helps? https://github.com/jaegertracing/jaeger/pull/2948/files#r666710826

[pavolloffay]

the CI is all green now

[pavolloffay]

PR rebased and ready for review/merge.

[albertteoh]

I thought --target would be useful if a Dockerfile contained multiple build stages like debug and release. However, from what I see, there is only a release build stage in cmd/es-index-cleaner/Dockerfile. Would it be useful to still include the target param in this case?

[pavolloffay]

True, this can be removed. Done.

[albertteoh]

lgtm