Skip to content
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

Implement LoadBalancer Strategy based on annotation #5091

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vrann
Copy link
Contributor

@vrann vrann commented Mar 15, 2021

Fix for #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)
  • 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).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • 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.

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:

  • 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 vrann closed this Mar 16, 2021
@vrann vrann reopened this Mar 16, 2021
@codecov-io
Copy link

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%) ⬇️
.../main/scala/org/apache/openwhisk/utils/Retry.scala 0.00% <0.00%> (-100.00%) ⬇️
...n/scala/org/apache/openwhisk/utils/JsHelpers.scala 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/openwhisk/common/ConfigMapValue.scala 0.00% <0.00%> (-100.00%) ⬇️
...g/apache/openwhisk/common/ResizableSemaphore.scala 0.00% <0.00%> (-100.00%) ⬇️
...pache/openwhisk/http/LenientSprayJsonSupport.scala 0.00% <0.00%> (-100.00%) ⬇️
.../apache/openwhisk/core/controller/Namespaces.scala 0.00% <0.00%> (-100.00%) ⬇️
...pache/openwhisk/core/controller/CorsSettings.scala 0.00% <0.00%> (-100.00%) ⬇️
...che/openwhisk/core/entitlement/RateThrottler.scala 0.00% <0.00%> (-100.00%) ⬇️
... 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.

@rabbah
Copy link
Member

rabbah commented Mar 16, 2021

@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 🙏

@vrann
Copy link
Contributor Author

vrann commented Mar 16, 2021

@rabbah ah, yes, would do that

@bdoyle0182
Copy link
Contributor

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

- implemented MuxLoadBalancer to dynamically select load balancing strategy based on annotation of the action
@vrann vrann closed this Apr 20, 2021
@vrann vrann reopened this Apr 20, 2021
- switched on MuxBalancer for the standalone
@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021

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%) ⬇️
...pache/openwhisk/core/containerpool/Container.scala 83.90% <0.00%> (+1.14%) ⬆️
.../openwhisk/core/containerpool/ContainerProxy.scala 71.39% <0.00%> (+2.05%) ⬆️
.../org/apache/openwhisk/common/NestedSemaphore.scala 81.81% <0.00%> (+3.03%) ⬆️
...whisk/core/containerpool/AkkaContainerClient.scala 70.21% <0.00%> (+4.25%) ⬆️
.../scala/org/apache/openwhisk/utils/Exceptions.scala 40.00% <0.00%> (+20.00%) ⬆️
...whisk/connector/kafka/KafkaProducerConnector.scala 87.50% <0.00%> (+27.50%) ⬆️

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.

@vrann vrann closed this Apr 21, 2021
@vrann vrann reopened this Apr 21, 2021
@vrann vrann closed this Apr 23, 2021
@vrann vrann reopened this Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants