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

Adjust user memory via api #4917

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ningyougang
Copy link
Contributor

@ningyougang ningyougang commented Jun 9, 2020

Description

  • Ajust invoker's containerPool's userMemory via API

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.

@@ -426,3 +426,22 @@ object EventMessage extends DefaultJsonProtocol {

def parse(msg: String) = Try(format.read(msg.parseJson))
}

case class ByteSizeMessage(userMemory: ByteSize) extends Message {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name should denote its purpose more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm.. already changed it from ByteSizeMessage to UserMemoryMessage

```
Note: you can add `?limit` to specify target invokers, e.g. specify invoker0 and invoker1
```
curl -u ${username}:${password} -X POST http://${controllerAddress}:${controllerPort}/config/memory?limit=0:1 -d '1024 MB'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this API should be based on JSON and have a better protocol.
For example, the name of the invoker may not be simple like just invoker0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, i changed it to like below

curl  -u admin:admin -X POST http://xxx.xxx.xxx.xxx:10001/config/memory  -d '
[{"invoker":0,"memory": "20480 MB"}, {"invoker":1,"memory": "10240 MB"}]
'

@@ -305,6 +308,13 @@ class ContainerPool(childFactory: ActorRefFactory => ActorRef,
case RescheduleJob =>
freePool = freePool - sender()
busyPool = busyPool - sender()
case message: ByteSizeMessage =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this also has a more detailed protocol.
This might be extended for general configurations.

@@ -74,6 +76,7 @@ class ContainerPool(childFactory: ActorRefFactory => ActorRef,
var busyPool = immutable.Map.empty[ActorRef, ContainerData]
var prewarmedPool = immutable.Map.empty[ActorRef, PreWarmedData]
var prewarmStartingPool = immutable.Map.empty[ActorRef, (String, ByteSize)]
var latestUserMemory = poolConfig.userMemory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we can avoid using var if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm.. seems the lastUserMemory need to support changable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can take advantage of Option.

s"user memory is reconfigured from ${latestUserMemory.toString} to ${message.userMemory.toString}")
latestUserMemory = message.userMemory
case UserMemoryQuery =>
sender() ! latestUserMemory.toString
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about sending the intact obejct directly and handle the type conversion in the InvokerServer layer?
I think this kind of type conversion is only related to the InvokerServer and the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm..
When we want to change invoker's user memory, in this pr, need to send the request to controller, there has one benefit that can modfiy many invoker's user memory with one request only. e.g.

curl  -u admin:admin -X POST http://xxx.xxx.xxx.xxx:10001/config/memory  -d '
[
{"invoker":0,"memory": "20480 MB"},
{"invoker":1,"memory": "10240 MB"}
{"invoker":2,"memory": "51200 MB"}
]
'

if send change invoker user memory request to invokerServer, if want to modify many invokers's user memory, need to send http request many times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok since this is just a ByteSize I think we can take this approach.

entity(as[String]) { memory =>
try {
val userMemoryMessage = ByteSizeMessage(ByteSize.fromString(memory))
if (userMemoryMessage.userMemory.size == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not the only precondition to configure memory.
This value can be smaller than MemoryLimit.min then invokers would not be able to run any container.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly, the user meomry can't be less than MemoryLimit.min

logging.info(
this,
s"user memory is reconfigured from ${latestUserMemory.toString} to ${message.userMemory.toString}")
latestUserMemory = message.userMemory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if controllers check the validity of this value, it would be great to cross-check the value again in the invoker layer.

And regarding the check, I feel the application should not concern the condition of the underlying host.
But on the other hand, I am not sure it would be reasonable to allow a bigger memory than the invoker host has.

I would defer this to other reviewers.

@ningyougang ningyougang force-pushed the adjust_user_memory branch 2 times, most recently from f9dd602 to c7f68ff Compare October 16, 2020 01:20
* @param userMemory
* @param targetInvokers
*/
def sendUserMemoryToInvoker(userMemoryMessage: UserMemoryMessage, targetInvoker: Int): Unit = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about changing the name like this?

Suggested change
def sendUserMemoryToInvoker(userMemoryMessage: UserMemoryMessage, targetInvoker: Int): Unit = {}
def sendChangeRequestToInvoker(userMemoryMessage: UserMemoryMessage, targetInvoker: Int): Unit = {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

extractCredentials {
case Some(BasicHttpCredentials(username, password)) =>
if (username == controllerCredentials.username && password == controllerCredentials.password) {
entity(as[String]) { memory =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we directly convert this to ConfigMemoryList?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, updated.

entity(as[String]) { memory =>
val configMemoryList = memory.parseJson.convertTo[ConfigMemoryList]

val existIllegalUserMemory = configMemoryList.items.exists { configMemory =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scalaism. I think we can change this something like this:

configMemoryList.items.find(MemoryLimit.MIN_MEMORY.compare(.memory) > 0)) match {
case Some(
) =>
// reject the request
case None =>
}

@@ -74,6 +76,7 @@ class ContainerPool(childFactory: ActorRefFactory => ActorRef,
var busyPool = immutable.Map.empty[ActorRef, ContainerData]
var prewarmedPool = immutable.Map.empty[ActorRef, PreWarmedData]
var prewarmStartingPool = immutable.Map.empty[ActorRef, (String, ByteSize)]
var latestUserMemory = poolConfig.userMemory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can take advantage of Option.

s"user memory is reconfigured from ${latestUserMemory.toString} to ${message.userMemory.toString}")
latestUserMemory = message.userMemory
case UserMemoryQuery =>
sender() ! latestUserMemory.toString
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok since this is just a ByteSize I think we can take this approach.

@@ -220,9 +223,3 @@ trait InvokerServerProvider extends Spi {
def instance(
invoker: InvokerCore)(implicit ec: ExecutionContext, actorSystem: ActorSystem, logger: Logging): BasicRasService
}

object DefaultInvokerServer extends InvokerServerProvider {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it to core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/DefaultInvokerServer.scala

}

case class ConfigMemory(invoker: Int, memory: ByteSize)
case class ConfigMemoryList(items: List[ConfigMemory])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can directly unmarshal a list of ConfigMemory rather than haveing this case class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

def parse(msg: String) = Try(serdes.read(msg.parseJson))
}

case class ConfigMemory(invoker: Int, memory: ByteSize)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about changing the name to InvokerConfiguration.
We might extend this to other invoker configurations as well in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@ningyougang ningyougang closed this Mar 3, 2021
@ningyougang ningyougang reopened this Mar 3, 2021
@style95 style95 added the stale old issue which needs to validate label May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale old issue which needs to validate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants