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

query: support store config file #2226

Closed
wants to merge 28 commits into from

Conversation

eswdd
Copy link

@eswdd eswdd commented Mar 6, 2020

Closes #977

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This change adds support for reading stores from an external configuration file, with TLS and file SD configuration variable between sets of stores. The configuration file allows specification of one or more configurations in a similar manner to #1939 and #1982 . Have decided to mandate naming the configurations so that we can label metrics with these names since the primary use case driving this seems to be TLS vs non-TLS and the feeling around our team is that we'd like to see metrics split along these lines.

It is backwards compatible with existing flags for specifying stores, TLS and file SD.

Verification

e2e tests have been updated to use the external store config file.

@bwplotka
Copy link
Member

Nice! Marking this as higher priority, would be nice to have review of this.

Can you mark this un-wip when ready for review?

cc @krasi-georgiev and @s-urbaniak , @simonpasquier for some help in review (:

@eswdd
Copy link
Author

eswdd commented Mar 10, 2020 via email

eswdd and others added 12 commits March 10, 2020 17:30
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Robin Clarke-Williams <43950815+robincw-gr@users.noreply.github.com>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
@eswdd eswdd force-pushed the query_config_file branch 2 times, most recently from 1448466 to 9abd230 Compare March 10, 2020 17:44
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
Signed-off-by: Simon Matic Langford <simon@exemel.co.uk>
@eswdd eswdd marked this pull request as ready for review March 10, 2020 20:14
@eswdd
Copy link
Author

eswdd commented Mar 17, 2020

@s-urbaniak have implemented all the suggestions, PTAL

@s-urbaniak
Copy link
Contributor

For me this is looking great, thank you! 🎉 If there is a last nit, I would love to see some unmarshaling test (especially with the DefaultConfig handling) in pkg/store/config.go.

else this LGTM

Leaving the final pair of eyes to @bwplotka

@bwplotka
Copy link
Member

Sorry for lag, let's wait until release is done... we are working heavily on it, then I will look on it closely, promise (:

Let's rebase, but I think rebase can wait until the merge release-0.12 -> master will happen. It might introduce some conflicts anyway.

Thanks @eswdd for patience!

@eswdd
Copy link
Author

eswdd commented Apr 21, 2020

@bwplotka sorry, was off last week, so only just seen this - would have wanted to get this into the release but I guess too late now. When do you expect to cut the next release?

@simonpasquier
Copy link
Contributor

According to the release planning, the first RC of v0.13.0 is slated for the 13th of May.

@bwplotka
Copy link
Member

Let's get back to this... can you rebase @eswdd ?

@bwplotka
Copy link
Member

bwplotka commented May 19, 2020

It's huge change plus lots of things changes since last rebase, so unlikely to get in 0.13 ):

@amrmahdi
Copy link
Contributor

amrmahdi commented Jul 9, 2020

Thanks @eswdd for this change, very much needed.
@bwplotka we are at 0.14.0 train now, would this make it to 0.14.0 ?

@bwplotka
Copy link
Member

Sorry for huge delay on this. Let's get back to this. We might have more time this week! (: (PromCon was last week).

Do you mind reading?

@stale
Copy link

stale bot commented Sep 19, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Sep 19, 2020
@s-urbaniak
Copy link
Contributor

@bwplotka i think this is still useful, PTAL :)

@stale stale bot removed the stale label Sep 22, 2020
@bwplotka
Copy link
Member

bwplotka commented Nov 3, 2020

We need rebase 🤔

@stale
Copy link

stale bot commented Jan 2, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jan 2, 2021
@markmsmith
Copy link

FWIW, I'm still interested in this. Hopefully the original author can check in before the stale bot kills it.

@stale stale bot removed the stale label Jan 4, 2021
Base automatically changed from master to main February 26, 2021 16:30
@stale
Copy link

stale bot commented Jun 2, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jun 2, 2021
@markmsmith
Copy link

Is this still blocked on a reabase from @eswdd ?

@stale
Copy link

stale bot commented Aug 3, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Aug 3, 2021
@stale stale bot closed this Aug 10, 2021
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.

[Feature] Per-Store TLS configuration in Thanos Query
7 participants