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

Integrating block-sync in an init-container #779

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kushalShukla-web
Copy link
Contributor

@kushalShukla-web kushalShukla-web commented Nov 8, 2024

  1. Changes that are introduced in this pr , followed by Create a CLI for uploading and downloading blocks #760 .
    Integrated download-key in an init container that downloads the key.yml file from the Prometheus repository. It uses the key.sh script to fetch the pull request and copy the key.yml file into the volumeMount section:
volumeMounts:
  - name: key
    mountPath: /config

  1. Integrated data-downloaderon both pr and released side, which uses the block-sync binary to download pre-generated block data.
  2. Updated the prometheus-builder image, modifying the builder.sh file to copy the key.yml file into the volumeMount section so that the PR section can also download the pre-generated block data.

@kushalShukla-web kushalShukla-web changed the title Added yaml file changes and prometheus builder changes that was removed changes that are not included in Block-sync Nov 8, 2024
@kushalShukla-web kushalShukla-web changed the title changes that are not included in Block-sync Integrating Block-sync in an Init-Container. Nov 9, 2024
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

I'm struggling to find my way around. Some trivial points stand out, but I need better orientation.

removed from block-sync:

Please explain each PR in terms of what it does, not what some other PR didn't do.

For example, you could describe the intended purpose of key.sh.

Comment on lines 122 to 124
- **Option 2: Skip downloading data**

If you don’t need to download data, edit the `3b_prometheus-test_deployment.yaml` file:
Copy link
Member

Choose a reason for hiding this comment

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

Could we instead tell people to create an empty Secret?

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

Choose a reason for hiding this comment

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

Fix up before merge.

@@ -6,7 +6,7 @@ if [[ -z $PR_NUMBER || -z $VOLUME_DIR || -z $GITHUB_ORG || -z $GITHUB_REPO ]]; t
echo "ERROR:: environment variables not set correctly"
exit 1;
fi

Copy link
Member

Choose a reason for hiding this comment

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

Spurious change

RUN chmod +x /go/src/github.com/build.sh

ENTRYPOINT ["/go/src/github.com/build.sh"]
ENTRYPOINT ["/go/src/github.com/build.sh"]
Copy link
Member

Choose a reason for hiding this comment

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

Spurious change

@@ -4,6 +4,8 @@ RUN mkdir -p /go/src/github.com

COPY ./build.sh /go/src/github.com/build.sh

COPY ./key.sh /download-key/key.sh
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in such a different place from the other script?

# emptyDir. This file will later be used by the data-downloader init container.
MKDIR="/config"
if [ -f "$DIR/key.yml" ]; then
echo "File exists."
Copy link
Member

Choose a reason for hiding this comment

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

Who is expected to read this message, and what are they expected to do about it?

Comment on lines 117 to 124
2. Before applying benchmarking objects , You have two choices to make:
- **Option 1: Download data from object storage**
Copy link
Member

Choose a reason for hiding this comment

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

How would someone coming to this for the first time understand this choice?
Can we give some context why data might be downloaded?

@kushalShukla-web kushalShukla-web force-pushed the block-yml branch 6 times, most recently from 65358a1 to b18501c Compare November 13, 2024 07:10
@kakkoyun kakkoyun changed the title Integrating Block-sync in an Init-Container. Integrating Block-sync in an init-container Nov 13, 2024
# Here, MKDIR is specified in the volumeMount section of the prometheus-builder init container,
# where it will copy the key.yml file from the Prometheus directory to the volume section of the
# emptyDir. This file will later be used by the data-downloader init container.
MKDIR="/config"
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to change the name.

@@ -24,6 +24,17 @@ fi

git checkout pr-branch

# Here, MKDIR is specified in the volumeMount section of the prometheus-builder init container,
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to have this in another script.

@kakkoyun kakkoyun changed the title Integrating Block-sync in an init-container Integrating block-sync in an init-container Nov 13, 2024
@kushalShukla-web kushalShukla-web force-pushed the block-yml branch 3 times, most recently from 42e7b36 to 1dfe367 Compare November 14, 2024 12:49
2. Modified Prometheus deployment yml file
3. changed Prometheus Builder Docker file
Signed-off-by: Kushal Shukla <[email protected]>
@kushalShukla-web kushalShukla-web force-pushed the block-yml branch 4 times, most recently from 8197ba6 to 1dbaaf1 Compare November 15, 2024 07:20
@kushalShukla-web kushalShukla-web force-pushed the block-yml branch 2 times, most recently from aab03cb to 18b922f Compare November 19, 2024 15:05
prombench/docs/eks.md Outdated Show resolved Hide resolved
Here’s how each option works:
- **Option 1: Download data from object storage**

To download data from object storage, create a Kubernetes secret with exact named `bucket-secret` and file name ```object-config.yml``` with the necessary credentials as per your object storage. This secret enables access to the stored data.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To download data from object storage, create a Kubernetes secret with exact named `bucket-secret` and file name ```object-config.yml``` with the necessary credentials as per your object storage. This secret enables access to the stored data.
To download data from object storage, create a Kubernetes secret with exact named `bucket-secret` and file name `object-config.yml` with the necessary credentials as per your object storage. This secret enables access to the stored data.

prombench/docs/eks.md Outdated Show resolved Hide resolved
prombench/docs/eks.md Show resolved Hide resolved
echo "INFO:: key.yml file is Present on $DIR/key.yml directory."
cp "$DIR/key.yml" "$STORAGE/key.yml"
else
echo "INFO:: key.yml File does not exist on $DIR/key.yml directory."
Copy link
Member

Choose a reason for hiding this comment

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

We should add more context or remove this line. e.g. File does not exists so we won't download XXX

# where it will copy the key.yml file from the Prometheus directory to the volume section of the
# emptyDir. This file will later be used by the data-downloader init container.
if [ -f "$DIR/key.yml" ]; then
echo "INFO:: key.yml file is Present on $DIR/key.yml directory."
Copy link
Member

@kakkoyun kakkoyun Nov 20, 2024

Choose a reason for hiding this comment

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

"It exists so we will download the blocks from XXX."

Signed-off-by: Kushal Shukla <[email protected]>
@kushalShukla-web kushalShukla-web force-pushed the block-yml branch 3 times, most recently from b405f52 to 0f98ebd Compare November 21, 2024 13:49
Removed the code for exiting container when secret is not present.
Signed-off-by: Kushal Shukla <[email protected]>
@kushalShukla-web kushalShukla-web force-pushed the block-yml branch 2 times, most recently from ec570f4 to 905b6b3 Compare November 22, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants