Add OpenStack Swift scaler #1462

pdominguite · 2020-12-29T20:27:30Z

Signed-off-by: Pedro Dominguite [email protected]

This PR adds a new scaler for the Swift OpenStack object-store.

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
Tests have been added
A PR is opened to update the documentation on https://github.com/kedacore/keda-docs (Add OpenStack Swift scaler documentation keda-docs#342)
Changelog has been updated

We are committed to delivering end to end test for this scaler soon.

Related to #1342.

zroubalik

Thanks for the contribution!

Looking good, just some minor remarks

zroubalik · 2021-01-04T14:46:30Z

pkg/scaling/scale_handler.go

@@ -464,6 +464,8 @@ func buildScaler(triggerType string, config *scalers.ScalerConfig) (scalers.Scal
 		return scalers.NewRedisStreamsScaler(config)
 	case "stan":
 		return scalers.NewStanScaler(config)
+	case "swift":


Suggested change

case "swift":

case "openstack-swift":

Constructor and other helper functions were named NewSwiftScaler or parseSwiftMetadata. Would it be the case to rename them to NewOpenstackSwiftScaler and parseOpenstackSwiftMetadata, for example?

Both 0313be5 and e12e140 applied the suggested changes 😃

zroubalik · 2021-01-04T14:49:52Z

pkg/scalers/openstack_swift_scaler.go

+
+	externalMetric := &v2beta2.ExternalMetricSource{
+		Metric: v2beta2.MetricIdentifier{
+			Name: kedautil.NormalizeString(fmt.Sprintf("%s-%s", "swift", s.metadata.containerName)),


The generated metricName should be unique enough, that if there are multiple scalers of the same type in one ScaledObject, each metric name is still unique.
Just double checking, because I don't have enough expertise in Openstack Swift, I am not sure whether this is the case?

Do you think that there's a sane scenario where we would like to have multiple swift triggers pointing to the same container? Or is that something, that doesn't make a sense?

OpenStack Swift should work similarly to Azure Blob Storage or any other object-store service.

We have identified one scenario where it could be possible to have multiple swift triggers pointing to the same container, though: when one wants to scale based on the number of objects inside different folders within the same container. In this case, delimiter and prefixes can be used. In order to set more than one prefix, new triggers for the same container in the same ScaledObject must be created.

Thus, we have modified metricName to contain the containerName + objectPrefix, when the latter is provided. That should be enough to cover this scenario.

Sounds good, thanks!

zroubalik · 2021-01-04T14:52:57Z

pkg/scalers/openstack_swift_scaler.go

+	objectPrefix      string
+	objectDelimiter   string
+	objectLimit       string
+	httpClientTimeout int


For handling the http, could you please take a look on this PR: #1251

there's kedautil.CreateHTTPClient(timeout time.Duration) function.

Done by 07fc5e4.

zroubalik

LGTM, thanks @pdominguite!

What is your plan for e2e tests? We would like to see one for this scaler.

pdominguite · 2021-01-05T17:29:29Z

Thanks for all feedbacks @zroubalik! We are very happy to contribute to Keda 😄

Regarding e2e tests: end-to-end tests are a must-have to add this contribution or the swift scaler can be merged and tests provided later on?

If the merge can happen without tests, we plan to submit them in a separate PR by the end of this month/beginning of February max. Is that ok?

ahmelsayed · 2021-01-05T22:35:17Z

pkg/scalers/openstack/keystone_authentication.go

+	passAuth := new(KeystoneAuthMetadata)
+
+	passAuth.Properties = new(authProps)
+
+	passAuth.Properties.Scope = new(scopeProps)
+	passAuth.Properties.Scope.Project = new(projectProps)
+
+	passAuth.Properties.Identity = new(identityProps)
+	passAuth.Properties.Identity.Password = new(passwordProps)
+	passAuth.Properties.Identity.Password.User = new(userProps)


it seems to me that you don't need to make passAuth, passAuth.Properties, passAuth.Properties.Identity, passAuth.Properties.Identity.Password, passAuth.Properties.Identity.Password.User, or passAuth.Properties.Scope references, and then you'll get a default object for all of them just by doing

passAuth := KeystoneAuthMetadata{}

right?

As far as I know (and tried), since KeystoneAuthMetadata is a struct that has other structs inside that are pointers, they need to be initialized somehow first. Thus,

passAuth := KeystoneAuthMetadata{}

is not sufficient to get a proper KeystoneAuthMetadata default object. Assigning values to it without manually making those references will result in:

runtime error: invalid memory address or nil pointer dereference

Maybe there is a more elegant way to do or another way to initialize these "inner structs", but my limited experience in Go does not allow me to find it out 😅

I guess what I meant is maybe non of these pointers need to be a pointer since you never expect them to actually be nil, right? So KeystoreAuthMetadata would be

type KeystoneAuthMetadata struct { AuthURL string `json:"-"` AuthToken string `json:"-"` HTTPClient *http.Client `json:"-"` - Properties *authProps `json:"auth"` + Properties authProps `json:"auth"` }

and in turn authProps would be

type authProps struct { - Identity *identityProps `json:"identity"` + Identity identityProps `json:"identity"` - Scope *scopeProps `json:"scope,omitempty"` + Scope scopeProps `json:"scope,omitempty"` }

and so on.

I think the only difference is in the json.Marshal() behavior where a nil is an empty object but any non-nil object is not empty, but I don't think it makes a difference really. Just thought it would make the code a bit easier to read since you're already initializing all the pointers with new() anyway. No worries if it gives you trouble.

Exactly! They do not need to be pointers. The only reason we did that way was because of json.Marshal() as you noticed. Since we use these structs to build a JSON payload for our token request, some of them need to be omitted (and thus be pointers, and then be nil) in order to make it work properly.

Also, we plan to submit new scalers for OpenStack projects soon. As they come, we can study better ways to do it and, eventually, updated the way we originally structured this.

That is nice feedback, anyway @ahmelsayed. Thanks! We'll keep an eye on it 😄

ahmelsayed · 2021-01-05T22:36:54Z

pkg/scalers/openstack/keystone_authentication.go

+		return resp.Header["X-Subject-Token"][0], nil
+	}
+
+	errBody, _ := ioutil.ReadAll(resp.Body)


nit: since this is an error case, maybe handle the err in case the stream is terminated from the server early.

Sorry, I missed this error handling. c6d9f14 added the basic handler.

ahmelsayed · 2021-01-05T22:39:40Z

I think an e2e test can come in a separate PR to not block this one on it.

Signed-off-by: Pedro Felipe Dominguite <[email protected]> * Add Swift scaler unit tests Signed-off-by: Pedro Felipe Dominguite <[email protected]> * Change scaler name to openstack-swift Signed-off-by: Pedro Felipe Dominguite <[email protected]> * Change scaler name references to openstackSwift * Add objectPrefix to metricName Signed-off-by: Pedro Felipe Dominguite <[email protected]> * Use kedautil for handling http Signed-off-by: Pedro Felipe Dominguite <[email protected]> * Add read stream error handler Signed-off-by: Pedro Felipe Dominguite <[email protected]> * Update CHANGELOG.md Signed-off-by: Pedro Felipe Dominguite <[email protected]>

pdominguite requested review from ahmelsayed and zroubalik as code owners December 29, 2020 20:27

pdominguite mentioned this pull request Dec 29, 2020

Add OpenStack Swift scaler documentation kedacore/keda-docs#342

Merged

zroubalik changed the title ~~Add swift scaler~~ Add OpenStack Swift scaler Jan 4, 2021

zroubalik reviewed Jan 4, 2021

View reviewed changes

pdominguite requested a review from zroubalik January 4, 2021 21:04

zroubalik approved these changes Jan 5, 2021

View reviewed changes

ahmelsayed reviewed Jan 5, 2021

View reviewed changes

pdominguite force-pushed the add_swift_scaler branch from 07fc5e4 to b7799cb Compare January 6, 2021 19:32

pdominguite force-pushed the add_swift_scaler branch from b7799cb to 67d088d Compare January 7, 2021 14:54

zroubalik merged commit 3ff035a into kedacore:main Jan 8, 2021

pdominguite mentioned this pull request Jan 21, 2021

Add OpenStack Swift scaler e2e tests #1522

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenStack Swift scaler #1462

Add OpenStack Swift scaler #1462

pdominguite commented Dec 29, 2020 •

edited

Loading

zroubalik left a comment •

edited

Loading

zroubalik Jan 4, 2021

pdominguite Jan 4, 2021 •

edited

Loading

zroubalik Jan 4, 2021

pdominguite Jan 4, 2021

zroubalik Jan 4, 2021

pdominguite Jan 4, 2021 •

edited

Loading

zroubalik Jan 5, 2021

zroubalik Jan 4, 2021

pdominguite Jan 4, 2021

zroubalik left a comment

pdominguite commented Jan 5, 2021 •

edited

Loading

ahmelsayed Jan 5, 2021

pdominguite Jan 6, 2021

ahmelsayed Jan 6, 2021

pdominguite Jan 7, 2021

ahmelsayed Jan 5, 2021

pdominguite Jan 6, 2021

ahmelsayed commented Jan 5, 2021

Add OpenStack Swift scaler #1462

Add OpenStack Swift scaler #1462

Conversation

pdominguite commented Dec 29, 2020 • edited Loading

Checklist

zroubalik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdominguite Jan 4, 2021 • edited Loading

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

pdominguite Jan 4, 2021 • edited Loading

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

zroubalik left a comment

Choose a reason for hiding this comment

pdominguite commented Jan 5, 2021 • edited Loading

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

ahmelsayed commented Jan 5, 2021

pdominguite commented Dec 29, 2020 •

edited

Loading

zroubalik left a comment •

edited

Loading

pdominguite Jan 4, 2021 •

edited

Loading

pdominguite Jan 4, 2021 •

edited

Loading

pdominguite commented Jan 5, 2021 •

edited

Loading