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.

Sign up for GitHub

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

Jump to bottom

Sidecar/Ruler/Receiver: Add flag to disable upload of debug meta files #3852

Closed
wants to merge 7 commits into from
Closed

Sidecar/Ruler/Receiver: Add flag to disable upload of debug meta files #3852

wants to merge 7 commits into from

Conversation

hitanshu-mehta
Copy link
Contributor

@hitanshu-mehta hitanshu-mehta commented Mar 1, 2021 •
edited
Loading

Signed-off-by: Hitanshu Mehta hitanshu99amehta@gmail.com

Fixes: #3839

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

Changes

  • Introduced new flag --shipper.upload-debug-meta-files in sidecar, ruler and receiver.
  • Added test to verify working of flag.

Verification

  • Ran unit and e2e tests locally.

bwplotka
bwplotka approved these changes Mar 1, 2021
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice. Let's decide if we want to remove completely or enable optionally.

pkg/block/block.go Show resolved Hide resolved
@bwplotka
Copy link
Member

bwplotka commented Mar 1, 2021

Can we move this under some flag for now? 🤗

@hitanshu-mehta
Copy link
Contributor Author

Sure. Flag named --shipper.upload-debug-meta-files in sidecar seems correct?

@yeya24
Copy link
Contributor

yeya24 commented Mar 2, 2021

Sure. Flag named --shipper.upload-debug-meta-files in sidecar seems correct?

Receiver and Ruler support uploading blocks as well.

@hitanshu-mehta
Copy link
Contributor Author

Thank you. I will add flags in them too.

@hitanshu-mehta hitanshu-mehta force-pushed the stop-uploading-debug-metas branch 2 times, most recently from 3967226 to 6e6c0b3 Compare March 3, 2021 04:18
@hitanshu-mehta hitanshu-mehta changed the title Stop uploading debug meta files to obj store Sidecar/Ruler/Receiver: Add flag disable upload of debug meta files Mar 3, 2021
@hitanshu-mehta hitanshu-mehta changed the title Sidecar/Ruler/Receiver: Add flag disable upload of debug meta files Sidecar/Ruler/Receiver: Add flag to disable upload of debug meta files Mar 3, 2021
bwplotka
bwplotka reviewed Mar 4, 2021
View reviewed changes
cmd/thanos/downsample.go Outdated Show resolved Hide resolved
bwplotka
bwplotka reviewed Mar 4, 2021
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice, some suggestion and idea for next step (:

pkg/block/block.go Outdated Show resolved Hide resolved
onprem
onprem reviewed Mar 8, 2021
View reviewed changes
cmd/thanos/tools_bucket.go Outdated Show resolved Hide resolved
pkg/block/block_test.go Outdated Show resolved Hide resolved
pkg/verifier/index_issue.go Outdated Show resolved Hide resolved
pkg/verifier/safe_delete.go Outdated Show resolved Hide resolved
cmd/thanos/config.go
@@ -133,6 +134,8 @@ func (sc *shipperConfig) registerFlag(cmd extkingpin.FlagClause) *shipperConfig
cmd.Flag("shipper.upload-compacted",
"If true shipper will try to upload compacted blocks as well. Useful for migration purposes. Works only if compaction is disabled on Prometheus. Do it once and then disable the flag when done.").
Default("false").BoolVar(&sc.uploadCompacted)
cmd.Flag("shipper.upload-debug-meta-files", "If true shipper will upload debug meta files which can be useful for debugging.").
Default("false").BoolVar(&sc.uploadDebubgMetaFiles)
Copy link
Member

@onprem onprem Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We are using the value true by default in a lot of places (tests etc) so why the default is "false"? IMO it makes more sense to have the default as "true" as it will maintain backward compatibility as well.

Looks like we decided to have this opt-in by default (#3852 (comment)). Then maybe let's add an entry in the CHANGELOG.

@hitanshu-mehta
Copy link
Contributor Author

PR is ready for review 🤗

bwplotka
bwplotka previously approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Looks good! Just rebase. I hope we can improve the Upload someday with optional attributes./parameters.

@bwplotka bwplotka enabled auto-merge (squash) April 9, 2021 14:14
@hitanshu-mehta
Copy link
Contributor Author

Thank you for the review :) I will create a follow-up PR for uploader struct.

@bwplotka bwplotka disabled auto-merge April 11, 2021 08:36
@bwplotka
Copy link
Member

We can see that we touch so many things...

Given you need to rebase any way, maybe it's worth creating uploader struct... in this PR? (:

@hitanshu-mehta
Copy link
Contributor Author

Yes, I agree. To rebase, I have to resolve many merge conflicts.

@pstibrany
Copy link
Contributor

If keeping support for use in Cortex was the only reason to keep the flag, I would suggest to simplify the PR, drop the flag and remove support for those debug meta files completely.

We can already add support for debug/meta.json file on our own easily, if we really want to. (eg. by having a special bucket that uploads meta.json to two places).

@kakkoyun
Copy link
Member

Hey @hitanshu-mehta. We're going to cut a release candidate soon, is there any chance for you to push this to the finish line?

@hitanshu-mehta
Copy link
Contributor Author

Yes, I will be able to do it. I was waiting for the decision on whether we going to completely remove support for debug meta files or not 😅

@hitanshu-mehta
Fix test
9bf2c68 
Signed-off-by: Hitanshu Mehta <hitanshu99amehta@gmail.com>

Fix merge conflict of tests
@hitanshu-mehta
Scope err variable
a3c001a 
Signed-off-by: Hitanshu Mehta <hitanshu99amehta@gmail.com>
@hitanshu-mehta
Add flag in tools
94c971c 
Signed-off-by: Hitanshu Mehta <hitanshu99amehta@gmail.com>
@hitanshu-mehta
Lint Fixes
1881cc5 
Signed-off-by: Hitanshu Mehta <hitanshu99amehta@gmail.com>
@hitanshu-mehta
Add uploader struct
250f3ca 
Signed-off-by: Hitanshu Mehta <hitanshu99amehta@gmail.com>
@hitanshu-mehta
Fix error
0584176 
Signed-off-by: Hitanshu Mehta <hitanshu99amehta@gmail.com>
@hitanshu-mehta
Generate docs
538204d 
Signed-off-by: Hitanshu Mehta <hitanshu99amehta@gmail.com>
@hitanshu-mehta
Copy link
Contributor Author

I guess, failed e2e test is not related.

@kakkoyun
Copy link
Member

Looks like we decided to have this opt-in by default (#3852 (comment)). Then maybe let's add an entry in the CHANGELOG.

I think the only decision to make is to enable this by default or not? @bwplotka @onprem @pracucci @pstibrany

@stale
Copy link

stale bot commented Jun 20, 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 20, 2021
@stale stale bot closed this Jun 28, 2021
@yeya24
Copy link
Contributor

yeya24 commented Nov 7, 2021

Let's revisit this. My opinion now is to remove debug metas totally and we don't need a flag.

@yeya24 yeya24 reopened this Nov 7, 2021
@stale stale bot removed the stale label Nov 7, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

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 9, 2022
@yeya24
Copy link
Contributor

yeya24 commented Jan 9, 2022

Let's get back to this finally. @hitanshu-mehta Are you able to continue this? Thank you!

@stale stale bot removed the stale label Jan 9, 2022
@hitanshu-mehta
Copy link
Contributor Author

@yeya24 Yes. So the final decision is to remove debug meta files completely without any flag. right?

@yeya24
Copy link
Contributor

yeya24 commented Jan 9, 2022

@yeya24 Yes. So the final decision is to remove debug meta files completely without any flag. right?

Yes! Thanks for the patience.

@hitanshu-mehta
Copy link
Contributor Author

I apologize for the delay from my side. @r0mdau has opened a PR #5110 fixing this issue. Hence, we can close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci left review comments

@pstibrany pstibrany pstibrany left review comments

@bwplotka bwplotka bwplotka left review comments

@onprem onprem Awaiting requested review from onprem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stop uploading debug meta files to obj store
7 participants
@hitanshu-mehta @bwplotka @yeya24 @pstibrany @kakkoyun @pracucci @onprem