Skip to content
This repository has been archived by the owner on Dec 1, 2018. It is now read-only.

Update influxdb and grafana dependencies #1993

Merged
merged 5 commits into from
Jul 3, 2018
Merged

Update influxdb and grafana dependencies #1993

merged 5 commits into from
Jul 3, 2018

Conversation

007
Copy link
Contributor

@007 007 commented Mar 27, 2018

Update the builds for influxdb and grafana to use recent versions.

Can't get grafana to cross-compile properly with 5.x because of warnings like this against go-sqlite3 even when compiling statically:

/tmp.k8s/go-link-464062632/000002.o: In function `unixDlOpen':
/go/src/github.com/grafana/grafana/vendor/github.com/mattn/go-sqlite3/sqlite3-binding.c:36542: warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

I don't have a good way to test, but if that's not a showstopper (as suggested elsewhere) then It would be nice to get grafana bumped to 5.0.3 as well. It might be as easy as changing FROM busybox to FROM busybox:glibc.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 27, 2018
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 27, 2018
@007
Copy link
Contributor Author

007 commented Mar 27, 2018

Supporting docs for moving to busybox:glibc and bumping to 5.0.3:
Building on kube-cross -> FROM golang:1.9.3
golang:1.9.3 -> FROM buildpack-deps:stretch-scm
buildpack-deps:stretch-scm -> FROM buildpack-deps:stretch-curl
buildpack-deps:stretch-curl -> FROM debian:stretch

libc on stretch is glibc.

busybox shows its default is uclibc, but does offer glibc and is even based on debian:stretch-slim.

@007
Copy link
Contributor Author

007 commented Mar 28, 2018

Single datapoint: this launches and runs (with grafana 5.0.3) on docker 18.03.0-ce on OSX with either libc - the default busybox:uclibc version or busybox:glibc. I don't know how to exercise it further than that, or how to convince it to load the linked libraries and try to execute code with them.

@Nodraak
Copy link

Nodraak commented Apr 18, 2018

Hello :)

What's the status on this PR? Do you need help?

BTW, Grafana v5.0.4 and influxdb v1.5.2 are out

@007
Copy link
Contributor Author

007 commented Apr 18, 2018

I can update tomorrow, but I'm stuck on how to test the "static" linking in sqlite. It's easy to repro the warnings and resulting image, just switch to 5.0.4 and docker build.

@Nodraak
Copy link

Nodraak commented Apr 18, 2018

The "dlopen in static linked app" issue did not get many answers in the go-sqlite3 repo ...


I don't know golang but the issue might be related to golang/go#21421 (comment) and confluentinc/confluent-kafka-go#59 (comment) ...

TL;DR: try adding the flag -static-all (https://github.com/007/heapster/blob/3cce3e41b8484791a70e0c60b8c9269d81024ba4/grafana/Makefile#L28). I'll try that later if I find the time

@andyxning
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 18, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 18, 2018
@007
Copy link
Contributor Author

007 commented Apr 18, 2018

@Nodraak Changing -static to -static-all doesn't work, and using the -tags static_all also seems iffy since it's throwing the same warnings. confluentinc/confluent-kafka-go#59 (comment) suggests that it's unstable / unsafe, and suggests the same solution as I indicated previously: it's a Docker image, so pack the appropriate dependencies (i.e. glibc) with it.

I pushed the changes to use the busybox:glibc version, we'll see how unit + e2e works from there.

Per k8s-ci-robot suggestion:
/assign @loburm

@Nodraak
Copy link

Nodraak commented Apr 20, 2018

So, I've cloned your repo and checkout-ed your branch. I have ran make build in both grafana and influx folders, and I've updated by k8s test cluster with the newly created images staging-k8s.gcr.io/heapster-grafana-amd64:v5.0.4 and staging-k8s.gcr.io/heapster-influxdb-amd64:v1.5.2.

Everything is running fine without errors.

I'm not sure what you meant by I'm stuck on how to test the "static" linking in sqlite, it looks good on my side!

@007
Copy link
Contributor Author

007 commented Apr 20, 2018

Thanks for another good data point!

I say "stuck" in that I don't know EXACTLY where the sqlite code is used, or how I can prove the negative - I'd like to see a +/- where libc explodes with the wrong one and works with the right one, but I couldn't get that to happen with a different busybox libc in my limited use.

@007
Copy link
Contributor Author

007 commented May 9, 2018

@andyxning @loburm any suggestions on how to test this to satisfy lgtm and get it merged?

Unit and e2e both seem to be working, but it's unclear if they use the built images for the dependencies or if they're pulling whatever base they already have.

@andyxning
Copy link
Contributor

andyxning commented May 10, 2018

Will take a look at this later today.

@Nodraak
Copy link

Nodraak commented May 17, 2018

ping @andyxning

@andyxning
Copy link
Contributor

Sorry, later today. @Nodraak

@andyxning
Copy link
Contributor

/lgtm

@007 @Nodraak Sorry for the late.

@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 3, 2018
@andyxning
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 007, andyxning

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:

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

@k8s-ci-robot k8s-ci-robot merged commit f2199dc into kubernetes-retired:master Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. sink/influxdb size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants