Skip to content

Add the support for redis in cAdvisor's storage#693

Closed
superzhaoyy wants to merge 54 commits intogoogle:masterfrom
superzhaoyy:master
Closed

Add the support for redis in cAdvisor's storage#693
superzhaoyy wants to merge 54 commits intogoogle:masterfrom
superzhaoyy:master

Conversation

@superzhaoyy
Copy link
Copy Markdown
Contributor

provide redis driver for cAdvisor's storage,we can push the data from cAdvisor to redis.

@googlebot
Copy link
Copy Markdown
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@cadvisorJenkinsBot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@superzhaoyy
Copy link
Copy Markdown
Contributor Author

I signed it!

@googlebot
Copy link
Copy Markdown
Collaborator

CLAs look good, thanks!

Comment thread storagedriver.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hostname() won't return what you expect if cAdvisor is running inside a container. You probably want to accept an IP or hostname as a flag.

@vmarmol
Copy link
Copy Markdown
Contributor

vmarmol commented May 7, 2015

ok to test

@vmarmol
Copy link
Copy Markdown
Contributor

vmarmol commented May 7, 2015

this looks great @superzhaoyy, thanks for the PR!

@vmarmol
Copy link
Copy Markdown
Contributor

vmarmol commented May 7, 2015

Looks like we'll need to add the redis godep btw.

@superzhaoyy
Copy link
Copy Markdown
Contributor Author

hi @vmarmol why I can't add the redis godep on Godeps?
when I run [ go get github.com/garyburd/redigo/redis] [ godep save ] ,I haven't changed anything in Godeps/Godeps.json and Godeps/_workspace

when I run [ go get github.com/garyburd/redigo/redis] [ godep save ]
result is :
godep: github.com/docker/docker/pkg/symlink: revision is af9dac9627e7b04a803df152cc24f0db073ea17a, want 8e107a93210c54f22ec1354d969c771b1abfbe058e107a93210c54f22ec1354d969c771b1abfbe05

@vmarmol
Copy link
Copy Markdown
Contributor

vmarmol commented May 8, 2015

Usually what works for me is this (from cAdvisor root):

$ godep restore
$ go get -u github.com/garyburd/redigo/redis
$ godep save ./...

Does that return any problems?

@superzhaoyy
Copy link
Copy Markdown
Contributor Author

thanks@vmarmol
As you said, I successfully added redis in Godeps.

Comment thread Godeps/Godeps.json Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you run with go1.4 or just change the version back in this file manually?

@rjnagal
Copy link
Copy Markdown
Contributor

rjnagal commented May 12, 2015

Looks great, just minor nits.

Can you also squash your commits. Thanks @superzhaoyy !

@superzhaoyy
Copy link
Copy Markdown
Contributor Author

hi@rjnagal
I have modified some mistakes that you had pointed out,but how can i squash my commits
thank you.

vmarmol and others added 24 commits May 12, 2015 21:48
This gives us a bit over one event per second for one day. Assuming 250
bytes per event, the max memory usage is ~24MiB per event type.
This will allow us to register and run custom collectors for each
container.
This will allow cAdvisor to be reverse proxied.

Fixes #513
Fixes #566
Fixes #596
Fixes #667
Added missing docker page handling.
The current logic assumes that entries are added to the store in
monotonically increasing order for time. This is not true when
we add creation events for existing containers.
Containers with subcontainers always report creation time to be same as the time
of creation of the latest subcontainer.

Still not an ideal solution, but accurate for most practical purposes.
Everything other than cache information is available through /proc/cpuinfo.
1) cgroups were getting mounted at /cgroup: not /cgroup
2) incorrect flag was used:

provided but not defined: --privilege
See 'docker run --help'.
Add the support of redis in cadvisor's storage

Add redis for cadvisor's storage

Add the support for redis in the cAdvisor's storage

Update storagedriver.go
@superzhaoyy
Copy link
Copy Markdown
Contributor Author

hi @vmarmol
oh ,it's sad .I add more commits because i make mistakes by using git .
This article http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html
says "A word of caution: Only do this on commits that haven’t been pushed an external repository"
but I have pushed commits to remote master, so can't I make my commits into less commits ?
can i give up this PR, then I give another PR ?

@vmarmol
Copy link
Copy Markdown
Contributor

vmarmol commented May 13, 2015

That works too :)

It looks like you got a rebase slightly incorrect. Git is a bit of a pain to get used to for stuff like this...

@superzhaoyy
Copy link
Copy Markdown
Contributor Author

@vmarmol
yes.it is sad .I am not able to skillfully use Git. I have to delete my repository,i refork google/cadvisor.
can i give up this PR, then I give another PR ?
Thank you for your patience to answer.
👍

@vmarmol
Copy link
Copy Markdown
Contributor

vmarmol commented May 13, 2015

Yes you certainly can close this PR and make a new one. You shouldn't need to delete your repository, just make a new branch:

Assuming that origin is the upstream fork in GitHub.

# Grab the latest code
$ git fetch origin master

# Make a new branch based on the current code
$ git branch new-branch origin/master

# Switch to that branch
$ git checkout new-branch

@superzhaoyy
Copy link
Copy Markdown
Contributor Author

thanks@vmarmol
I have make a new PR. Please close this PR.
It's nice talking with you.

@vmarmol vmarmol closed this May 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants