Added targetMetric field in CRD #168

ajanth97 · 2021-05-18T17:30:51Z

Provide a description of what has been changed
I added the target metric field in the CRD.

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
Any necessary documentation is added, such as:

Fixes #

arschles

@ajanth97 thanks for this! two requests

arschles · 2021-05-18T17:44:29Z

go.mod

@@ -3,8 +3,14 @@ module github.com/kedacore/http-add-on
 go 1.16

 require (
+ github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6 // indirect


@ajanth97 I'm not seeing why all these new dependencies need to be added. could you try running go mod tidy? it should get rid of these

Yes, that fixed it

arschles · 2021-05-18T17:47:15Z

scaler/handlers.go

@@ -60,15 +63,30 @@ func (e *impl) StreamIsActive(
 }
 }

+func getTargetMetric() int64 {
+ targetMetricStr, err := env.Get("KEDA_HTTP_SCALER_TARGET_METRIC")


@ajanth97 can you read this environment variable in the main function near where all the other configs are read, and then pass the value into this function?

I made new changes, please check it

tomkerkhove · 2021-05-18T17:52:18Z

Don't forget to update the Helm chart as well please on kedacore/charts

arschles

@ajanth97 can you remove the mage binary from this PR? As a sidenote, you might want to consider putting it somewhere in your PATH so you can use it anywhere

arschles · 2021-05-19T18:04:58Z

go.mod

@@ -17,6 +17,7 @@ require (
 google.golang.org/grpc v1.33.2
 google.golang.org/protobuf v1.26.0
 k8s.io/api v0.20.4
+ k8s.io/apiextensions-apiserver v0.20.2 // indirect


@ajanth97 does this still show up in the go.mod after you run this command:

go mod tidy

?

Yes I removed the mage binary and added it to my PATH. And yes the go.mod file still shows those depenedencies

arschles · 2021-05-19T18:07:20Z

operator/config/rbac/role.yaml

@@ -4,7 +4,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
 creationTimestamp: null
- name: keda-http-addon-manager-role
+ name: keda-http-manager-role


@ajanth97 did the mage generate command change this?

I used mage dockerBuildAll, I am not really sure what caused this change

This requires kedacore/http-add-on#168 Signed-off-by: Aaron Schlesinger <[email protected]>

arschles · 2021-05-25T20:12:51Z

Corresponding chart change in kedacore/charts#153

This requires kedacore/http-add-on#168 Signed-off-by: Aaron Schlesinger <[email protected]>

arschles · 2021-06-15T21:24:51Z

@ajanth97 this looks good! I want to hold on this until the 0.1.0 release, which should be very soon. stand by!

arschles · 2021-06-15T21:25:14Z

feel free to resolve conflicts in the meantime though

ajanth97 · 2021-06-16T11:21:33Z

Sure

arschles · 2021-06-24T22:37:00Z

@ajanth97 this is ready to merge. can you resolve conflicts?

ajanth97 · 2021-06-25T05:03:48Z

@ajanth97 this is ready to merge. can you resolve conflicts?

Thanks, Sure

khaosdoctor

Amazing work. I just got confused on the name targetMetric, maybe this will generate some confusion as it is a bit generic.

Maybe scaleRequestAmount (I'm terrible at naming)

khaosdoctor · 2021-06-30T23:10:10Z

charts/keda-http-operator/crds/http.keda.sh_httpscaledobjects.yaml

@@ -85,6 +93,10 @@ spec:
 - port
 - service
 type: object
+ targetMetric:
+ description: (optional) Target metric value


Could you add a bit more explanation here? I think people will get lost on what this is for

Thank you, Yeah sure I will add some explanation. As for the naming yeah lets change it if its causing confusion. However I feel scaleRequestAmount is a bit counterintuitive since the lower this value the faster it scales. Let me know your thoughts

What about queueMinScaleSize?

That makes more sense but is a bit too long, How about targetQueueSize ?

The thing about the target is that it seems that it'll only scale once it hits this exact number. What about scaleReqCount?

Frankly, if you do not know how it works using queue is very confusing as you are using HTTP traffic and not a queue.

When scaling HTTP, they don't care about the inner workings - They just want it to scale to meet the demand.

But if @zroubalik likes the name as well then that's fine for me.

@tomkerkhove how about targetPendingRequests then? I agree with you RE the queue nomenclature, but I think if a user wants to tweak this, then they will be interested enough in inner workings to learn about how the Addon treats pending requests.

@tomkerkhove how about targetPendingRequests then? I agree with you RE the queue nomenclature, but I think if a user wants to tweak this, then they will be interested enough in inner workings to learn about how the Addon treats pending requests.

I think you might be surprised about how many people will (want) to define their own number, without fully understanding/caring how it works internally.

Name is fine for me.

I think you might be surprised about how many people will (want) to define their own number, without fully understanding/caring how it works internally.

if they do want to do so, and don't understand/care how it works internally, we'll have good documentation (which I'll submit in a follow-up to this PR, see #230) to explain it

arschles

This is good with me, but I'd like to hear from you @tomkerkhove about the name. this PR currently is using targetPendingRequests

arschles · 2021-08-18T19:47:06Z

RELEASE-PROCESS 2.md

@@ -0,0 +1,43 @@
+# Release Process


@ajanth97 can you remove this document? it appears to be a duplicate of the release process document

arschles · 2021-08-18T19:47:21Z

docs/developing 2.md

@@ -0,0 +1,163 @@
+# Developing


@ajanth97 same here regarding the duplicate document

arschles · 2021-08-18T20:19:07Z

operator/api/v1alpha1/httpscaledobject_types.go

@@ -115,6 +115,8 @@ type HTTPScaledObjectSpec struct {
 // (optional) Replica information
 //+optional
 Replicas ReplicaStruct `json:"replicas,omitempty"`
+ //(optional) Target metric value
+ TargetMetric int32 `json:"targetMetric,omitempty" description:"The target metric value for the HPA (Default 100)"`


@ajanth97 you'll need to change this to TargetPendingRequests as well, and then re-generate the CRD

Signed-off-by: Ajanth <[email protected]>

KEDA was accepted as CNCF Incubation project as per cncf/toc#622 Signed-off-by: Tom Kerkhove <[email protected]> Signed-off-by: Ajanth <[email protected]>

Signed-off-by: Ajanth <[email protected]>

ajanth97 · 2021-08-18T21:25:02Z

Thank you @arschles for helping out with this PR

* Forgot to sign last commit Signed-off-by: Ajanth <[email protected]>

ajanth97 requested review from ahmelsayed, arschles, tomkerkhove and zroubalik as code owners May 18, 2021 17:30

arschles requested changes May 18, 2021

View reviewed changes

ajanth97 requested a review from arschles May 19, 2021 14:25

arschles requested changes May 19, 2021

View reviewed changes

arschles added a commit to arschles/kedacharts that referenced this pull request May 25, 2021

HTTP Addon: Adding CRD fields for target metric configuration

4160b0f

This requires kedacore/http-add-on#168 Signed-off-by: Aaron Schlesinger <[email protected]>

arschles mentioned this pull request May 25, 2021

HTTP Addon: Adding CRD fields for target metric configuration kedacore/charts#153

Merged

2 tasks

tomkerkhove pushed a commit to kedacore/charts that referenced this pull request May 26, 2021

HTTP Addon: Adding CRD fields for target metric configuration (#153)

93e6366

This requires kedacore/http-add-on#168 Signed-off-by: Aaron Schlesinger <[email protected]>

arschles added the on hold label Jun 15, 2021

khaosdoctor suggested changes Jun 30, 2021

View reviewed changes

arschles mentioned this pull request Aug 16, 2021

Fixed scale to and from 0 issue #223

Closed

arschles reviewed Aug 18, 2021

View reviewed changes

arschles enabled auto-merge (squash) August 18, 2021 16:48

auto-merge was automatically disabled August 18, 2021 19:32
Head branch was pushed to by a user without write access

ajanth97 force-pushed the target-metric_crd branch from ddb3f4b to dc3fe95 Compare August 18, 2021 19:32

arschles enabled auto-merge (squash) August 18, 2021 19:46

arschles requested changes Aug 18, 2021

View reviewed changes

arschles mentioned this pull request Aug 18, 2021

Documentation for target metric #230

Closed

auto-merge was automatically disabled August 18, 2021 19:54
Head branch was pushed to by a user without write access

arschles requested changes Aug 18, 2021

View reviewed changes

arschles approved these changes Aug 18, 2021

View reviewed changes

ajanth97 and others added 10 commits August 19, 2021 02:41

Forgot to sign last commit

62d4f7f

Signed-off-by: Ajanth <[email protected]>

chore: KEDA was accepted as CNCF Incubation project (kedacore#229)

efde18a

KEDA was accepted as CNCF Incubation project as per cncf/toc#622 Signed-off-by: Tom Kerkhove <[email protected]> Signed-off-by: Ajanth <[email protected]>

Duplicate Release Process

7778495

Signed-off-by: Ajanth <[email protected]>

Duplicate

2a8ada7

Signed-off-by: Ajanth <[email protected]>

duplicate

885d2ef

Signed-off-by: Ajanth <[email protected]>

Delete developing 2.md

6b519f7

Signed-off-by: Ajanth <[email protected]>

func GetInt64Or isn't used so deleting it

4a3ffdb

Signed-off-by: Ajanth <[email protected]>

Changing to TargetPendingRequests

96e12f7

Signed-off-by: Ajanth <[email protected]>

Update external_scaler.go

f9ae92d

Signed-off-by: Ajanth <[email protected]>

Update http.keda.sh_httpscaledobjects.yaml

bb65cb7

Signed-off-by: Ajanth <[email protected]>

ajanth97 force-pushed the target-metric_crd branch from 2df285b to bb65cb7 Compare August 18, 2021 21:11

arschles enabled auto-merge (squash) August 18, 2021 21:12

Merge branch 'main' into target-metric_crd

6e70941

arschles merged commit 2a88fcd into kedacore:main Aug 18, 2021

arschles pushed a commit to arschles/http-add-on that referenced this pull request Aug 18, 2021

Added targetMetric field in CRD (kedacore#168)

b82dd44

* Forgot to sign last commit Signed-off-by: Ajanth <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added targetMetric field in CRD #168

Added targetMetric field in CRD #168

ajanth97 commented May 18, 2021 •

edited by arschles

Loading

arschles left a comment

arschles May 18, 2021

ajanth97 May 18, 2021

arschles May 18, 2021

ajanth97 May 19, 2021

tomkerkhove commented May 18, 2021

arschles left a comment

arschles May 19, 2021

ajanth97 May 20, 2021

arschles May 19, 2021

ajanth97 May 20, 2021 •

edited

Loading

arschles commented May 25, 2021

arschles commented Jun 15, 2021

arschles commented Jun 15, 2021

ajanth97 commented Jun 16, 2021

arschles commented Jun 24, 2021

ajanth97 commented Jun 25, 2021

khaosdoctor left a comment

khaosdoctor Jun 30, 2021

ajanth97 Jul 1, 2021

khaosdoctor Jul 2, 2021

ajanth97 Jul 2, 2021

khaosdoctor Jul 5, 2021

tomkerkhove Aug 17, 2021

tomkerkhove Aug 17, 2021

arschles Aug 18, 2021

tomkerkhove Aug 18, 2021

arschles Aug 18, 2021 •

edited

Loading

arschles left a comment

arschles Aug 18, 2021

arschles Aug 18, 2021

arschles Aug 18, 2021

ajanth97 commented Aug 18, 2021

Added targetMetric field in CRD #168

Added targetMetric field in CRD #168

Conversation

ajanth97 commented May 18, 2021 • edited by arschles Loading

Checklist

arschles 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

tomkerkhove commented May 18, 2021

arschles 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

ajanth97 May 20, 2021 • edited Loading

Choose a reason for hiding this comment

arschles commented May 25, 2021

arschles commented Jun 15, 2021

arschles commented Jun 15, 2021

ajanth97 commented Jun 16, 2021

arschles commented Jun 24, 2021

ajanth97 commented Jun 25, 2021

khaosdoctor 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

arschles Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

arschles 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

ajanth97 commented Aug 18, 2021

ajanth97 commented May 18, 2021 •

edited by arschles

Loading

ajanth97 May 20, 2021 •

edited

Loading

arschles Aug 18, 2021 •

edited

Loading