fix: Use the correct opentracing plugin for Jaeger #2744

mikebryant · 2018-07-05T13:03:53Z

What this PR does / why we need it: Uses the correct opentracing plugin for Jaeger - without this there is a regression and that feature doesn't work in 0.16.2

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): part of #2738

Special notes for your reviewer:

mikebryant · 2018-07-05T13:04:11Z

/assign @aledbf

mikebryant · 2018-07-05T13:05:17Z

I've built this locally and was able to run the controller with Jaeger enabled without it crashing. Haven't tested it more extensively than that

aledbf · 2018-07-05T13:13:21Z

Makefile

@@ -59,7 +59,7 @@ IMAGE = $(REGISTRY)/$(IMGNAME)
 MULTI_ARCH_IMG = $(IMAGE)-$(ARCH)

 # Set default base image dynamically for each arch
-BASEIMAGE?=quay.io/kubernetes-ingress-controller/nginx-$(ARCH):0.53
+BASEIMAGE?=quay.io/kubernetes-ingress-controller/nginx-$(ARCH):0.54


Please leave this as 0.53. First, we need to merge the change in the nginx image, publish and then open a PR to update the version (we don't merge PRs failing CI)

(And updated the description, as now this PR won't fix the issue, the next one will)

codecov-io · 2018-07-05T13:45:50Z

Codecov Report

Merging #2744 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master    #2744   +/-   ##
=======================================
  Coverage   40.91%   40.91%           
=======================================
  Files          73       73           
  Lines        5101     5101           
=======================================
  Hits         2087     2087           
  Misses       2725     2725           
  Partials      289      289

Impacted Files	Coverage Δ
internal/ingress/controller/template/template.go	`66.52% <0%> (ø)`	⬆️

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 64cf1b9...0e26cff. Read the comment docs.

aledbf · 2018-07-05T13:56:03Z

images/nginx/build.sh

 cd "$BUILD_PATH/jaeger-client-cpp-$JAEGER_VERSION"
 sed -i 's/-Werror//' CMakeLists.txt
 mkdir .build
 cd .build
-cmake -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=1 -DBUILD_TESTING=OFF -DJAEGERTRACING_WITH_YAML_CPP=OFF -DJAEGERTRACING_BUILD_EXAMPLES=OFF ..
+# Taken from https://github.com/jaegertracing/jaeger-client-cpp/blob/v0.4.1/scripts/build-plugin.sh


@rnburn is this ok? I was using the same steps in your repository https://github.com/opentracing-contrib/nginx-opentracing/blob/master/Dockerfile#L65

That looks good.

Jaeger needs the JAEGERTRACING_WITH_YAML_CPP option enabled to be able to load a JSON configuration, which was probably what was causing trouble.

But BTW - since Jaeger links in everything statically, you should be able to remove the _3rdParty/Hunter/ directory when you're finished. It would make your image smaller.

@mikebryant can you implement what @rnburn mentioned?

@mikebryant last change and we are ready to merge

@aledbf Isn't that already taken care of by the rm -rf "$BUILD_PATH" line later in the script, that cleans up all of the build stuff?

Maybe most of it's taken care of, but you can delete this line

cp $HUNTER_INSTALL_DIR/lib/libthrift* /usr/local/lib

Ah, I see, sure

rnburn · 2018-07-05T18:01:56Z

images/nginx/build.sh

+ local: *;
+};
+EOF
+cmake -DCMAKE_BUILD_TYPE=Release -DJAEGERTRACING_PLUGIN=ON -DBUILD_TESTING=OFF -DJAEGERTRACING_BUILD_EXAMPLES=OFF -DHUNTER_CONFIGURATION_TYPES=Release ..
 make
 make install


Also, you shouldn't need make install here

Thanks, done :)

Part of #2738

…ally linked See #2744 (comment)

aledbf · 2018-07-05T23:20:51Z

/lgtm
/approve

k8s-ci-robot · 2018-07-05T23:20:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, mikebryant

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aledbf · 2018-07-05T23:21:07Z

@mikebryant I'll let you know when the image is available

gianrubio · 2018-07-06T11:52:55Z

I built a image from master and seems like the issue wasn't fixed.

