-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add administrative interface to invoker and controller to reconfigure runtimes #4790
base: master
Are you sure you want to change the base?
Conversation
extractCredentials { | ||
case Some(BasicHttpCredentials(username, password)) => | ||
if (username == controllerUsername && password == controllerPassword) { | ||
entity(as[String]) { runtime => |
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.
Usually, entity(as[Runtimes]) may be better, but if we apply this, need to change Runtimes
to support convert json to entity Runtimes, and on the other hand, runtime.json's format doesn't match Runtimes entity, need to change a lot if use entity(as[Runtimes])
to receive the data.
Fortunately,we can reuse openwhisk itself's initialize method, just convert the runtime string to Runtimes.
So here, i think pass runtime string would be ok.
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.
I think you can do as json object
instead of string
to tighten this a bit more. I agree not to deserialize into a Runtimes
instance.
logging.error(this, s"Received invalid runtimes manifest") | ||
complete(s"Received invalid runtimes manifest") | ||
} else { | ||
parameter('limit.?) { limit => |
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.
Support passed limit invokers, e.g. ?limit=0:1
( sent to invoker0, invoker1 only)
And the config runtime request can be sent to some limited invokers which included in managed invokers as well.
@@ -27,7 +27,7 @@ whisk.spi { | |||
EntitlementSpiProvider = org.apache.openwhisk.core.entitlement.LocalEntitlementProvider | |||
AuthenticationDirectiveProvider = org.apache.openwhisk.core.controller.BasicAuthenticationDirective | |||
InvokerProvider = org.apache.openwhisk.core.invoker.InvokerReactive | |||
InvokerServerProvider = org.apache.openwhisk.core.invoker.DefaultInvokerServer | |||
InvokerServerProvider = org.apache.openwhisk.core.invoker.InvokerServer |
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.
This should be reverted.
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.
DefaultInvokerServer
is already reverted
val invokerArray = targetValue.split(":") | ||
val beginIndex = invokerArray(0).toInt | ||
val finishIndex = invokerArray(1).toInt | ||
if (finishIndex < beginIndex) { |
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.
Would be great to support 0:0
case as well.
Ansible is using the same way.
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.
Yes, already support 0:0 case
.
5be2eab
to
8573912
Compare
Codecov Report
@@ Coverage Diff @@
## master #4790 +/- ##
==========================================
- Coverage 82.35% 75.49% -6.87%
==========================================
Files 198 199 +1
Lines 9021 9123 +102
Branches 353 379 +26
==========================================
- Hits 7429 6887 -542
- Misses 1592 2236 +644
Continue to review full report at Codecov.
|
@@ -203,6 +203,9 @@ | |||
"CONFIG_whisk_db_activationsFilterDdoc": "{{ db_whisk_activations_filter_ddoc | default() }}" | |||
"CONFIG_whisk_userEvents_enabled": "{{ user_events | default(false) | lower }}" | |||
|
|||
"CONFIG_whisk_credentials_controller_username": "{{ controller.username }}" | |||
"CONFIG_whisk_credentials_controller_password": "{{ controller.password }}" | |||
|
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.
If generate controller/invoker credentials to container's /conf/, the Standalone Tests
run failed due to lack /conf/ under the build machine
On the other hand, couchdb credentials is passed via environment variable
, so controller/invoker credentials can be passed via environment variable as well.
556b5bd
to
1ec0720
Compare
Already added test cases |
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 limiting of a runtime to specific invokers is related to how invokers are partitioned for black box vs non-black box invokers (based on a % of active invokers in the system). I suggest splitting out the "limit" feature into a separate PR and having a mechanism for specifying distributions of runtimes to invokers and letting the load balancer calculate the partition.
That isn't powerful enough to when it is desirable to route to a specific invoker - is that a desired feature (vs limiting to a range of invokers active in the system)?
val execManifest = ExecManifest.initialize(runtime) | ||
if (execManifest.isFailure) { | ||
logging.error(this, s"Received invalid runtimes manifest") | ||
complete(s"Received invalid runtimes manifest") |
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.
this completes the https request with status code 200 - is that what was intended?
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.
Hi, almost forgot this patch :(
I changed like below
logging.info(this, s"received invalid runtimes manifest")
complete(StatusCodes.BadRequest)
entity(as[String]) { runtime => | ||
val execManifest = ExecManifest.initialize(runtime) | ||
if (execManifest.isFailure) { | ||
logging.error(this, s"Received invalid runtimes manifest") |
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.
.error
is for internal errors, where here this is user input error, i think .info
is better.
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.
Already changed to .info
parameter('limit.?) { limit => | ||
limit match { | ||
case Some(targetValue) => | ||
val pattern = "\\d+:\\d" |
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.
use
val pattern = "\\d+:\\d" | |
val pattern = """\d+:\d""" |
using triple quotes allows you to use a regex without escape characters.
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.
Changed
if (username == controllerCredentials.username && password == controllerCredentials.password) { | ||
entity(as[String]) { runtime => | ||
val execManifest = ExecManifest.initialize(runtime) | ||
if (execManifest.isFailure) { |
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.
scala nit: you can use a case match here instead.
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.
I just follow other codes using execManifest.isFailure
to judge whether success or fail
complete(s"finishIndex can't be less than beginIndex") | ||
} else { | ||
val targetInvokers = (beginIndex to finishIndex).toList | ||
loadBalancer.sendRuntimeToInvokers(runtime, Some(targetInvokers)) |
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.
is there a later validation to check that the invoker indexing is in range?
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.
Yes, validate it in ShardingContainerPoolLoadbalacer.scala
/** send runtime to invokers*/
override def sendRuntimeToInvokers(runtime: String, targetInvokers: Option[List[Int]]): Unit = {
val runtimeMessage = RuntimeMessage(runtime)
schedulingState.managedInvokers.filter { manageInvoker =>
targetInvokers.getOrElse(schedulingState.managedInvokers.map(_.id.instance)).contains(manageInvoker.id.instance)
} foreach { invokerHealth =>
val topic = s"invoker${invokerHealth.id.toInt}"
messageProducer.send(topic, runtimeMessage).andThen {
case Success(_) =>
logging.info(this, s"Successfully posted runtime to topic $topic")
case Failure(_) =>
logging.error(this, s"Failed posted runtime to topic $topic")
}
}
}
limit match { | ||
case Some(targetValue) => | ||
val pattern = "\\d+:\\d" | ||
if (targetValue.matches(pattern)) { |
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.
scala nit: you can rewrite this either if/else and nested clauses using case matching on regex.
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.
hm.. can you show a example? my codes like below
if (targetValue.matches(pattern)) {
val invokerArray = targetValue.split(":")
val beginIndex = invokerArray(0).toInt
val finishIndex = invokerArray(1).toInt
if (finishIndex < beginIndex) {
logging.info(this, "finishIndex can't be less than beginIndex")
complete(StatusCodes.BadRequest)
} else {
val targetInvokers = (beginIndex to finishIndex).toList
loadBalancer.sendRuntimeToInvokers(runtime, Some(targetInvokers))
logging.info(this, "config runtime request is already sent to target invokers")
complete(StatusCodes.BadRequest)
}
} else {
logging.info(this, "limit value can't match [beginIndex:finishIndex]")
complete(StatusCodes.BadRequest)
}
|
||
private val invokerCredentials = loadConfigOrThrow[InvokerCredentials](ConfigKeys.invokerCredentials) | ||
|
||
override def routes(implicit transid: TransactionId): Route = { |
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.
how will new runtime images get pulled to the invoker nodes? are you assuming they get pulled either via stem-cell "run" or when an action actually runs in the future?
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.
When send to reqeust to backend (e.g. curl -u xxx:xxx-X POST http://xxx:xxx:10001/config/runtime
), it will pull new runtime image immediately and create prewarm container if the stem-cell > 0.
@@ -258,10 +258,40 @@ public static int getControllerBasePort() { | |||
return Integer.parseInt(whiskProperties.getProperty("controller.host.basePort")); | |||
} | |||
|
|||
public static String getControllerProtocol() { |
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.
i dont think any of these changes are necessary if you read configs through pureconfig.
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.
Yes, already deleted.
response.status shouldBe StatusCodes.OK | ||
} | ||
|
||
Thread.sleep(5.seconds.toMillis) |
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.
why?
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.
Just make previous's request handled successfully, because previous is a ansyc operation.
81caa3f
to
3e0603d
Compare
hm.. this patch just reconfigure the runtime.json(recreate prewarm containers according to passed runtime string), i don't understand what's mean
Yes, it is not powerful, so i deleted |
case None => | ||
loadBalancer.sendRuntimeToInvokers(runtime, None) | ||
logging.info(this, "config runtime request is already sent to all managed invokers") | ||
complete(StatusCodes.Accepted) |
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.
I changed to complete(StatusCodes.Accepted)
, because it is a async operation.
@@ -279,6 +285,23 @@ class ContainerPool(childFactory: ActorRefFactory => ActorRef, | |||
case RescheduleJob => | |||
freePool = freePool - sender() | |||
busyPool = busyPool - sender() | |||
case prewarmConfigList: PreWarmConfigList => | |||
laststPrewarmConfig = prewarmConfigList.list |
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.
Add a inner variable laststPrewarmConfig
, just make it pointed to the passed config runtime.
Because in this patch: #4698
when received ContainerRemoved
(may be prewarm container crashed), it will backfill the prewarm.
be5a6e5
to
6750ea9
Compare
fc63c07
to
12201bb
Compare
12201bb
to
c245908
Compare
Rebased. |
6733631
to
30a3e64
Compare
30a3e64
to
842baa1
Compare
Rebased |
passedConfig.exec.kind == config.exec.kind && passedConfig.memoryLimit == config.memoryLimit) | ||
.getOrElse(config) | ||
} | ||
latestPrewarmConfig = newPrewarmConfig |
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.
Support just change specify runtime config as well, e.g.
Let's assume runtimes.json include nodejs:12, python:2, swift4.1, we can change nodejs:12 runtime only via just pass nodejs:12 runtime info
, for other runtimes, just use previous runtime info directly.
Codecov Report
@@ Coverage Diff @@
## master #4790 +/- ##
==========================================
- Coverage 83.56% 76.99% -6.57%
==========================================
Files 201 202 +1
Lines 9515 9613 +98
Branches 400 415 +15
==========================================
- Hits 7951 7402 -549
- Misses 1564 2211 +647
Continue to review full report at Codecov.
|
a5c2986
to
214f8e4
Compare
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.
I like elements of this PR but the orchestration with the invokers (where it's selective) I think can lead to a situation where the controller has a view of the supported runtimes that is not supported by all the invokers. Don't you then need to inform the scheduler/loadbalancer of this disparity so it can route requests correctly to the supporting invokers?
Or do you envision only changing the stem cell configurations, and not the content of the runtimes manifest otherwise?
*/ | ||
|
||
package org.apache.openwhisk.common | ||
|
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.
You can use akka.http.scaladsl.model.headers.BasicHttpCredentials
instead of creating this class.
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.
I see later this is created to read/load the configuration - it might still work with BasicHttpCredentials
because it's case class with the same fields.
* @param runtime | ||
* @return the manifest if initialized successfully, or an failure | ||
*/ | ||
protected[core] def initialize(runtime: String): Try[Runtimes] = { |
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.
why not use the method above and set the manifestOverride
to Some(...)
at the call site?
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.
Yes, you are right, updated accordingly.
protected[core] def initialize(runtime: String): Try[Runtimes] = { | ||
val rmc = loadConfigOrThrow[RuntimeManifestConfig](ConfigKeys.runtimes) | ||
val mf = Try(runtime.parseJson.asJsObject).flatMap(runtimes(_, rmc)) | ||
var manifest: Option[Runtimes] = None |
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.
this is incorrect - the reason we do the assignment to manifest
on line 57 is to set the singleton. Here, this has no effect because the variable is local.
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.
I already changed to use existed initialize method.
extractCredentials { | ||
case Some(BasicHttpCredentials(username, password)) => | ||
if (username == controllerUsername && password == controllerPassword) { | ||
entity(as[String]) { runtime => |
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.
I think you can do as json object
instead of string
to tighten this a bit more. I agree not to deserialize into a Runtimes
instance.
(path("config" / "runtime") & post) { | ||
extractCredentials { | ||
case Some(BasicHttpCredentials(username, password)) => | ||
if (username == controllerCredentials.username && password == controllerCredentials.password) { |
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.
you can use as authentication directive which will automatically check the credentials and reject the request. This is OK just making an observation.
a7be1d4
to
695bb14
Compare
695bb14
to
edefa8a
Compare
Codecov Report
@@ Coverage Diff @@
## master #4790 +/- ##
==========================================
- Coverage 81.81% 74.15% -7.67%
==========================================
Files 204 212 +8
Lines 9950 10319 +369
Branches 447 435 -12
==========================================
- Hits 8141 7652 -489
- Misses 1809 2667 +858
Continue to review full report at Codecov.
|
@ningyougang I want to check if you want to continue this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4790 +/- ##
==========================================
- Coverage 82.10% 75.06% -7.04%
==========================================
Files 211 212 +1
Lines 10215 10319 +104
Branches 450 435 -15
==========================================
- Hits 8387 7746 -641
- Misses 1828 2573 +745 ☔ View full report in Codecov by Sentry. |
Sometimes, admin may want to reinitalize the runtime config, e.g. nodejs:10 prewarm container number is less, lead to
cold start
, in order to handle user's request as soon as possible, admin may want to reinitalize the runtime configuration to increase the nodejs:10 prewarm containers.And admin may want to reinitalize the runtime config on some limited invokers, this patch includes below changes
Description
Related issue and scope
My changes affect the following components
Types of changes
Checklist: