Fix conflicting Serverless scaling options

[moelsayed]

Description

Changes proposed in this pull request:

• ...
• ...
• ...

Related issue(s)

#15152

[moelsayed]

/hold
I am still working on a defaulting bug.

[kwiatekus]

my 3 cents:

I see the benefits of the proposed idea.
I understand you tried to separate those scalability modes and distinguish which resource is scaled to avoid collisions.
But I think there are also problems.. for instance :

• if the external scaler is used than it pollutes the scaleConfig (as replicas override the scaleConfig.. or did I get it wrong?)
• the necessity to clear the replicas field in order to bring back serverless managed HPA. It would be more natural to me to clear the scaleConfig as in "I dont want you to scale it.. I will scale myself"
• two sources of truth for scaling ( replicas and scaleconifg ) could be error prone and increase maintenance costs

I like to see the spec.scaleConfig a way to help user to configure a very basic scaler.
I believe this basic scaler (HPA) should target function.. this way function.spec.replicas would become the single source of truth for replication.
If the user enable other HPAs referencing the function indeed there might be collisions of two or more scalers trying to overrule others and set the number of replicas... but he can always do that, cant he? ( i.e by applying multiple scaledObjects). Its user's responsibility to model scalers that would not conflict with each other.

[pPrecel]

In general, this proposal looks ok. I have only one small suggestion for it - I think mirroring value of the spec.replicas in the spec.scaleConfig looks a little bit tricky. As a user, I would be really confused after reading the documentation, where we write that scaleConfig is used to define internal hpa, and then after applying an external one the scaleConfig has exactly the same values as replicas (?) and this config is following values from replicas? It would be really confusing.

I would suggest the other solution to our problem:

• spec.replicas is THE ONLY ONE source of truth for any functionality in the function-controller
• spec.scaleConfig is ONLY used to create an internal scaler (and should be not used in any other way) - if spec.scaleConfig == nil, then do nothing
• Internal scalers should refer to function, not deployment

I think the huge advantage of this proposition is simplicity ( simple code == simple documentation == functionality easy to understand ).

I remember a few discussions about what if the user would use scaleConfig and external scaler at the same time? and I think this question is totally not a case because if we would make scaleConfig optional and users would enable it intentionally then it's not our fault that it would not work :D the exact same case would be when the user will use two (or more) external scalers for one function at the same time.

What do you think about it @moelsayed? I don't want to decide which proposition is better I only want to open a discussion to talk about it because I think it's really important to decide on the such thing right now before we will make a mistake.

[moelsayed]

/test pull-function-controller-unit-test

[moelsayed]

/retest

[kyma-bot]

@moelsayed: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-application-operator-unit-test	560b45a	link	true	/test pull-application-operator-unit-test
pull-central-application-connectivity-validator-unit-test	560b45a	link	true	/test pull-central-application-connectivity-validator-unit-test
pull-central-application-gateway-unit-test	560b45a	link	true	/test pull-central-application-gateway-unit-test
pull-compass-runtime-agent-unit-test	560b45a	link	true	/test pull-compass-runtime-agent-unit-test
pull-event-publisher-proxy-unit-test	560b45a	link	true	/test pull-event-publisher-proxy-unit-test
pull-directory-size-exporter-unit-test	560b45a	link	true	/test pull-directory-size-exporter-unit-test
pull-telemetry-operator-unit-test	560b45a	link	true	/test pull-telemetry-operator-unit-test
pull-eventing-controller-unit-test	560b45a	link	true	/test pull-eventing-controller-unit-test
pull-function-runtimes-nodejs14-build	f05b020	link	true	/test pull-function-runtimes-nodejs14-build
pull-function-runtimes-nodejs12-build	f05b020	link	true	/test pull-function-runtimes-nodejs12-build
pull-function-runtimes-python39-build	f05b020	link	true	/test pull-function-runtimes-python39-build
pull-function-controller-lint	9ed233f	link	false	/test pull-function-controller-lint

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.