-
Notifications
You must be signed in to change notification settings - Fork 51
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.
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
feat(CustomMetrics): Allow Different App to Send Custom Metrics #3211
base: main
Are you sure you want to change the base?
Conversation
fae6122
to
9859b16
Compare
3edfa26
to
c217dd7
Compare
a4de8af
to
f63ba3c
Compare
b30a9fc
to
200be0b
Compare
31ee44d
to
2e9256a
Compare
1fb8dce
to
840fa60
Compare
fix api->broker tests fix acceptance test add some logging fix test fix db add acceptance tests fix db test by setting default to same_app custom metric strategy should be at the scaling side add some tests and refactor fix linters refactor naming
c5d8a30
to
18a84ce
Compare
• [FAILED] [0.063 seconds] BindingSqldb CreateServiceBinding When service instance exists When service binding is being created first time [It] should succeed /__w/app-autoscaler-release/app-autoscaler-release/src/autoscaler/db/sqldb/binding_sqldb_test.go:374 Timeline >> {"timestamp":"1729248292.991818190","source":"binding-sqldb-test","message":"binding-sqldb-test.Cleaning service binding test-binding-id-4 failed:doesn't exist","log_level":1,"data":{}} {"timestamp":"1729248292.992060423","source":"binding-sqldb-test","message":"binding-sqldb-test.Cleaning service binding test-binding-id-42 failed:doesn't exist","log_level":1,"data":{}} {"timestamp":"1729248292.992296696","source":"binding-sqldb-test","message":"binding-sqldb-test.Cleaning service binding test-binding-id-43 failed:doesn't exist","log_level":1,"data":{}} {"timestamp":"1729248293.024851084","source":"binding-sqldb-test","message":"binding-sqldb-test.Cleaning service instance test-instance-id-42 failed:doesn't exist","log_level":1,"data":{}} {"timestamp":"1729248293.025138617","source":"binding-sqldb-test","message":"binding-sqldb-test.Cleaning service instance test-instance-id-43 failed:doesn't exist","log_level":1,"data":{}} {"timestamp":"1729248293.026132822","source":"binding-sqldb-test","message":"binding-sqldb-test.get-service-instance","log_level":2,"data":{"error":"sql: no rows in result set","query":"SELECT * FROM service_instance WHERE service_instance_id = $1","serviceInstanceId":"test-instance-id-4"}} {"timestamp":"1729248293.028556824","source":"binding-sqldb-test","message":"binding-sqldb-test.create-service-binding","log_level":2,"data":{"appid":"test-app-id-4","bindingid":"test-binding-id-4","customMetricsStrategy":"","error":"ERROR: insert or update on table \"binding\" violates foreign key constraint \"fk_binding_custom_metrics_strategy\" (SQLSTATE 23503)","query":"INSERT INTO binding(binding_id, service_instance_id, app_id, created_at, custom_metrics_strategy) VALUES($1, $2, $3, $4,$5)","serviceinstanceid":"test-instance-id-4"}} [FAILED] in [It] - /__w/app-autoscaler-release/app-autoscaler-release/src/autoscaler/db/sqldb/binding_sqldb_test.go:375 @ 10/18/24 11:44:53.029 << Timeline [FAILED] Unexpected error: <*pgconn.PgError | 0xc0003be200>: ERROR: insert or update on table "binding" violates foreign key constraint "fk_binding_custom_metrics_strategy" (SQLSTATE 23503) { Severity: "ERROR", SeverityUnlocalized: "ERROR", Code: "23503", Message: "insert or update on table \"binding\" violates foreign key constraint \"fk_binding_custom_metrics_strategy\"", Detail: "Key (custom_metrics_strategy)=() is not present in table \"metrics_submission\".", Hint: "", Position: 0, InternalPosition: 0, InternalQuery: "", Where: "", SchemaName: "public", TableName: "binding", ColumnName: "", DataTypeName: "", ConstraintName: "fk_binding_custom_metrics_strategy", File: "ri_triggers.c", Line: 2608, Routine: "ri_ReportViolation", } occurred In [It] at: /__w/app-autoscaler-release/app-autoscaler-release/src/autoscaler/db/sqldb/binding_sqldb_test.go:375 @ 10
18a84ce
to
8e4bf83
Compare
b105a31
to
3b57893
Compare
3b57893
to
1cd4ac9
Compare
b67cb3c
to
979581a
Compare
8d27a10
to
dec3f28
Compare
@@ -103,7 +113,7 @@ func (*CustomMetricAPIClient) PostCustomMetric(ctx context.Context, appConfig *c | |||
} | |||
|
|||
metrics := createSingletonMetric(metricName, metricValue) | |||
|
|||
logger.Info("sending metric to autoscaler for app", "appId", appId, "metricName", metricName, "metricValue", metricValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: included for better logging from the test side
0eee25c
to
dec3f28
Compare
• Abstracted database configuration logic into configureDb function for reuse • Extended configuration to support BindingDb alongside PolicyDb • Updated tests to reflect additional BindingDb configuration • Added binding_db tag to mta.tpl.yaml resources
Quality Gate failedFailed conditions |
@@ -24,6 +24,10 @@ templates: | |||
policy_db.crt.erb: config/certs/policy_db/crt | |||
policy_db.key.erb: config/certs/policy_db/key | |||
|
|||
binding_db_ca.crt.erb: config/certs/binding_db/ca.crt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering we have added a new database for the metrics forwarder, we need to ensure that the MTA deployment is functioning correctly: https://github.com/cloudfoundry/app-autoscaler-release/actions/runs/11464983060/job/31903137875?pr=3211. Currently, it seems to be failing due to a different error.
We will need to add the bindingDB tag in the PostgreSQL user-provided service: https://github.com/cloudfoundry/app-autoscaler-release/blob/main/src/autoscaler/mta.tpl.yaml#L43.
Additionally, we must check the metrics forwarder configuration to ensure we are loading the database when running on Cloud Foundry as well: https://github.com/cloudfoundry/app-autoscaler-release/blob/main/src/autoscaler/metricsforwarder/config/config.go#L124.cloudfoundry/app-autoscaler-release/blob/main/src/autoscaler/metricsforwarder/config/config.go#L124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asalan316 this should fix it: a373e94
// Therefore, only mtls connection will be supported for custom metrics in future | ||
Context("when scaling by custom metrics", func() { | ||
BeforeEach(func() { | ||
//instanceName = CreatePolicy(cfg, appToScaleName, appToScaleGUID, policy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might want to remove this commented test
// Therefore, only mtls connection will be supported for custom metrics in future | ||
Context("when scaling by custom metrics", func() { | ||
BeforeEach(func() { | ||
//instanceName = CreatePolicy(cfg, appToScaleName, appToScaleGUID, policy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this file are all for logging purposes, right? The app that sends metrics should behave the same, even if it is not scaling itself.
policy, err := os.ReadFile(policyFile) | ||
fmt.Println("policy", string(policy)) //FIXME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove this ?
@@ -192,6 +223,7 @@ var _ = Describe("AutoScaler Service Broker", func() { | |||
var instance serviceInstance | |||
It("should update a service instance from one plan to another plan", func() { | |||
servicePlans := GetServicePlans(cfg) | |||
fmt.Println("servicePlans", servicePlans) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mayber we remove this too or turn it into a By statement?
testAppId = "app-to-scale-id" | ||
vars["appid"] = testAppId | ||
req.Header.Add("X-Forwarded-Client-Cert", MustReadXFCCcert(validClientCert2)) | ||
// ToDO: this should be read via configurations aka scaling policy binding parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we look into this TODO
Previously, only the application sending custom metrics to the autoscaler could be scaled.
The new feature enables users to scale an application based on custom metrics sent by another app (Custom Metrics Producer), This is achieve if:
"metric_submission_strategy.allow_from"
set tobound_app
during the binding processHow It Works
To enable this feature, specify the
metric_submission_strategy.allow_from : "bound_app"
parameter in the configuration object during the binding process with the application to be scaled (as shown below)Binding Configuration (aka.scaling policy)
Bind to Applications
Other Changes
bindingDB
in metricsforwarder service to retrieve binding informationbound_app
andsame_app
(default)