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

feat(Scaler): Adds CouchDB Scaler #3804

Merged
merged 6 commits into from
Dec 8, 2022
Merged

feat(Scaler): Adds CouchDB Scaler #3804

merged 6 commits into from
Dec 8, 2022

Conversation

26tanishabanik
Copy link
Contributor

@26tanishabanik 26tanishabanik commented Nov 1, 2022 •
edited by tomkerkhove
Loading

Provide a description of what has been changed

  • Adds CouchDB Scaler for autoscaling based on Couchdb queries

Checklist

  • When introducing a new scaler, I agree with the scaling governance policy
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Relates to #3746
Relates to kedacore/keda-docs#967

@26tanishabanik 26tanishabanik requested a review from a team as a code owner November 1, 2022 15:51
@26tanishabanik 26tanishabanik force-pushed the couchdb-scaler branch 2 times, most recently from 2bcc5f0 to fe6862e Compare November 1, 2022 18:47
@tomkerkhove tomkerkhove mentioned this pull request Nov 2, 2022
Merged
1 task
JorTurFer
JorTurFer requested changes Nov 13, 2022
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Sorry for the slow response... Thanks a lot for the new scaler, I have left some comments inline.
In general, don't use fmt.Print functions because there are loggers for doing it, following the log policy applied by end-users.
There are also error in static checks and also unit test are failing

CHANGELOG.md Outdated
@@ -42,6 +42,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio
- **General:** Introduce new Loki Scaler ([#3699](https://github.com/kedacore/keda/issues/3699))
- **General**: Add ratelimitting parameters to keda manager to allow override of client defaults ([#3730](https://github.com/kedacore/keda/issues/2920))
- **General**: Provide Prometheus metric with indication of total number of triggers per trigger type in `ScaledJob`/`ScaledObject`. ([#3663](https://github.com/kedacore/keda/issues/3663))
- **General:** Introduce new CouchDB Scaler ([#3746](https://github.com/kedacore/keda/issues/3746))
Copy link
Member

Choose a reason for hiding this comment

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

We have a style guide for the changelog, all entries should be alphabetically sorted, with the General section in the top (bit also sorted)

config/manager/kustomization.yaml Outdated
@@ -5,5 +5,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
images:
- name: ghcr.io/kedacore/keda
newName: ghcr.io/kedacore/keda
newName: docker.io/tanishabanik/keda
Copy link
Member

Choose a reason for hiding this comment

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

Undo this change please

config/metrics-server/kustomization.yaml Outdated
@@ -10,5 +10,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
images:
- name: ghcr.io/kedacore/keda-metrics-apiserver
newName: ghcr.io/kedacore/keda-metrics-apiserver
newName: docker.io/tanishabanik/keda-metrics-apiserver
Copy link
Member

Choose a reason for hiding this comment

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

undo this change please

pkg/scalers/couchdb_scaler.go Outdated
Comment on lines 4 to 14
"context"
"encoding/json"
"fmt"
couchdb "github.com/go-kivik/couchdb/v3"
"github.com/go-kivik/kivik/v3"
"github.com/go-logr/logr"
kedautil "github.com/kedacore/keda/v2/pkg/util"
v2 "k8s.io/api/autoscaling/v2"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/metrics/pkg/apis/external_metrics"
"strconv"
Copy link
Member

Choose a reason for hiding this comment

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

We split the import section in 3 blocks:

  • golang
  • deps
  • keda
    The static analyzer checks this indeed, that's one of the failures in static checks

pkg/scalers/couchdb_scaler.go Outdated
var request couchDBQueryRequest
err := json.Unmarshal([]byte(s.metadata.query), &request)
if err != nil {
fmt.Println("Couldn't unmarshal query string: ", err)
Copy link
Member

Choose a reason for hiding this comment

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

Please, don't print messages, we use loggers for that

tests/scalers/couchdb/couchdb_test.go Outdated
dsn := fmt.Sprintf("http://admin:%s@test-release-svc-couchdb.%s.svc.cluster.local:5984/animals", password, testNamespace)
cmd := "-vX PUT " + dsn
fmt.Println("Splitted command: ", strings.Split(cmd, " "))
deployPod(kc, "activation", cmd)
Copy link
Member

Choose a reason for hiding this comment

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

Use templates for the sake of consistency

tests/scalers/couchdb/couchdb_test.go Outdated
Comment on lines 251 to 271
func deployPodDelete(kc *kubernetes.Clientset, podName, args string) {
podSpec := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "test-pod-couchdb-" + podName,
Namespace: testNamespace,
},
Spec: corev1.PodSpec{
RestartPolicy: "OnFailure",
Containers: []corev1.Container{
{
Name: "test-couchdb-container",
Image: "nginx:alpine",
Command: []string{"curl"},
Args: strings.Split(args, " "),
},
},
},
}
_, err := kc.CoreV1().Pods(testNamespace).Create(context.Background(), podSpec, metav1.CreateOptions{})
if err != nil {
fmt.Println("Failed to create test pod: ", err)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please, use templates for this

tests/scalers/couchdb/couchdb_test.go Outdated
Comment on lines 276 to 282
fmt.Printf("Rev id: %s-test", rev)
fmt.Println()
dsn := fmt.Sprintf("http://admin:%s@test-release-svc-couchdb.%s.svc.cluster.local:5984/animals/%s?rev=%s", password, testNamespace, uuid, rev)
cmd := " -X DELETE " + dsn
fmt.Println("curl" + cmd)
fmt.Println("curl" + cmd)
deployPodDelete(kc, name, cmd)
Copy link
Member

Choose a reason for hiding this comment

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

Same as other lines

tests/scalers/couchdb/couchdb_test.go Outdated
Comment on lines 285 to 302
type PodLog struct {
OK bool `json:"ok"`
ID string `json:"id"`
Rev string `json:"rev"`
}

func getPodLogs(kc *kubernetes.Clientset, podName string) (string, error) {
podLogOpts := corev1.PodLogOptions{}
req := kc.CoreV1().Pods(testNamespace).GetLogs(podName, &podLogOpts)
podLogs, err := req.Stream(context.Background())
if err != nil {
fmt.Println("error in opening stream: ", err)
return "", err
}
defer podLogs.Close()

buf := new(bytes.Buffer)
_, err = io.Copy(buf, podLogs)
if err != nil {
fmt.Println("error in copy information from podLogs to buf: ", err)
return "", err
}
str := buf.String()
fmt.Println("Index: ", strings.Index(str, `"rev":`))
chars := str[strings.Index(str, `"rev":`)+7 : strings.Index(str, `}`)-1]
fmt.Println(chars)
fmt.Println("Type of chars: ", reflect.TypeOf(chars))
return chars, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

There is a helper for this https://github.com/kedacore/keda/blob/main/tests/helper/helper.go#L507

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a customised pod log function, I am trying to get specific lines with some string ops

tests/scalers/couchdb/couchdb_test.go Show resolved Hide resolved
@JorTurFer JorTurFer requested a review from zroubalik November 13, 2022 20:04
@JorTurFer
Copy link
Member

JorTurFer commented Nov 13, 2022 •
edited by github-actions bot
Loading

/run-e2e couch*
Update: You can check the progress here

JorTurFer
JorTurFer reviewed Nov 13, 2022
View reviewed changes
tests/scalers/couchdb/couchdb_test.go Outdated
func installCouchDB(t *testing.T) {
_, err := ExecuteCommand(fmt.Sprintf("helm repo add couchdb %s", couchdbHelmRepo))
assert.NoErrorf(t, err, "cannot execute command - %s", err)
_, err = ExecuteCommand("sudo helm repo update")
Copy link
Member

Choose a reason for hiding this comment

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

Do we need sudo here? e2e test fails because sudo isn't installed in the agents

tests/scalers/couchdb/couchdb_test.go Outdated
_, err = ExecuteCommand("sudo helm repo update")
assert.NoErrorf(t, err, "cannot execute command - %s", err)
uuid := generateUUID()
_, err = ExecuteCommand(fmt.Sprintf("sudo helm install test-release --set couchdbConfig.couchdb.uuid=%s --namespace %s couchdb/couchdb", uuid, testNamespace))
Copy link
Member

Choose a reason for hiding this comment

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

Do we need sudo here? e2e test fails because sudo isn't installed in the agents

@zroubalik
Copy link
Member

@26tanishabanik any update on this please? we plan to do a release in ~2 weeks.

@26tanishabanik
Copy link
Contributor Author

@26tanishabanik any update on this please? we plan to do a release in ~2 weeks.

Yes, @zroubalik , I will complete it by this weekend, have been busy with work

@zroubalik
Copy link
Member

@26tanishabanik any update on this please? we plan to do a release in ~2 weeks.

Yes, @zroubalik , I will complete it by this weekend, have been busy with work

cool, and no worries, if you are not able to make it, we will ship this in the next release.

@tomkerkhove
Copy link
Member

We are going to release KEDA v2.9 on Thursday. Do you think you can complete the open work by Wednesday @26tanishabanik?

@26tanishabanik
Copy link
Contributor Author

We are going to release KEDA v2.9 on Thursday. Do you think you can complete the open work by Wednesday @26tanishabanik?

Yes @tomkerkhove , I can complete it by weekend itself

@tomkerkhove
Copy link
Member

Great, thank you!

@tomkerkhove tomkerkhove requested a review from JorTurFer December 5, 2022 08:51
@zroubalik
Copy link
Member

@26tanishabanik could you please fix the problems in Static Checks? and also the unit test failed:

=== RUN   TestCouchDBGetMetricSpecForScaling
    couchdb_scaler_test.go:96: Wrong External metric source name: s0-s0-coucdb-animals
    couchdb_scaler_test.go:96: Wrong External metric source name: s1-s1-coucdb-animals

@zroubalik
Copy link
Member

zroubalik commented Dec 7, 2022 •
edited by github-actions bot
Loading

/run-e2e couch*
Update: You can check the progress here

@26tanishabanik
Copy link
Contributor Author

26tanishabanik commented Dec 8, 2022 •
edited
Loading

@26tanishabanik could you please fix the problems in Static Checks? and also the unit test failed:

=== RUN   TestCouchDBGetMetricSpecForScaling
    couchdb_scaler_test.go:96: Wrong External metric source name: s0-s0-coucdb-animals
    couchdb_scaler_test.go:96: Wrong External metric source name: s1-s1-coucdb-animals

The problems in static checks were related to keda version unit test and duplicate lines of code
The unit test cases, I am checking them

@26tanishabanik
Copy link
Contributor Author

Details

What should I do about the duplicate lines of code?

semgrep-app[bot]
semgrep-app bot reviewed Dec 8, 2022
View reviewed changes
pkg/scalers/couchdb_scaler.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Dec 8, 2022 •
edited by github-actions bot
Loading

/run-e2e couch*
Update: You can check the progress here

@zroubalik
Copy link
Member

@26tanishabanik there is still error in unit tests:

 RUN   TestParseCouchDBMetadata
    couchdb_scaler_test.go:80: Expected error but got success
--- FAIL: TestParseCouchDBMetadata (0.00s)

@zroubalik
Copy link
Member

@26tanishabanik also, could you please fix the PR to contain only your commits?

@26tanishabanik
Copy link
Contributor Author

@26tanishabanik also, could you please fix the PR to contain only your commits?

How can I do that @zroubalik

@zroubalik
Copy link
Member

@26tanishabanik to fix the Static Check, please add similar rule for this scaler: https://github.com/kedacore/keda/blob/cbdb405dc3f9936d100f39ee0a1c6c1f98c1c2dd/.golangci.yml

@26tanishabanik
Copy link
Contributor Author

26tanishabanik commented Dec 8, 2022 •
edited
Loading

@26tanishabanik to fix the Static Check, please add similar rule for this scaler: https://github.com/kedacore/keda/blob/cbdb405dc3f9936d100f39ee0a1c6c1f98c1c2dd/.golangci.yml

Like this:

keda/.golangci.yml

Lines 87 to 89 in cbdb405

- path: nats_jetstream_scaler.go
linters:
- dupl

@zroubalik
Copy link
Member

zroubalik commented Dec 8, 2022 •
edited
Loading

@26tanishabanik also, could you please fix the PR to contain only your commits?

How can I do that @zroubalik

@26tanishabanik this could be tricky, sometimes might be better to use this approach:

  1. backup the changes you did
  2. reset your branch: git reset --hard couchdb-scaler This removes all your changes!
  3. update branch: git pull --rebase upstream main (in case upstream points to this repo)
  4. move back your changes from the backup
  5. commit
  6. force push: git push -f origin couchdb-scaler

@zroubalik
Copy link
Member

also, .golangci.yml is not properly formatted, see the error

@26tanishabanik
Copy link
Contributor Author

Details

Let me try that @zroubalik , thank you for the suggestion

@26tanishabanik
Copy link
Contributor Author

@26tanishabanik also, could you please fix the PR to contain only your commits?

How can I do that @zroubalik

@26tanishabanik this could be tricky, sometimes might be better to use this approach:

  1. backup the changes you did
  2. reset your branch: git reset --hard couchdb-scaler This removes all your changes!
  3. update branch: git pull --rebase upstream main (in case upstream points to this repo)
  4. move back your changes from the backup
  5. commit
  6. force push: git push -f origin couchdb-scaler
keda on  couchdb-scaler via 🐹 v1.19.1 
❯ git reset --hard couchdb-scaler
HEAD is now at 56cbc982 added dupl fix for mongodb

keda on  couchdb-scaler via 🐹 v1.19.1 
❯ git pull --rebase upstream main
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@zroubalik
Copy link
Member

@26tanishabanik also, could you please fix the PR to contain only your commits?

How can I do that @zroubalik

@26tanishabanik this could be tricky, sometimes might be better to use this approach:

  1. backup the changes you did
  2. reset your branch: git reset --hard couchdb-scaler This removes all your changes!
  3. update branch: git pull --rebase upstream main (in case upstream points to this repo)
  4. move back your changes from the backup
  5. commit
  6. force push: git push -f origin couchdb-scaler
keda on  couchdb-scaler via 🐹 v1.19.1 
❯ git reset --hard couchdb-scaler
HEAD is now at 56cbc982 added dupl fix for mongodb

keda on  couchdb-scaler via 🐹 v1.19.1 
❯ git pull --rebase upstream main
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

it depends how you have named your git remotes, see that in:
git remote -v

@zroubalik
Copy link
Member

@26tanishabanik also, could you please fix the PR to contain only your commits?

How can I do that @zroubalik

@26tanishabanik this could be tricky, sometimes might be better to use this approach:

  1. backup the changes you did
  2. reset your branch: git reset --hard couchdb-scaler This removes all your changes!
  3. update branch: git pull --rebase upstream main (in case upstream points to this repo)
  4. move back your changes from the backup
  5. commit
  6. force push: git push -f origin couchdb-scaler
keda on  couchdb-scaler via 🐹 v1.19.1 
❯ git reset --hard couchdb-scaler
HEAD is now at 56cbc982 added dupl fix for mongodb

keda on  couchdb-scaler via 🐹 v1.19.1 
❯ git pull --rebase upstream main
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

it depends how you have named your git remotes, see that in: git remote -v

@26tanishabanik feel free to ping me on slack about this

@26tanishabanik
added couchdb-scaler
43a831d 
Signed-off-by: 26tanishabanik <[email protected]>
@26tanishabanik
fixed dupl for mongodb and couchdb
1f3771b 
Signed-off-by: 26tanishabanik <[email protected]>
@26tanishabanik
fixed dupl for mongodb and couchdb
533ce44 
Signed-off-by: 26tanishabanik <[email protected]>
@26tanishabanik
added new line
cf64b3d 
Signed-off-by: 26tanishabanik <[email protected]>
zroubalik
zroubalik reviewed Dec 8, 2022
View reviewed changes
.golangci.yml Outdated Show resolved Hide resolved
@zroubalik
Update .golangci.yml
22144f2 
Signed-off-by: Zbynek Roubalik <[email protected]>
@zroubalik
Update .golangci.yml
fbc4f9c 
Signed-off-by: Zbynek Roubalik <[email protected]>
@zroubalik
Copy link
Member

zroubalik commented Dec 8, 2022 •
edited by github-actions bot
Loading

/run-e2e couch*
Update: You can check the progress here

@zroubalik zroubalik merged commit 46d817b into kedacore:main Dec 8, 2022
@zroubalik zroubalik mentioned this pull request Mar 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer Awaiting requested review from JorTurFer

Assignees

@zroubalik zroubalik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@26tanishabanik @JorTurFer @zroubalik @tomkerkhove