You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/09/21 13:39:31 UTC

[GitHub] [couchdb-helm] flimzy opened a new pull request #42: Cluster setup

flimzy opened a new pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42


   #### What this PR does / why we need it:
   This PR adds the `clusterSetup` variable, which defaults to false. When enabled, a Helm `post-install` hook is initialized which waits for the `_up` endpoint to respond with a 200, then finalizes cluster setup by sending the requisite POST to `/_cluster_setup`.
   
   #### Which issue this PR fixes
   Fixes #41 
   
   #### Special notes for your reviewer:
   
   #### Checklist
   [Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.
   - [x] Chart Version bumped
   - [ ] e2e tests pass
   - [x] Variables are documented in the README.md
   - [ ] Chart tgz added to /docs and index updated
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696134224






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696137912


   > * `adminPassword` might not match what's deployed. This is a hangover from allowing users to set the admin hash explicitly to ensure that all pods use an identical value, but means they need to ensure the admin password is also passed to the chart.
   
   Is there anything we can do about this, other than documenting the behavior?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] willholley commented on a change in pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

willholley commented on a change in pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#discussion_r492057733



##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       it's possible `_up` requires authentication. You likely need to copy the logic from https://github.com/apache/couchdb-helm/blob/master/couchdb/templates/statefulset.yaml#L87 to deal with this




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] krjackso commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

krjackso commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-710502617


   I ran into this same issue, is this fix still being considered?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy removed a comment on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy removed a comment on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696148909


   > We can verify the behaviour by running the `Kind` tests and adding a new scenario to https://github.com/apache/couchdb-helm/tree/master/couchdb/ci
   
   Please forgive my ignorance on this. How are these test scenarios used? A `git grep` for them doesn't find them listed anywhere, so they must be referenced by a glob somewhere that I'm not finding?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] willholley commented on a change in pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

willholley commented on a change in pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#discussion_r492082880



##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       ah sorry - I think your approach is neater.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] willholley commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

willholley commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696147441


   > E2E tests do pass for me, once I apply this small patch. Should I add this to the PR?
   > 
   > ```diff
   > --- a/test/kind-config.yaml
   > +++ b/test/kind-config.yaml
   > @@ -1,5 +1,5 @@
   >  kind: Cluster
   > -apiVersion: kind.sigs.k8s.io/v1alpha3
   > +apiVersion: kind.x-k8s.io/v1alpha4
   >  nodes:
   >    - role: control-plane
   >  E2E tests do pass for me, once I apply this small patch. Should I add this to the PR?
   
   Yes please. Bear in mind the tests will use the default values, so won't execute with `clusterSetup: true` unless it's added as a scenario in https://github.com/apache/couchdb-helm/tree/master/couchdb/ci. Essentially, the E2E tests are executed using each file as the `values.yaml` input to Helm, so you can either add a new file to test a specific set of values or incorporate the new setting into an existing test.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] willholley commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

willholley commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696147441






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696134224


   E2E tests do pass for me, once I apply this small patch.  Should I add this to the PR?
   
   ```diff
   --- a/test/kind-config.yaml
   +++ b/test/kind-config.yaml
   @@ -1,5 +1,5 @@
    kind: Cluster
   -apiVersion: kind.sigs.k8s.io/v1alpha3
   +apiVersion: kind.x-k8s.io/v1alpha4
    nodes:
      - role: control-plane
      - role: worker
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy commented on a change in pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy commented on a change in pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#discussion_r492070818



##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       I went to a little effort to avoid providing the password on the curl command line. See here: https://github.com/apache/couchdb-helm/pull/42/files#diff-339223d6a106ee3d80bf47312764fc20R25-R28
   
   I'm not sure how important that really is within Kubernetes. If it's not important, I can use your simpler approach in both places. If you think it does provide any value, should I use the same/

##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       I went to a little effort to avoid providing the password on the curl command line. See here: https://github.com/apache/couchdb-helm/pull/42/files#diff-339223d6a106ee3d80bf47312764fc20R25-R28
   
   I'm not sure how important that really is within Kubernetes. If it's not important, I can use your simpler approach in both places. If you think it does provide any value, should I use the same?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] willholley commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

willholley commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696183021


   I wonder if this has rotted against the latest `kind` verison. `master` passes for me using:
   
   ```
   $ kind version
   kind v0.8.1 go1.14.2 darwin/amd64
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696148909


   > We can verify the behaviour by running the `Kind` tests and adding a new scenario to https://github.com/apache/couchdb-helm/tree/master/couchdb/ci
   
   Please forgive my ignorance on this. How are these test scenarios used? A `git grep` for them doesn't find them listed anywhere, so they must be referenced by a glob somewhere that I'm not finding?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy commented on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy commented on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696154241

I seem to have spoken prematurely about the passage of E2E tests. I see now there is an error. I get the same error when running the tests against `master` (with the same patch I mentioned above). Should I open a separate issue for this?

```
$ make test
./test/e2e-kind.sh
Running ct container...
2e66ed0293436ba70915cb3e6620a618076384a559099f840967ce2cb0b8f1d9

Deleting cluster "chart-testing" ...
Creating cluster "chart-testing" ...
✓ Ensuring node image (kindest/node:v1.18.2) 🖼
✓ Preparing nodes 📦 📦
✓ Writing configuration 📜
✓ Starting control-plane 🕹️
✓ Installing CNI 🔌
✓ Installing StorageClass 💾
✓ Joining worker nodes 🚜
✓ Waiting ≤ 1m0s for control-plane = Ready ⏳
• Ready after 0s 💚
Set kubectl context to "kind-chart-testing"
You can now use your cluster with:

kubectl cluster-info --context kind-chart-testing

Have a question, bug, or feature request? Let us know! https://kind.sigs.k8s.io/#community 🙂
Copying kubeconfig to container...
error: Missing or incomplete configuration info. Please point to an existing, complete config file:

1. Via the command-line flag --kubeconfig
2. Via the KUBECONFIG environment variable
3. In your home directory as ~/.kube/config

To view or setup config directly use the 'config' command.
Removing ct container...
Deleting cluster "chart-testing" ...
Done!
make: *** [Makefile:31: test] Error 1
```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy commented on a change in pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy commented on a change in pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#discussion_r492070818



##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       I went to a little effort to avoid providing the password on the curl command line. See here: https://github.com/apache/couchdb-helm/pull/42/files#diff-339223d6a106ee3d80bf47312764fc20R25-R28
   
   I'm not sure how important that really is within Kubernetes. If it's not important, I can use your simpler approach in both places. If you think it does provide any value, should I use the same/

##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       I went to a little effort to avoid providing the password on the curl command line. See here: https://github.com/apache/couchdb-helm/pull/42/files#diff-339223d6a106ee3d80bf47312764fc20R25-R28
   
   I'm not sure how important that really is within Kubernetes. If it's not important, I can use your simpler approach in both places. If you think it does provide any value, should I use the same?

##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       I've made the update using the curl config file option, but if you tell me that's not useful in a K8s context, I'll update to use CLI flags.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] flimzy removed a comment on pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

flimzy removed a comment on pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#issuecomment-696148909


   > We can verify the behaviour by running the `Kind` tests and adding a new scenario to https://github.com/apache/couchdb-helm/tree/master/couchdb/ci
   
   Please forgive my ignorance on this. How are these test scenarios used? A `git grep` for them doesn't find them listed anywhere, so they must be referenced by a glob somewhere that I'm not finding?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb-helm] willholley commented on a change in pull request #42: Cluster setup

Posted by GitBox <gi...@apache.org>.

willholley commented on a change in pull request #42:
URL: https://github.com/apache/couchdb-helm/pull/42#discussion_r492057733



##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       it's possible `_up` requires authentication. You likely need to copy the logic from https://github.com/apache/couchdb-helm/blob/master/couchdb/templates/statefulset.yaml#L87 to deal with this

##########
File path: couchdb/templates/clustersetup.yaml
##########
@@ -0,0 +1,76 @@
+{{ if .Values.clusterSetup }}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cluster-setup-script
+  labels:
+    app: {{ template "couchdb.name" . }}
+    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
+    release: {{ .Release.Name }}
+    heritage: {{ .Release.Service }}
+data:
+  setup.sh: |
+    set -e
+    BASE_URL=http://{{ template "couchdb.fullname" . }}:5984
+    set -x
+
+    echo "Waiting for CouchDB service to start..."
+    until curl --silent -max-time 5 --head --fail "${BASE_URL}/_up"; do

Review comment:
       ah sorry - I think your approach is neater.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org