You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2022/07/12 03:04:08 UTC

[GitHub] [openwhisk] hunhoffe opened a new pull request, #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

hunhoffe opened a new pull request, #5278:
URL: https://github.com/apache/openwhisk/pull/5278

   ## Description
   
   I am working on integrating the new scheduler into openwhisk-deploy-kube (see this draft PR [here](https://github.com/apache/openwhisk-deploy-kube/pull/729)). While most of the changes that need to be made are within the apache/openwhisk-deploy-kube repo, some changes need to be made here for full support.
   
   Namely:
   * Scheduler images need to be build and pushed to dockerhub
   * The scheduler and controller both need to option to use the akka cluster discovery in the case that they are replicated; the existing mechanism of using seedNodes is not a great fit for Kubernetes deployments
   
   I tested locally (mostly using Kubernetes deployments) but I'm not sure the best way to test the tooling and changes to travis/CI. 
   
   ## My changes affect the following components
   <!--- Select below all system components are affected by your change. -->
   <!--- Enter an `x` in all applicable boxes. -->
   - [ ] API
   - [x] Controller
   - [ ] Message Bus (e.g., Kafka)
   - [ ] Loadbalancer
   - [x] Scheduler
   - [ ] Invoker
   - [ ] Intrinsic actions (e.g., sequences, conductors)
   - [ ] Data stores (e.g., CouchDB)
   - [ ] Tests
   - [x] Deployment
   - [x] CLI
   - [x] General tooling
   - [ ] Documentation
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
   - [ ] 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:
   <!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
   
   - [x] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
   - [x] I reviewed the [style guides](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md#coding-standards) 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.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r929011333


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   One more thought - the ShardingContainerPoolBalancer does not fail if a cluster is not created properly, so I took that approach here too.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r928360615


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   The `ShardingContaienrPoolBalancer` considers both seed-nodes-based and discovery-based approaches.
   It looks this is to support both ansible-based deployment and deployment on K8S.
   https://github.com/apache/openwhisk/pull/3548
   
   I believe discovery would work with Ansible too but it would be great to keep options for those who are using seed nodes.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] codecov-commenter commented on pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#issuecomment-1182787632

   # [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5278?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#5278](https://codecov.io/gh/apache/openwhisk/pull/5278?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9d025b5) into [master](https://codecov.io/gh/apache/openwhisk/commit/8d31e96e1a321987f9f5c3b7289ba83bf4eebff6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8d31e96) will **decrease** coverage by `17.64%`.
   > The diff coverage is `12.50%`.
   
   ```diff
   @@             Coverage Diff             @@
   ##           master    #5278       +/-   ##
   ===========================================
   - Coverage   80.08%   62.44%   -17.65%     
   ===========================================
     Files         238      231        -7     
     Lines       14122    13866      -256     
     Branches      589      578       -11     
   ===========================================
   - Hits        11310     8658     -2652     
   - Misses       2812     5208     +2396     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/5278?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...rg/apache/openwhisk/core/scheduler/Scheduler.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvU2NoZWR1bGVyLnNjYWxh) | `0.65% <0.00%> (-9.42%)` | :arrow_down: |
   | [.../apache/openwhisk/core/controller/Controller.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9Db250cm9sbGVyLnNjYWxh) | `56.07% <25.00%> (-1.21%)` | :arrow_down: |
   | [...che/openwhisk/core/invoker/LogStoreCollector.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvaW52b2tlci9Mb2dTdG9yZUNvbGxlY3Rvci5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...nwhisk/core/scheduler/queue/ContainerCounter.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvcXVldWUvQ29udGFpbmVyQ291bnRlci5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...hisk/core/scheduler/message/ContainerMessage.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvbWVzc2FnZS9Db250YWluZXJNZXNzYWdlLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...core/database/cosmosdb/RxObservableImplicits.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvUnhPYnNlcnZhYmxlSW1wbGljaXRzLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ore/database/cosmosdb/cache/CacheInvalidator.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NhY2hlSW52YWxpZGF0b3Iuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...e/database/cosmosdb/cache/ChangeFeedConsumer.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NoYW5nZUZlZWRDb25zdW1lci5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...core/scheduler/queue/SchedulingDecisionMaker.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvcXVldWUvU2NoZWR1bGluZ0RlY2lzaW9uTWFrZXIuc2NhbGE=) | `0.00% <0.00%> (-98.80%)` | :arrow_down: |
   | [...sk/core/scheduler/container/ContainerManager.scala](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvY29udGFpbmVyL0NvbnRhaW5lck1hbmFnZXIuc2NhbGE=) | `0.00% <0.00%> (-96.90%)` | :arrow_down: |
   | ... and [71 more](https://codecov.io/gh/apache/openwhisk/pull/5278/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/openwhisk/pull/5278?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5278?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8d31e96...9d025b5](https://codecov.io/gh/apache/openwhisk/pull/5278?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926218313


##########
core/scheduler/build.gradle:
##########
@@ -66,6 +66,9 @@ dependencies {
 
     compile "org.scala-lang:scala-library:${gradle.scala.version}"
     compile "io.altoo:akka-kryo-serialization_${gradle.scala.depVersion}:1.0.0"
+    compile "com.lightbend.akka.management:akka-management-cluster-http_${gradle.scala.depVersion}:${gradle.akka_management.version}" 

Review Comment:
   @style95 , have no idea why we don't add this 
   ```
   compile "com.lightbend.akka.management:akka-management-cluster-http_${gradle.scala.depVersion}:${gradle.akka_management.version}"
   ```
   @hunhoffe , why do you add this? which code would use this lib?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r928984692


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   I believe my proposed changes work just fine with seed nodes (in the single-replica controller and scheduler deployments with Kubernetes, I use a single seed node and not cluster discovery to save time on component startup). However, I believe the loadConfigOrThrow proposed above also works with seed nodes set as environment variables - so both solutions should work well with ansible and seed nodes.
   
   The only different for adding the logic pointed out by @ningyougang (e.g., code snippet in my last comment) is that it would fail if useCluster flag is not specified AND seed nodes are not specified. Currently, if neither is specified, there is no explicit failure, a cluster is just not created properly... but the controller (and scheduler) still appear to run without issue in that case.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926221471


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   As far as i know, ShardingContainerPoolBalancer already has relative logic.
   ![image](https://user-images.githubusercontent.com/11749867/180123883-40e882ea-9c7f-4d1f-aa05-82c35ff52217.png)
   Since ShardingContainerPoolBalancer's codes and these codes locates in same place, does it repeat?
   
   Currently, we have ShardingContainerBalancer and FPCPoolBalancer, seems can delete the Sharding one, just keep here?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r933764471


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {

Review Comment:
   AFAIK, `FPCPoolBalancer` does not need to discover different actors in a remote node.
   The scheduler does. `ShardingPoolBalancer` needs a cluster to determine the number of controller nodes.
   This is for sharding and `ShardingPoolBalancer` already has the same logic.
   
   That's why I asked if akka-cluster bootstrap is required for all load balancers other than `ShardingPoolBalancer`.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926221471


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   As far as i know, ShardingContainerPoolBalancer already has relative logic.
   ![image](https://user-images.githubusercontent.com/11749867/180123883-40e882ea-9c7f-4d1f-aa05-82c35ff52217.png)
   Since ShardingContainerPoolBalancer's codes and these codes locates in same place, is it repeat?
   
   Currently, we have ShardingContainerBalancer and FPCPoolBalancer, seems can delete the Sharding one, just keep here?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r928349153


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   I am not sure, wWould love to hear other comments for this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r929000054


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {

Review Comment:
   The controller has an actor system now so if you replicate the controller, it needs to know how to discover different actors in the ```controller-actor-system```. A reason I assumed this is needed is that ansible does configure a controller cluster (e.g., specified here: https://github.com/apache/openwhisk/blob/9bc5e043444d115cec0d79ac47d7002baa522924/ansible/controller.yml#L45). My goal in this PR was to enable a Kubernetes deployment to function as similarly to the ansible deployment as possible.
   
   Before the new scheduler, the controller did not have an actor system at all (other than ```ShardingContainerPoolBalancer``` which has similar logic to what I added). If you don't support replicating the controller, this is not needed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#issuecomment-1191649341

   Rebased to master so I can make sure new changes integrate well into openwhisk-deploy-kube, also - I did confirm my ICLA is good.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#issuecomment-1183196401

   Apologies for making the PR live - then draft - then live again, but I believe it is in a good state now and ready for review!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] dgrove-oss commented on pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
dgrove-oss commented on PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#issuecomment-1185530545

   ok, cool.  That's fine then.  The tool for looking up ICLAs changed recently. Now you have to search by email address instead of by name and I haven't figured out how to do fuzzy searches yet.  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r961183630


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {

Review Comment:
   I had been concerned about multiple actors with the same name contacting the scheduler, but after further exploration, I do not think it was a valid concern.
   
   I've removed the changes to the controller. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#issuecomment-1234919518

   Rebased to master, finished addressing comments about changes to controller (by removing changes to the controller)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926919483


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   @ningyougang, I believe the ShardingContainerPoolBalancer only calls Cluster(actorSystem) because it needs access to the cluster object in order to manage cluster state, as part of the main functionality of that class. Since the Scheduler and Controller are not performing cluster management tasks, I do not think they need to make this call (and thus do not need the check for seed nodes). 
   
   If we wanted to do a check just to make sure configuration is correct, I could add that (e.g.,
   ```scala
   ...
   } else {
      /* create error if seed nodes not set */
     loadConfigOrThrow[Seq[String]]("akka.cluster.seed-nodes")
   }
   ```
   
   What do you think?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r962411907


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   > I believe my proposed changes work just fine with seed nodes (in the single-replica controller and scheduler deployments with Kubernetes
   
   This is where I feel we are not on the same page.
   In my downstream, we are running controllers and schedulers with more than 1 replica without any issue.
   But anyway, I don't want to impede this PR from being merged.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
style95 commented on PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#issuecomment-1236464179

   Thank you, Erika, for your effort. 👍 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 merged pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
style95 merged PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926867150


##########
core/scheduler/build.gradle:
##########
@@ -66,6 +66,9 @@ dependencies {
 
     compile "org.scala-lang:scala-library:${gradle.scala.version}"
     compile "io.altoo:akka-kryo-serialization_${gradle.scala.depVersion}:1.0.0"
+    compile "com.lightbend.akka.management:akka-management-cluster-http_${gradle.scala.depVersion}:${gradle.akka_management.version}" 

Review Comment:
   @ningyougang thank you, you are correct, akka-management-cluster-http is not needed although the other two are. I've removed in recent commit.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926919483


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   @ningyougang, I believe the ```ShardingContainerPoolBalancer``` only calls ```Cluster(actorSystem)``` because it needs access to the cluster object in order to manage cluster state, as part of the main functionality of that class. Since the Scheduler and Controller are not performing cluster management tasks, I do not think they need to make this call (and thus do not need the check for seed nodes). 
   
   If we wanted to do a check just to make sure configuration is correct, I could add that (e.g.,
   ```scala
   ...
   } else {
      /* create error if seed nodes not set */
     loadConfigOrThrow[Seq[String]]("akka.cluster.seed-nodes")
   }
   ```
   
   What do you think?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r928984692


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   I believe my proposed changes work just fine with seed nodes (in the single-replica controller and scheduler deployments with Kubernetes, I use a single seed node and not cluster discovery to save time on component startup). However, I believe the loadConfigOrThrow proposed above also works with seed nodes set as environment variables - so both solutions should work well with ansible and seed nodes.
   
   The only difference for adding the logic pointed out by @ningyougang (e.g., code snippet in my last comment) is that it would fail if useCluster flag is not specified AND seed nodes are not specified. Currently, if neither is specified, there is no explicit failure, a cluster is just not created properly... but the controller (and scheduler) still appear to run without issue in that case. I am not sure if that is intended behavior or not.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r929000054


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {

Review Comment:
   The controller has an actor system now so if you replicate the controller, it needs to know how to discover different actors in the ```controller-actor-system```. Another reason I assumed this is needed is that ansible does configure a controller cluster (e.g., specified here: https://github.com/apache/openwhisk/blob/9bc5e043444d115cec0d79ac47d7002baa522924/ansible/controller.yml#L45). 
   
   Before the new scheduler, the controller did not have an actor system at all (other than ```ShardingContaienrPoolBalancer``` which has similar logic to what I added).
   
   To be fair, if you don't replicate the controller, this is not needed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r929000054


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {

Review Comment:
   The controller has an actor system now so if you replicate the controller, it needs to know how to discover different actors in the ```controller-actor-system```. Another reason I assumed this is needed is that ansible does configure a controller cluster (e.g., specified here: https://github.com/apache/openwhisk/blob/9bc5e043444d115cec0d79ac47d7002baa522924/ansible/controller.yml#L45). 
   
   Before the new scheduler, the controller did not have an actor system at all (other than ```ShardingContainerPoolBalancer``` which has similar logic to what I added).
   
   To be fair, if you don't replicate the controller, this is not needed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926222535


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   Refer to the sharding place, maybe we can make it stronger?
   ![image](https://user-images.githubusercontent.com/11749867/180124549-33228c6b-67a3-4e28-9a8c-3cb7b6428263.png)
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926867150


##########
core/scheduler/build.gradle:
##########
@@ -66,6 +66,9 @@ dependencies {
 
     compile "org.scala-lang:scala-library:${gradle.scala.version}"
     compile "io.altoo:akka-kryo-serialization_${gradle.scala.depVersion}:1.0.0"
+    compile "com.lightbend.akka.management:akka-management-cluster-http_${gradle.scala.depVersion}:${gradle.akka_management.version}" 

Review Comment:
   @ningyougang thank you, you are correct, akka-management-cluster-http is not needed although I believe the other two are. I've removed in recent commit.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r928361395


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {

Review Comment:
   IIRC, the `FPCPoolBalancer` doesn't need any Akka-cluster feature.
   Is there any reason to add this to the controller?
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r929011333


##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/Scheduler.scala:
##########
@@ -289,6 +292,11 @@ object Scheduler {
 
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
 
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   One more thought - the ShardingContainerPoolBalancer does not fail if a cluster is not created properly, so I took that approach here too as I was trying to keep my changes minimal. I'm also not sure how well forcing akka cluster creation works with the standalone deployment of OpenWhisk.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r921016098


##########
core/scheduler/build.gradle:
##########
@@ -66,6 +66,9 @@ dependencies {
 
     compile "org.scala-lang:scala-library:${gradle.scala.version}"
     compile "io.altoo:akka-kryo-serialization_${gradle.scala.depVersion}:1.0.0"
+    compile "com.lightbend.akka.management:akka-management-cluster-http_${gradle.scala.depVersion}:${gradle.akka_management.version}" 

Review Comment:
   IIRC, our downstream is not using these libraries while we are running OW on Kubernetes.
   @ningyougang Do you have any idea about the difference between downstream and upstream?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#issuecomment-1185105956

   @dgrove-oss I did submit an ICLA in the past, so I should be good to go. To be on the safe side and check for clerical errors, I have sent an email asking for confirmation that my form is on file.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] dgrove-oss commented on pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
dgrove-oss commented on PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#issuecomment-1184778580

   Hi Erika, have you filed an Individual Contributors License Agreement with Apache (https://www.apache.org/licenses/contributor-agreements.html#clas)?  I did a quick search and I don't think I found one for you, but the tooling isn't always perfect.
   
   If you haven't done so already please go ahead and do that.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r922179250


##########
core/scheduler/build.gradle:
##########
@@ -66,6 +66,9 @@ dependencies {
 
     compile "org.scala-lang:scala-library:${gradle.scala.version}"
     compile "io.altoo:akka-kryo-serialization_${gradle.scala.depVersion}:1.0.0"
+    compile "com.lightbend.akka.management:akka-management-cluster-http_${gradle.scala.depVersion}:${gradle.akka_management.version}" 

Review Comment:
   If there's a different way to do this, I am happy to hear it and chance it if need be!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r921016098


##########
core/scheduler/build.gradle:
##########
@@ -66,6 +66,9 @@ dependencies {
 
     compile "org.scala-lang:scala-library:${gradle.scala.version}"
     compile "io.altoo:akka-kryo-serialization_${gradle.scala.depVersion}:1.0.0"
+    compile "com.lightbend.akka.management:akka-management-cluster-http_${gradle.scala.depVersion}:${gradle.akka_management.version}" 

Review Comment:
   IIRC, our downstream is not using these libraries while we are running FPCScheduler on Kubernetes.
   @ningyougang Do you have any idea about the difference between downstream and upstream?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r926867150


##########
core/scheduler/build.gradle:
##########
@@ -66,6 +66,9 @@ dependencies {
 
     compile "org.scala-lang:scala-library:${gradle.scala.version}"
     compile "io.altoo:akka-kryo-serialization_${gradle.scala.depVersion}:1.0.0"
+    compile "com.lightbend.akka.management:akka-management-cluster-http_${gradle.scala.depVersion}:${gradle.akka_management.version}" 

Review Comment:
   @ningyougang thank you, you are correct, akka-management-cluster-http is not needed although I believe the other two are. I've removed in recent commit. Does this resolve your original comment, @style95 ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] hunhoffe commented on a diff in pull request #5278: Prepare to integrate new scheduler into apache/openwhisk-deploy-kube

Posted by GitBox <gi...@apache.org>.
hunhoffe commented on code in PR #5278:
URL: https://github.com/apache/openwhisk/pull/5278#discussion_r929015057


##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -230,6 +233,10 @@ object Controller {
   def main(args: Array[String]): Unit = {
     implicit val actorSystem = ActorSystem("controller-actor-system")
     implicit val logger = new AkkaLogging(akka.event.Logging.getLogger(actorSystem, this))
+    if (useClusterBootstrap) {
+      AkkaManagement(actorSystem).start()
+      ClusterBootstrap(actorSystem).start()
+    }

Review Comment:
   I believe it would be correct to delete lines 158 & 159 (which I will do), but the ShardingContainerPoolBalancer still needs a handle to the cluster object (when a cluster is created) so I believe the rest needs to stay.
   
   Unless we make it so the controller fails if a cluster is not started (e.g., no cluster discovery and no seed nodes), then we could replace the above portion with something like:
   ```scala
   private val cluster: Cluster = Cluster(actorSystem)
   ```
   
   However, I'm not sure how well that jives with the standalone version of OpenWhisk. I'd have to look into that. I was trying to keep my changes minimal, so I did not go in that direction.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org