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

cmd: make bucket upload command take lset from flags #7059

Conversation

MichaHoffmann
Copy link
Contributor

@MichaHoffmann MichaHoffmann commented Jan 13, 2024

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

Changes

  • tools bucket upload-blocks should get lset from flags ( which is nicely in-line with ruler and receiver then too )
  • fixed deadlock because of missing actor function on the command
  • is changlog needed here since the original command was not yet released?

Verification

@MichaHoffmann MichaHoffmann force-pushed the mhoffm-pass-extlabels-to-upload-blocks-from-flags branch from 5214faa to 7ce06ee Compare January 13, 2024 17:09
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 13, 2024
@nicolastakashi
Copy link
Contributor

Just a note On docs would be nice show how to pass multiple label values.

Besides that LGTM 🙌

@yeya24
Copy link
Contributor

yeya24 commented Jan 13, 2024

Umm, why we don't keep the previous functionality as well? Taking lset from flag can be an additional flag. But I am fine to merge this change.

@MichaHoffmann
Copy link
Contributor Author

Umm, why we don't keep the previous functionality as well? Taking lset from flag can be an additional flag. But I am fine to merge this change.

I think it might be redundant, if we have prometheus around its arguable that we know its ext labels; we can just read prometheus config and put the labels here

@yeya24
Copy link
Contributor

yeya24 commented Jan 13, 2024

Since @nicolastakashi 's usecase is in Prometheus operator, I think it would be nice to get this value dynamically rather than specifying ext labels manually, considering multi cluster scenario.

But I am fine if he is ok with it

yeya24
yeya24 previously approved these changes Jan 13, 2024
@MichaHoffmann
Copy link
Contributor Author

MichaHoffmann commented Jan 13, 2024

One second, during tests i found there is a deadlock somewhere, ill try to debug and include fix here to!

Edit: found and fixed it.

@MichaHoffmann MichaHoffmann marked this pull request as draft January 13, 2024 17:36
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-pass-extlabels-to-upload-blocks-from-flags branch from 7ce06ee to d693128 Compare January 13, 2024 17:46
@MichaHoffmann MichaHoffmann marked this pull request as ready for review January 13, 2024 17:47
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-pass-extlabels-to-upload-blocks-from-flags branch from d693128 to 43d90f2 Compare January 13, 2024 18:04
Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-pass-extlabels-to-upload-blocks-from-flags branch from 43d90f2 to 0cf52fa Compare January 13, 2024 18:13
@yeya24
Copy link
Contributor

yeya24 commented Jan 13, 2024

What's the fix for deadlock?
It is hard to figure it out from your changes as it is in a single commit.
Let's try to use multiple commits since we always squash at the end.

@MichaHoffmann
Copy link
Contributor Author

What's the fix for deadlock? It is hard to figure it out from your changes as it is in a single commit. Let's try to use multiple commits since we always squash at the end.

No actor on g, it would hang in main on g.Run() when I debugged it in dlv

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks

@yeya24 yeya24 merged commit 2dcfabe into thanos-io:main Jan 14, 2024
20 checks passed
hanyuting8 pushed a commit to hanyuting8/thanos that referenced this pull request Jan 19, 2024
Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
Signed-off-by: hanyuting8 <hytxidian@163.com>
hanyuting8 pushed a commit to hanyuting8/thanos that referenced this pull request Jan 19, 2024
Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
Signed-off-by: hanyuting8 <hytxidian@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants