Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Elasticsearch 5.x storage support #1597

Closed
wants to merge 7 commits into from
Closed

Conversation

oopschen
Copy link

Add Elasticsearch 5.x storage support.
Enhance date format for index name resolvition.
Enhance client options.

@k8s-ci-robot
Copy link
Collaborator

Hi @oopschen. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dashpole dashpole self-assigned this Mar 15, 2017
@dashpole
Copy link
Collaborator

@k8s-bot ok to test

@dashpole
Copy link
Collaborator

@timstclair this looks like an entirely new storage driver. Is it not possible just to update the old ES driver? If it is a new one, this is probably blocked on #1458...

@dashpole
Copy link
Collaborator

dashpole commented Apr 4, 2017

@k8s-bot test this

@dashpole
Copy link
Collaborator

dashpole commented Apr 4, 2017

@oopschen apologies for the delay. Is it possible to update the existing ES storage driver rather than adding a new one while maintaining compatibility with older versions? I don't think we should have a driver for each version of each driver if we can avoid it.

@oopschen
Copy link
Author

oopschen commented Apr 6, 2017

@dashpole There is two major release of elasticsearch: 2.x and 5.x, and there are not compatible. I think it is better to support two of them to offer a better user experience for developers.

It definitely possible to merge two major release ES storage, if it is required, should i open a new pull request or merge them in this pull request?

@dashpole
Copy link
Collaborator

@timstclair should this wait on #1458?

@timstclair
Copy link
Contributor

timstclair commented Apr 20, 2017 via email

@dashpole
Copy link
Collaborator

It is a new plugin, since elasticsearch 2.x and 5.x are not compatible

@rikatz
Copy link
Contributor

rikatz commented May 8, 2017

Since the drivers from 2 and 5 aren't compatible, we need to have 2 drivers :(

I've faced this very same issue in Heapster.

There are some prettier solutions here (maybe using an interface, configuring the version in storage URI when starting cAdvisor), but the point here is that we still need 2 drivers :)

Haven't tested this implementation by itself. I've missed an _test file.

About the refactor of cAdvisor storage (as in #1458), can't say something about this (as I'm no expert in developing) but sounds better to me anyway to have plugable storage instead of maintaining each solution market. By the way, would prometheus be a 'storage' implementation, or should we keep this separate as a '/metric' implementation? :)

@jjduhamel
Copy link

Any update on this? Would be good to add support for this asap.

@jindov
Copy link

jindov commented Aug 18, 2017

Waiting for this support, I use ELK 5.5 stack on system

@mostolog
Copy link

Already added +1.

Having this blocked by #1458 for so long doesn't make any sense to me.

Adding this driver as "elasticsearch5" while the other is still mantained as "elasticsearch" would do the trick.

@liangyuanpeng
Copy link

liangyuanpeng commented Feb 12, 2020

any update?

maybe current the es version supported is too old, we should update version of es for stoarge 😢

@bobbypage
Copy link
Collaborator

I agree with the direction in #1458 (comment) we want to avoid adding new storage drivers in cAdvisor and instead rely on prometheus metrics + external backends that can ingest those metrics. As a result, closing this PR for now.

@bobbypage bobbypage closed this Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants