Integrate BanyanDB as storage solution #126

ButterBright · 2023-07-19T08:20:50Z

Integrate BanyanDB as storage solution.

hanahmily · 2023-08-07T11:52:03Z

chart/banyandb/templates/deployment.yaml

+# under the License.
+
+apiVersion: apps/v1
+kind: Deployment


This should be a statefulset.

kezhenxu94 · 2023-08-08T02:04:08Z

CHANGES.md

@@ -14,6 +14,7 @@ Release Notes.
 - Use startup probe option for first initialization of application
 - Allow setting env for UI deployment.
 - Add Istio ServiceEntry permissions.
+- Integrate BanyanDB as storage solution


4.5.0 is released, please don't change the changelog here, add a new section for 4.6.0

wu-sheng · 2023-08-08T13:33:02Z

Let's update the readme in the root. There are a lot of ElasticSearch docs there, you are setting up new storage, so you should refer to them.

ButterBright · 2023-08-08T13:43:53Z

Okay, I got it.

innerpeacez · 2023-08-09T03:10:42Z

test/e2e/e2e.yaml

@@ -55,7 +53,9 @@ setup:
                       --set ui.image.tag=$UI_TAG \
                       --set oap.image.repository=$OAP_REPO \
                       --set oap.image.tag=$OAP_TAG \
-                       --set oap.storageType=elasticsearch \
+                       --set oap.storageType=banyandb \
+                       --set elasticsearch.enabled=false \


Both ES and BanyanDB should be covered by e2e tests.

kezhenxu94 · 2023-08-10T01:58:26Z

chart/banyandb/Chart.yaml

+# A chart can be either an 'application' or a 'library' chart.
+#
+# Application charts are a collection of templates that can be packaged into versioned archives
+# to be deployed.
+#
+# Library charts provide useful utilities or functions for the chart developer. They're included as
+# a dependency of application charts to inject those utilities and functions into the rendering
+# pipeline. Library charts do not define any templates and therefore cannot be deployed.
+type: application
+
+# This is the chart version. This version number should be incremented each time you make changes
+# to the chart and its templates, including the app version.
+# Versions are expected to follow Semantic Versioning (https://semver.org/)
+version: 0.1.0
+
+# This is the version number of the application being deployed. This version number should be
+# incremented each time you make changes to the application. Versions are not expected to
+# follow Semantic Versioning. They should reflect the version the application is using.
+# It is recommended to use it with quotes.
+appVersion: "1.16.0"


Let's clean up the comments in this file, most of the comments are not necessary

kezhenxu94 · 2023-08-10T02:03:14Z

chart/skywalking/Chart.yaml

+  - name: banyandb
+    version: 0.1.0
+    repository: file://../banyandb
+    condition: banyandb.enabled


I think this doesn't work (?) when we release the skywalking Chart to Docker hub, using this installation method https://github.com/apache/skywalking-kubernetes#install-released-version-using-docker-helm-repository--430

What should I change the repo to? Something like oci://registry-1.docker.io/apache/skywalking-banyandb-helm?

wu-sheng

Hold due to new repo should be created.

Mail list, https://lists.apache.org/thread/l6c9s5t2opljbrs5fsd6xs42nh4lh7bn

@ButterBright Please reach @hanahmily for more context. And most of your PR should be reusable.

wu-sheng · 2023-08-11T08:00:07Z

New repo created, please move your helm charts there, https://github.com/apache/skywalking-banyandb-helm

ButterBright · 2023-08-11T08:10:38Z

I see.

hanahmily

LGTM

wu-sheng · 2023-09-14T00:08:09Z

chart/skywalking/Chart.yaml

@@ -38,3 +38,7 @@ dependencies:
    version: 12.1.2
    repository: https://raw.githubusercontent.com/bitnami/charts/archive-full-index/bitnami
    condition: postgresql.enabled
+  - name: skywalking-banyandb-helm
+    version: 0.0.0-67df1c7
+    repository: oci://ghcr.io/apache/skywalking-banyandb-helm


I don't think we could use this as the final release. We should request for docker hub repository, and do a release for banyandb helm. @hanahmily

Because we deploy skywalking chart snapshot to ghcr so this makes sense in main branch but have to remember to change to a released one for banyandb chart before releasing. This also implies that a skywalking chart release depends on a banyandb release.

Makes sense to me. We will release the banyandb helm once the cluster mode is supported later. @ButterBright please make a note of this.

Because we deploy skywalking chart snapshot to ghcr so this makes sense in main branch but have to remember to change to a released one for banyandb chart before releasing. This also implies that a skywalking chart release depends on a banyandb release.

For me, the concern is, that we don't release helm repo in high frequency, so, most likely we definitely will lose track and release the next helm with this snapshot repository.

This PR is never urgent, so, if you feel the BanyanDB helm repo is ready to release, please go ahead to do so. I believe no matter banyandb helm repo is relying on BanyanDB server 0.5 release or not, it is close to be ready.

Makes sense to me. We will release the banyandb helm once the cluster mode is supported later. @ButterBright please make a note of this.

Okay.

Because we deploy skywalking chart snapshot to ghcr so this makes sense in main branch but have to remember to change to a released one for banyandb chart before releasing. This also implies that a skywalking chart release depends on a banyandb release.

For me, the concern is, that we don't release helm repo in high frequency, so, most likely we definitely will lose track and release the next helm with this snapshot repository.

This PR is never urgent, so, if you feel the BanyanDB helm repo is ready to release, please go ahead to do so. I believe no matter banyandb helm repo is relying on BanyanDB server 0.5 release or not, it is close to be ready.

Let's wait for banyandb's first official release then.

Agreed. @ButterBright, could you please release the banyandb helm chart version 0.1.0? @kezhenxu94, would you kindly guide them through the process?

Agreed. @ButterBright, could you please release the banyandb helm chart version 0.1.0? @kezhenxu94, would you kindly guide them through the process?

Reach out to me if there is any help I can offer, btw, it's too troublesome to ask a non-committer to release, even it's a committer who is releasing, one of the steps in release process still needs a PMC member to help, so basically there is few stuff can be done by @ButterBright to propose a release yet.

We can't accept non committer to do a release. Because ASF requires apache ID and his/her sign based on that.
@hanahmily You need to take the responsibility to be release manager.

I will take care of the release

innerpeacez · 2023-09-14T06:03:41Z

chart/skywalking/templates/_helpers.tpl

+  image: curlimages/curl
+  imagePullPolicy: IfNotPresent
+  command: ['sh', '-c', 'for i in $(seq 1 60); do curl {{ $banyandbHost }}:{{ .Values.banyandb.config.httpPort }}/api/healthz && exit 0 || sleep 5; done; exit 1']
+


Suggested change

innerpeacez · 2023-09-14T06:09:04Z

test/e2e/setup-e2e-shell/install.sh

@@ -27,9 +27,9 @@ CURRENT_DIR="$(cd "$(dirname $0)"; pwd)"
 # prepare base dir
 TMP_DIR=/tmp/skywalking-infra-e2e
 BIN_DIR=/usr/local/bin
-mkdir -p $TMP_DIR && cd $TMP_DIR
+mkdir -p $TMP_DIR  && cd $TMP_DIR


Suggested change

mkdir -p $TMP_DIR && cd $TMP_DIR

mkdir -p $TMP_DIR && cd $TMP_DIR

innerpeacez · 2023-09-14T06:25:25Z

test/e2e/setup-e2e-shell/install-helm.sh

@@ -25,6 +25,5 @@ HELMVERSION=${HELMVERSION:-'helm-v3.0.0'}

 if ! command -v helm &> /dev/null; then
  mkdir -p $BASE_DIR/helm && cd $BASE_DIR/helm
-  curl -sSL https://get.helm.sh/${HELMVERSION}-linux-amd64.tar.gz
-  tar xz -C $BIN_DIR --strip-components=1 linux-amd64/helm
+  curl -sSL https://get.helm.sh/${HELMVERSION}-linux-amd64.tar.gz | tar xz -C $BIN_DIR --strip-components=1 linux-amd64/helm


The | pipeline seems to be unnecessary.

wu-sheng

Unblock

wu-sheng · 2023-10-09T00:14:30Z

@innerpeacez Please recheck.

innerpeacez · 2023-10-09T01:58:46Z

README.md

+
+```shell
+export REPO=chart
+git clone https://github.com/apache/skywalking-kubernetes


Suggested change

git clone https://github.com/apache/skywalking-kubernetes

git clone https://github.com/apache/skywalking-helm

innerpeacez · 2023-10-09T01:59:27Z

README.md

+```shell
+export REPO=chart
+git clone https://github.com/apache/skywalking-kubernetes
+cd skywalking-kubernetes


Suggested change

cd skywalking-kubernetes

cd skywalking-helm

wu-sheng · 2023-10-09T03:07:30Z

.github/workflows/e2e.ci.yaml

+      - name: Run Skywalking E2E Test (Elasticsearch as database)
        uses: apache/skywalking-infra-e2e@45584853d6f660102c523b1e9cb5815d12ae55d3
        with:
-          e2e-file: $GITHUB_WORKSPACE/test/e2e/e2e.yaml
+          e2e-file: $GITHUB_WORKSPACE/test/e2e/e2e-elasticsearch.yaml
+      - name: Run Skywalking E2E Test (BanyanDB as database)
+        uses: apache/skywalking-infra-e2e@45584853d6f660102c523b1e9cb5815d12ae55d3
+        with:
+          e2e-file: $GITHUB_WORKSPACE/test/e2e/e2e-banyandb.yaml
      - name: Run SWCK E2E Test
        uses: apache/skywalking-infra-e2e@afdf1cca0519d65bc480d8680b7a27f9b41fc421
        with:


We should run e2e in the parallel mode. You could ref the main repo skywalking.yml to use matrix to define name and config.

wu-sheng · 2023-10-10T03:34:46Z

