Create a CLI for uploading and downloading blocks #760

kushalShukla-web · 2024-09-24T14:58:47Z

This PR introduces a powerful CLI tool for seamless block uploading and downloading. The initial implementation leverages an object storage as the storage backend, with the potential for other storage backends in the future.

Added a dedicated upload function for uploading data to the object store, with error handling and status logs using log/slog.
A separate download function was created to fetch data from the object store.
Structured logging is a key feature of this PR, made possible by the utilization of the log/slog package. This enables easier traceability and debugging during storage operations.
Insured clear separation of concerns by modularizing upload and download logic into distinct functions.
Integrated the Thanos object storage package to facilitate efficient data storage and retrieval handling.

Test plan

Manual testing locally against a Minio service.

bboreham

I see 'upload'; will you be downloading data at some point?

tools/load-generator/main.go

kushalShukla-web · 2024-09-25T07:47:20Z

I see 'upload'; will you be downloading data at some point?

yes @bboreham I have to implement both upload and download functionality.

kakkoyun

It's a good start. Let's keep up the good work.

infra/infra.go

pkg/provider/store/object_store.go

kakkoyun · 2024-09-25T09:56:47Z

@kushalShukla-web Also, we should work on the PR title and the description.

Please also sign your commits https://github.com/prometheus/test-infra/pull/760/checks?check_run_id=30633470860

tools/object-storage/obj.go

kakkoyun

It's going in a good direction. Thanks a lot. I have commented. We need to clean up the architecture.

We should also work on the naming and documentation. We miss the README as well.

Check this out https://go.dev/blog/package-names

tools/object-storage/Dockerfile

tools/object-storage/obj.go

kakkoyun · 2024-09-27T12:06:13Z

tools/object-storage/upload_download.go

+	config := c.ObjectConfig
+	configBytes, err := os.ReadFile(config)
+	if err != nil {
+		panic(fmt.Errorf("failed to read config file: %v", err))


This function returns an error. Why do we panic? Let's just return the error.

tools/object-storage/upload_download.go

kakkoyun · 2024-09-27T12:12:21Z

tools/object-storage/obj.go

+	app := kingpin.New(filepath.Base(os.Args[0]), "Tool for storing TSDB data to object storage")
+	app.HelpFlag.Short('h')
+
+	s := newstore()


This constructor should get the flag variable. Why do we have a store in an inconsistent state?

kakkoyun · 2024-09-27T12:13:25Z

tools/object-storage/upload_download.go

+	}
+	fmt.Println(exists)
+
+	err = objstore.UploadDir(context.Background(), log.NewNopLogger(), bucket, c.TsdbPath, commitDir)


We should give a proper logger to the buckets.

kakkoyun · 2024-09-27T12:15:13Z

tools/object-storage/upload_download.go

+	}
+	commitDir := c.ObjectKey
+
+	bucket, err := client.NewBucket(log.NewNopLogger(), configBytes, "example")


Suggested change

bucket, err := client.NewBucket(log.NewNopLogger(), configBytes, "example")

bucket, err := client.NewBucket(log.NewNopLogger(), configBytes, "block-sync")

We could use a proper component name.

kakkoyun · 2024-09-27T12:17:09Z

tools/object-storage/upload_download.go

+	if err != nil {
+		panic(err)
+	}
+	exists, err := bucket.Exists(context.Background(), "example")


example is wrong here, no? This should be a parent directory we get as a command line argument.

kakkoyun · 2024-09-27T12:17:48Z

tools/object-storage/upload_download.go

+		panic(fmt.Errorf("failed to read config file: %v", err))
+	}
+	commitDir := c.ObjectKey
+	bucket, err := client.NewBucket(log.NewNopLogger(), configBytes, "example")


Again we should use a proper component name.

tools/blockSyncer/Dockerfile

tools/blockSyncer/Readme.md

kakkoyun · 2024-10-02T10:10:46Z

tools/blockSyncer/obj.go

+	app.HelpFlag.Short('h')
+
+	var tsdbPath, objectConfig, objectKey string
+	var s *Store


You can omit this.

kakkoyun · 2024-10-02T10:11:06Z

tools/blockSyncer/obj.go

+	objstore.Flag("tsdb-path", "Path for The TSDB data in prometheus").Required().StringVar(&tsdbPath)
+	objstore.Flag("objstore.config-file", "Path for The Config file").Required().StringVar(&objectConfig)
+	objstore.Flag("key", "Path for the Key where to store block data").Required().StringVar(&objectKey)
+


Suggested change

s = newstore(tsdbPath, objectConfig, objectKey)

Suggested change

s, err := newStore(...)

if err != nil {

logger.Log(..)

}

Suggested change

ctx, cancel := context.WithCancel(context.Backgroud())

defer cancel()

Suggested change

ctx, stop := signal.NotifyContext(ctx, os.Interrupt)

defer stop()

kakkoyun · 2024-10-02T10:13:14Z

tools/blockSyncer/obj.go

+	objstore.Flag("tsdb-path", "Path for The TSDB data in prometheus").Required().StringVar(&tsdbPath)
+	objstore.Flag("objstore.config-file", "Path for The Config file").Required().StringVar(&objectConfig)
+	objstore.Flag("key", "Path for the Key where to store block data").Required().StringVar(&objectKey)
+


Suggested change

kingpin.MustParse(app.Parse(os.Args[1:]))

I think if we move this to line 34, it will not be able to recognize the download and upload functions because these lines are declared below.

tools/blockSyncer/upload_download.go

bwplotka · 2024-10-14T10:25:46Z

Is this something that would be useful in promtool? 🤔 https://github.com/prometheus/prometheus/tree/main/cmd/promtool

bwplotka · 2024-10-14T10:26:47Z

Ah this is for old data in prombench, cool.

Signed-off-by: Kushal Shukla <[email protected]>

bboreham

few comments

tools/block-sync/README.md

bboreham · 2024-10-22T16:22:44Z

prombench/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml

+            cd /repo1 && \
+            git fetch origin pull/{{ .PR_NUMBER }}/head:pr-branch && \
+            git checkout pr-branch && \
+            cp  objectconfig.yml /mnt/repo/objectconfig.yml && \


This is where you would be combining password etc from some Secret?

prombench/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml

bboreham · 2024-10-22T16:23:18Z

prombench/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml

+      - name: config
+        hostPath:
+          path: /object-config        


Don't do this; just use the instance-ssd volume.

bboreham · 2024-10-22T16:25:22Z

prombench/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml

+      - name: config
+        hostPath:
+          path: /config-file 


Don't do this; just use the instance-ssd volume.

tools/prometheus-builder/build.sh

bboreham · 2024-10-22T16:43:26Z

tools/block-sync/README.md

+The configuration file is essential for connecting to your object storage solution. Below are basic templates for different object storage systems.
+
+```yaml
+type: s3


Are other types available? E.g. GCS?

tools/load-generator/main.go

bboreham · 2024-10-28T16:37:16Z

tools/load-generator/main.go

 	defer resp.Body.Close()

-	duration := time.Since(start)
+	duration := time.Since(*baseTime)


2. changed upload-download function to extract key 3. updated bash.sh file Signed-off-by: Kushal Shukla <[email protected]>

bboreham

We've discussed this PR on video-call and on Slack; I'm repeating a couple of points here:

Make this PR just the CLI program; pull out the yamls and scripting to another PR.
Get the images updated in one PR, then change the yamls that use them. We can't merge a change to use your privately-built image.

bboreham · 2024-11-06T10:40:02Z

prombench/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml

@@ -44,10 +44,35 @@ spec:
        - name: GITHUB_ORG
          value: "{{ .GITHUB_ORG }}"
        - name: GITHUB_REPO
-          value: "{{ .GITHUB_REPO }}"
+          value: "{{ .GITHUB_REPO }}"     


spurious change

bboreham · 2024-11-06T10:40:14Z

prombench/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml

@@ -34,7 +34,7 @@ spec:
        runAsUser: 0
      initContainers:
      - name: prometheus-builder
-        image: docker.io/prominfra/prometheus-builder:master
+        image: kushalshukla/builder


Guessing this one is built from the change in this PR.

bboreham · 2024-11-06T10:52:41Z

tools/prometheus-builder/build.sh

@@ -5,8 +5,7 @@ DIR="/go/src/github.com/prometheus/prometheus"
 if [[ -z $PR_NUMBER || -z $VOLUME_DIR || -z $GITHUB_ORG || -z $GITHUB_REPO ]]; then
    echo "ERROR:: environment variables not set correctly"
    exit 1;
-fi
-
+fi 


Spurious change - removal of blank line.

bboreham · 2024-11-06T10:54:04Z

tools/prometheus-builder/build.sh

+MKDIR="/config"
+cp  "$DIR/key.yml" "$MKDIR/key.yml"


This needs a comment at least.

bboreham · 2024-11-06T13:50:12Z

prombench/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml

+            apk add --no-cache bash && \
+            git clone --depth 1 https://github.com/{{ .GITHUB_ORG }}/{{ .GITHUB_REPO }}.git /repo1 && \
+            cd /repo1 && \
+            git fetch origin pull/{{ .PR_NUMBER }}/head:pr-branch && \
+            git checkout pr-branch && \
+            cp  key.yml /config/key.yml && \
+            rm -rf /repo1


This multi-line command is getting unwieldly. Pull it out to a .sh file.

bboreham · 2024-11-07T13:37:49Z

prombench/manifests/prombench/benchmark/2a_prometheus-secret.yaml

+type: Opaque
+stringData:
+  object-config.yml: |
+    type: S3


This is going to be configured differently for each installation - s3 is good for local installs using Minio, but when run at Google Cloud it needs to say gcs.
I suggest you take this Secret out of the PR and instead put instructions in the docs, e.g. here, for the person who installs Prombench to set it up.

bboreham · 2024-11-07T13:40:17Z

prombench/manifests/prombench/benchmark/2a_prometheus-secret.yaml

+kind: Secret
+metadata:
+  name: s3-secret
+  namespace: prombench-{{ .PR_NUMBER }}  # Replace with your actual namespace


I see you put the Secret in the namespace for the PR, but I see the same bucket being used across many runs.

Key changes 1. removed multiline command and put it in script on prometheus-builder image. 2. configure this in prometheus deployment. 3. added download.sh script which will download the data if key present , else skip the downloading process and move to next container.

bboreham · 2024-11-07T15:29:38Z

One thing that could be added (not vital right now) is to be able to upload several blocks.

Example:

# du -h
4.6G    ./01JC3JCA8Y39ZV72Y0F0FEEQND/chunks
5.8G    ./01JC3JCA8Y39ZV72Y0F0FEEQND
1.5G    ./01JC3J7CCY83F1EZGCBMEPG2RY/chunks
1.9G    ./01JC3J7CCY83F1EZGCBMEPG2RY
4.6G    ./01JC2XS9SGSE6YTKJZSAJP3YR8/chunks
5.8G    ./01JC2XS9SGSE6YTKJZSAJP3YR8
[...]
958.2M  ./chunks_head
85.4M   ./wal/checkpoint.00002015
4.2G    ./wal

I can specify one block, e.g. 01JC3J7CCY83F1EZGCBMEPG2RY, but I can't specify two.
If I upload the entire Prometheus data direectory I will get wal and chunks_head, which we don't want to download for a subsequent Prombench run.

…t.yaml. 2. Added a section for secretes in kind.md file Signed-off-by: Kushal Shukla <[email protected]>

Signed-off-by: Kushal Shukla <[email protected]>

bboreham

Great, let's merge this and see how well it works.

kushalShukla-web force-pushed the blocks branch 3 times, most recently from c2e16e5 to 2bd722d Compare September 24, 2024 15:49

bboreham reviewed Sep 24, 2024

View reviewed changes

tools/load-generator/main.go Outdated Show resolved Hide resolved

kushalShukla-web force-pushed the blocks branch 4 times, most recently from b2a0dcb to 25bcf19 Compare September 25, 2024 09:10

kakkoyun reviewed Sep 25, 2024

View reviewed changes

infra/infra.go Outdated Show resolved Hide resolved

pkg/provider/store/object_store.go Outdated Show resolved Hide resolved

kushalShukla-web force-pushed the blocks branch 4 times, most recently from 15d50a2 to bb712d2 Compare September 25, 2024 20:07

kushalShukla-web commented Sep 25, 2024

View reviewed changes

tools/object-storage/obj.go Outdated Show resolved Hide resolved

kushalShukla-web force-pushed the blocks branch 2 times, most recently from 93ab7c4 to ffb9fdd Compare September 27, 2024 09:49

kakkoyun reviewed Sep 27, 2024

View reviewed changes

kushalShukla-web changed the title ~~Used Module Object storage Bucket~~ Implement Thanos Object Storage Support with Command-Line Data Upload and Download Sep 27, 2024

kushalShukla-web force-pushed the blocks branch 2 times, most recently from b6cb4e7 to c9d5111 Compare October 1, 2024 09:54

kakkoyun reviewed Oct 2, 2024

View reviewed changes

kushalShukla-web requested a review from kakkoyun October 3, 2024 10:41

kushalShukla-web force-pushed the blocks branch from f9f13a2 to 5840472 Compare October 4, 2024 14:13

kushalShukla-web changed the title ~~Implement Thanos Object Storage Support with Command-Line Data Upload and Download~~ Create Thanos Object Storage Support with Command-Line Data Upload and Download Oct 4, 2024

kushalShukla-web force-pushed the blocks branch from 5840472 to b596e31 Compare October 6, 2024 20:48

kakkoyun changed the title ~~Create Thanos Object Storage Support with Command-Line Data Upload and Download~~ Create a CLI for uploading and downloading blocks Oct 8, 2024

kushalShukla-web added 2 commits October 21, 2024 17:04

Implement block sync as init container for Prometheus TSDB setup

1e141c2

Signed-off-by: Kushal Shukla <[email protected]>

Integrated block sync with Prometheus Released version and Pr version.

79a3b3a

Signed-off-by: Kushal Shukla <[email protected]>

kushalShukla-web force-pushed the blocks branch from 8a26921 to 79a3b3a Compare October 21, 2024 11:43

kushalShukla-web requested review from bboreham and kakkoyun October 21, 2024 12:06

Changed Loadgen Query to query data in respective time!

d01a5d7

Signed-off-by: Kushal Shukla <[email protected]>

bboreham reviewed Oct 30, 2024

View reviewed changes

kushalShukla-web force-pushed the blocks branch 6 times, most recently from 06d8387 to ad81105 Compare November 3, 2024 09:54

1. Added key.yml

db44635

2. changed upload-download function to extract key 3. updated bash.sh file Signed-off-by: Kushal Shukla <[email protected]>

kushalShukla-web force-pushed the blocks branch from ad81105 to db44635 Compare November 4, 2024 08:08

bboreham reviewed Nov 6, 2024

View reviewed changes

bboreham reviewed Nov 7, 2024

View reviewed changes

kushalShukla-web force-pushed the blocks branch from 3459af4 to 540fc28 Compare November 7, 2024 15:07

1. Removed secretes , env variables from 3b_prometheus-test_deploymen…

0ef32ea

…t.yaml. 2. Added a section for secretes in kind.md file Signed-off-by: Kushal Shukla <[email protected]>

kushalShukla-web force-pushed the blocks branch 2 times, most recently from 77d1d9d to b8630f4 Compare November 8, 2024 18:44

Removed some files!

8232940

Signed-off-by: Kushal Shukla <[email protected]>

kushalShukla-web force-pushed the blocks branch from b8630f4 to 8232940 Compare November 11, 2024 12:19

bboreham approved these changes Nov 12, 2024

View reviewed changes

bboreham merged commit d9fd1a1 into prometheus:master Nov 12, 2024
4 checks passed

This was referenced Nov 13, 2024

Integrating block-sync in an init-container #779

Open

Changed Upload functionality to upload TSDB data only. #786

Open

	bucket, err := client.NewBucket(log.NewNopLogger(), configBytes, "example")
	bucket, err := client.NewBucket(log.NewNopLogger(), configBytes, "block-sync")

+s, err := newStore(...)
+if err != nil {
+  logger.Log(..)
+ }


	ctx, cancel := context.WithCancel(context.Backgroud())
	defer cancel()


	ctx, stop := signal.NotifyContext(ctx, os.Interrupt)
	defer stop()

Create a CLI for uploading and downloading blocks #760

Create a CLI for uploading and downloading blocks #760

Conversation

kushalShukla-web commented Sep 24, 2024 • edited Loading

Test plan

bboreham left a comment

Choose a reason for hiding this comment

kushalShukla-web commented Sep 25, 2024

kakkoyun left a comment

Choose a reason for hiding this comment

kakkoyun commented Sep 25, 2024

kakkoyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Oct 14, 2024 • edited Loading

bwplotka commented Oct 14, 2024

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham commented Nov 7, 2024

bboreham left a comment

Choose a reason for hiding this comment

kushalShukla-web commented Sep 24, 2024 •

edited

Loading

bwplotka commented Oct 14, 2024 •

edited

Loading