k logs -f -n ingress ingress-nginx-ingress-intern-controller-ff48c5bc-vjhw5
I0706 11:50:32.240482       7 flags.go:180] Watching for Ingress class: intern
W0706 11:50:32.240540       7 flags.go:183] Only Ingresses with class "intern" will be processed by this Ingress controller
W0706 11:50:32.240756       7 flags.go:213] SSL certificate chain completion is disabled (--enable-ssl-chain-completion=false)
-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:    0.16.2-1
  Build:      git-5827c981
  Repository: https://github.com/kubernetes/ingress-nginx
-------------------------------------------------------------------------------
....

-------------------------------------------------------------------------------
Error: exit status 1
2018/07/06 11:50:33 [error] 39#39: Failed to load tracing library /usr/local/lib/libjaegertracing_plugin.so: /usr/local/lib/libjaegertracing_plugin.so: cannot open shared object file: No such file or directory
nginx: [error] Failed to load tracing library /usr/local/lib/libjaegertracing_plugin.so: /usr/local/lib/libjaegertracing_plugin.so: cannot open shared object file: No such file or directory
nginx: configuration file /tmp/nginx-cfg280008842 test failed

-------------------------------------------------------------------------------
W0706 11:50:33.694344       7 queue.go:130] requeuing initial-sync, err
-------------------------------------------------------------------------------

mikebryant · 2018-07-06T12:01:29Z

@gianrubio See #2748

And hmm, oops, it looks like I shouldn't have had the changes to use the new plugin in this MR, they should have been kept back till #2748 since it's not going to work without that change as well..

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 5, 2018

k8s-ci-robot assigned aledbf Jul 5, 2018

mikebryant mentioned this pull request Jul 5, 2018

nginx: [error] Failed to load tracing library /usr/local/lib/libjaegertracing.so #2738

Closed

aledbf reviewed Jul 5, 2018

View reviewed changes

rnburn reviewed Jul 5, 2018

View reviewed changes

mikebryant added 2 commits July 5, 2018 19:09

fix: Use the correct opentracing plugin for Jaeger

85d1742

Part of #2738

refactor: Remove unnecessary libthrift libraries, as Jaeger is static…

0e26cff

…ally linked See #2744 (comment)

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2018

k8s-ci-robot merged commit 88b7d97 into kubernetes:master Jul 5, 2018

mikebryant deleted the wip-2738 branch July 6, 2018 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use the correct opentracing plugin for Jaeger #2744

fix: Use the correct opentracing plugin for Jaeger #2744

mikebryant commented Jul 5, 2018 •

edited

Loading

mikebryant commented Jul 5, 2018

mikebryant commented Jul 5, 2018

aledbf Jul 5, 2018

mikebryant Jul 5, 2018

mikebryant Jul 5, 2018

codecov-io commented Jul 5, 2018 •

edited

Loading

aledbf Jul 5, 2018

rnburn Jul 5, 2018 •

edited

Loading

aledbf Jul 5, 2018

aledbf Jul 5, 2018

mikebryant Jul 5, 2018

rnburn Jul 5, 2018

mikebryant Jul 5, 2018

mikebryant Jul 5, 2018

rnburn Jul 5, 2018 •

edited

Loading

mikebryant Jul 5, 2018

aledbf commented Jul 5, 2018

k8s-ci-robot commented Jul 5, 2018

aledbf commented Jul 5, 2018 •

edited

Loading

gianrubio commented Jul 6, 2018

mikebryant commented Jul 6, 2018

fix: Use the correct opentracing plugin for Jaeger #2744

fix: Use the correct opentracing plugin for Jaeger #2744

Conversation

mikebryant commented Jul 5, 2018 • edited Loading

mikebryant commented Jul 5, 2018

mikebryant commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 5, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

rnburn Jul 5, 2018 • edited Loading

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

rnburn Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Jul 5, 2018

k8s-ci-robot commented Jul 5, 2018

aledbf commented Jul 5, 2018 • edited Loading

gianrubio commented Jul 6, 2018

mikebryant commented Jul 6, 2018

mikebryant commented Jul 5, 2018 •

edited

Loading

codecov-io commented Jul 5, 2018 •

edited

Loading

rnburn Jul 5, 2018 •

edited

Loading

rnburn Jul 5, 2018 •

edited

Loading

aledbf commented Jul 5, 2018 •

edited

Loading