openwhisk icon indicating copy to clipboard operation
openwhisk copied to clipboard

Implement LoadBalancer Strategy based on annotation

Open vrann opened this issue 3 years ago • 5 comments

Fix for https://github.com/apache/openwhisk/issues/5097

Description

This change allows selecting custom Load Balancing strategies based on the annotation of the action. I introduced the new MuxBalancer. When action is created, a developer can specify "activationStrategy" annotation. If OpenWhisk is configured to use MuxBalancer, it will pick up the annotation value and will look in the map of the configured activation strategies for that alias. In case if the alias is found, it will delegate the publishing of the activation to the LoadBalancer corresponding to the alias. Otherwise, MuxBalancer will use the "default" Load Balancer to delegate the publication.

The configuration of the MuxBalancer looks like folloving:

whisk.loadbalancer {
    strategy {
      default = "fully.qualified.class.name.for. LoadBalancer0"
      custom = {
           "activationStrategyName1" = "fully.qualified.class.name.for. LoadBalancer1"
           "activationStrategyName2" = "fully.qualified.class.name.for. LoadBalancer2"
            ...
      }
    }
}

a developer would use it in the following way: wsk action create hello hello.js --annotation activationStrategy activationStrategyName1

Breaking change?

It is not a breaking change, because the support for activation strategy annotation would be enabled only if MuxBalancer is selected as a default Load Balancer. And only in that case, the strategy node of the configuration needs to be configured. Otherwise, system does not use it.

Related issue and scope

  • [ ] I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • [ ] API
  • [ ] Controller
  • [ ] Message Bus (e.g., Kafka)
  • [x] Loadbalancer
  • [ ] Invoker
  • [ ] Intrinsic actions (e.g., sequences, conductors)
  • [ ] Data stores (e.g., CouchDB)
  • [ ] Tests
  • [ ] Deployment
  • [ ] CLI
  • [ ] General tooling
  • [ ] Documentation

Types of changes

  • [ ] Bug fix (generally a non-breaking change which closes an issue).
  • [x] Enhancement or new feature (adds new functionality).
  • [ ] Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • [x] I signed an Apache CLA.
  • [x] I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • [x] I added tests to cover my changes.
  • [x] My changes require further changes to the documentation.
  • [ ] I updated the documentation where necessary.

anges. -->

Related issue and scope

  • [ ] I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • [ ] API
  • [ ] Controller
  • [ ] Message Bus (e.g., Kafka)
  • [ ] Loadbalancer
  • [ ] Scheduler
  • [ ] Invoker
  • [ ] Intrinsic actions (e.g., sequences, conductors)
  • [ ] Data stores (e.g., CouchDB)
  • [ ] Tests
  • [ ] Deployment
  • [ ] CLI
  • [ ] General tooling
  • [ ] Documentation

Types of changes

  • [ ] Bug fix (generally a non-breaking change which closes an issue).
  • [ ] Enhancement or new feature (adds new functionality).
  • [ ] Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • [x] I signed an Apache CLA.
  • [ ] I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • [ ] I added tests to cover my changes.
  • [ ] My changes require further changes to the documentation.
  • [ ] I updated the documentation where necessary.

vrann avatar Mar 15 '21 14:03 vrann

Codecov Report

Merging #5091 (fec63fe) into master (1753946) will decrease coverage by 53.77%. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5091       +/-   ##
===========================================
- Coverage   82.80%   29.03%   -53.78%     
===========================================
  Files         207      207               
  Lines       10034    10068       +34     
  Branches      444      449        +5     
===========================================
- Hits         8309     2923     -5386     
- Misses       1725     7145     +5420     
Impacted Files Coverage Δ
...ache/openwhisk/core/loadBalancer/MuxBalancer.scala 0.00% <0.00%> (ø)
...e/loadBalancer/ShardingContainerPoolBalancer.scala 0.00% <ø> (-80.80%) :arrow_down:
.../main/scala/org/apache/openwhisk/utils/Retry.scala 0.00% <0.00%> (-100.00%) :arrow_down:
...n/scala/org/apache/openwhisk/utils/JsHelpers.scala 0.00% <0.00%> (-100.00%) :arrow_down:
...a/org/apache/openwhisk/common/ConfigMapValue.scala 0.00% <0.00%> (-100.00%) :arrow_down:
...g/apache/openwhisk/common/ResizableSemaphore.scala 0.00% <0.00%> (-100.00%) :arrow_down:
...pache/openwhisk/http/LenientSprayJsonSupport.scala 0.00% <0.00%> (-100.00%) :arrow_down:
.../apache/openwhisk/core/controller/Namespaces.scala 0.00% <0.00%> (-100.00%) :arrow_down:
...pache/openwhisk/core/controller/CorsSettings.scala 0.00% <0.00%> (-100.00%) :arrow_down:
...che/openwhisk/core/entitlement/RateThrottler.scala 0.00% <0.00%> (-100.00%) :arrow_down:
... and 164 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1753946...fec63fe. Read the comment docs.

codecov-io avatar Mar 16 '21 13:03 codecov-io

@vrann can you move the description to an issue and then we can link the PR to the issue - this is to separate any potential discussion from the code review 🙏

rabbah avatar Mar 16 '21 13:03 rabbah

@rabbah ah, yes, would do that

vrann avatar Mar 16 '21 19:03 vrann

With the current architecture of openwhisk, the load balancers push activations to invokers without state knowledge of what's going on in each invoker. So I'm not sure based on that if this can be implemented without each load balancer stepping on the toes of one another if you're defining what load balancer to use at the action level. You would need to split up invoker pools to separate different load balancers affecting each other or share state between all load balancers.

I'm curious what changes you're working on. We've had similar desire to improve the load balancer, but with the forthcoming re-architecture of activation scheduling that's in progress; we've put off doing any improvements to the load balancer using the push model waiting for the new pull based model. I guess we can discuss this in the issue

bdoyle0182 avatar Mar 17 '21 00:03 bdoyle0182

Codecov Report

Merging #5091 (0271f1a) into master (a201e02) will increase coverage by 0.15%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5091      +/-   ##
==========================================
+ Coverage   37.25%   37.41%   +0.15%     
==========================================
  Files         211      212       +1     
  Lines       10435    10452      +17     
  Branches      450      445       -5     
==========================================
+ Hits         3888     3911      +23     
+ Misses       6547     6541       -6     
Impacted Files Coverage Δ
...ache/openwhisk/core/loadBalancer/MuxBalancer.scala 0.00% <0.00%> (ø)
...e/loadBalancer/ShardingContainerPoolBalancer.scala 0.00% <ø> (ø)
...g/apache/openwhisk/common/ResizableSemaphore.scala 80.76% <0.00%> (-7.70%) :arrow_down:
...pache/openwhisk/core/containerpool/Container.scala 83.90% <0.00%> (+1.14%) :arrow_up:
.../openwhisk/core/containerpool/ContainerProxy.scala 71.39% <0.00%> (+2.05%) :arrow_up:
.../org/apache/openwhisk/common/NestedSemaphore.scala 81.81% <0.00%> (+3.03%) :arrow_up:
...whisk/core/containerpool/AkkaContainerClient.scala 70.21% <0.00%> (+4.25%) :arrow_up:
.../scala/org/apache/openwhisk/utils/Exceptions.scala 40.00% <0.00%> (+20.00%) :arrow_up:
...whisk/connector/kafka/KafkaProducerConnector.scala 87.50% <0.00%> (+27.50%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a201e02...0271f1a. Read the comment docs.

codecov-commenter avatar Apr 21 '21 13:04 codecov-commenter