You need to resolve the conflicts, maybe your local branch is not up-to-date?

ButterBright · 2023-10-10T03:35:42Z

You need to resolve the conflicts, maybe your local branch is not up-to-date?

Oh yeah, I see.

* Fix ci

wu-sheng · 2023-10-10T07:19:18Z

Please check the task names, they have the same recent suffix. They should have meaningful suffix, such as banyandb, swck...

ButterBright · 2023-10-10T07:49:08Z

Please check the task names, they have the same recent suffix. They should have meaningful suffix, such as banyandb, swck...

Can I simply make it the same as e2e test name? For example, Run Skywalking E2E Test (Elasticsearch as database), which I think also makes sense.

wu-sheng · 2023-10-10T09:11:23Z

Please check the task names, they have the same recent suffix. They should have meaningful suffix, such as banyandb, swck...

Can I simply make it the same as e2e test name? For example, Run Skywalking E2E Test (Elasticsearch as database), which I think also makes sense.

Oops, yes, please do that. I accidentally merge this. Please open another PR to rename. @ButterBright

Integrate BanyanDB as storage solution

40c8a6c

ButterBright marked this pull request as draft July 19, 2023 08:21

ButterBright and others added 4 commits July 20, 2023 15:16

Bump up swck to v0.8.0

a508921

Merge branch 'apache:master' into master

b46bdb0

Update grpc-health-probe image

8fe0feb

Use BanyanDB healthz api to do health check

d19eaf3

ButterBright marked this pull request as ready for review August 3, 2023 09:01

hanahmily reviewed Aug 7, 2023

View reviewed changes

kezhenxu94 reviewed Aug 8, 2023

View reviewed changes

ButterBright added 3 commits August 8, 2023 21:16

Change deployment to statefulset

22861b6

Update CHANGES.md

25220da

Update CHANGES.md

bc6a673

wu-sheng added this to the 4.6.0 milestone Aug 8, 2023

innerpeacez reviewed Aug 9, 2023

View reviewed changes

ButterBright added 2 commits August 9, 2023 20:47

Add README.md for BanyanDB

678d6a0

Set up test for Elasticsearch

53d61bd

kezhenxu94 reviewed Aug 10, 2023

View reviewed changes

wu-sheng requested changes Aug 11, 2023

View reviewed changes

ButterBright and others added 3 commits September 11, 2023 17:29

Merge branch 'apache:master' into master

de6caef

Update README.md (apache#129)

1f5cf73

Remove BanyanDB chart

4c27e3e

hanahmily requested review from wu-sheng, kezhenxu94 and innerpeacez September 13, 2023 23:27

hanahmily approved these changes Sep 13, 2023

View reviewed changes

wu-sheng reviewed Sep 14, 2023

View reviewed changes

innerpeacez reviewed Sep 14, 2023

View reviewed changes

ButterBright added 4 commits September 14, 2023 14:40

Some fixups

456a583

Update BanyanDB chart

16b2035

Fix repository name

b1faf52

Remove needless ci step

03069e6

ButterBright mentioned this pull request Sep 27, 2023

Add CI apache/skywalking-banyandb-helm#6

Merged

wu-sheng requested a review from innerpeacez October 9, 2023 00:13

wu-sheng approved these changes Oct 9, 2023

View reviewed changes

innerpeacez reviewed Oct 9, 2023

View reviewed changes

innerpeacez and others added 2 commits October 9, 2023 10:00

Merge branch 'master' into master

3dc2878

Merge branch 'master' into master

220696d

wu-sheng added the enhancement New feature or request label Oct 9, 2023

wu-sheng requested changes Oct 9, 2023

View reviewed changes

ButterBright added 2 commits October 10, 2023 11:45

Merge remote-tracking branch 'upstream/master'

6da849c

* Fix doc

8138cce

* Fix ci

ButterBright force-pushed the master branch from 48c110a to 8138cce Compare October 10, 2023 06:34

wu-sheng approved these changes Oct 10, 2023

View reviewed changes

wu-sheng merged commit 1cadd6d into apache:master Oct 10, 2023
4 checks passed

	mkdir -p $TMP_DIR && cd $TMP_DIR
	mkdir -p $TMP_DIR && cd $TMP_DIR

	git clone https://github.com/apache/skywalking-kubernetes
	git clone https://github.com/apache/skywalking-helm

Integrate BanyanDB as storage solution #126

Integrate BanyanDB as storage solution #126

Conversation

ButterBright commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng commented Aug 8, 2023

ButterBright commented Aug 8, 2023

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

wu-sheng left a comment

Choose a reason for hiding this comment

wu-sheng commented Aug 11, 2023

ButterBright commented Aug 11, 2023

hanahmily 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

wu-sheng left a comment

Choose a reason for hiding this comment

wu-sheng commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng commented Oct 10, 2023

ButterBright commented Oct 10, 2023

wu-sheng commented Oct 10, 2023

ButterBright commented Oct 10, 2023

wu-sheng commented Oct 10, 2023