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 2020/04/17 05:53:31 UTC
[GitHub] [openwhisk] selfxp opened a new pull request #4885: Disable db
record store for successful blocking activations
selfxp opened a new pull request #4885: Disable db record store for successful blocking activations
URL: https://github.com/apache/openwhisk/pull/4885
## Description
<!--- Provide a detailed description of your changes. -->
<!--- Include details of what problem you are solving and how your changes are tested. -->
This PR makes storing of blocking successful activations volatile. More on the reasoning behind this PR can be found in the issue #4881. This PR is also related to #4659.
- Blocking successful `activations`, `sequences` and `compositions` won't be stored in the database in case the environment has `whisk.activation.disable-store-result` configuration variable set to `true`.
- Non-blocking or unsuccessful `activations`,`sequences` and `compositions` are not affected, and will continue to be stored in the database.
- User-events monitoring is not affected. All activations, including blocking successful will be tracked.
- Logs will continue to be accessible in case external logging services are configured.
We'll first try to get the activation record from the database, in case no record has been found we'll still try to get the logs using the activation Id provided in the call.
One concern was around security. The existing implementations in both `ElasticSearchLogStore` and `SplunkLogStore` only used the `activation_id` to query for the logs. In order to make sure that one tenant can't access other tenant's logs, we have changed the implementation to use both the `namespace` and the `activation_id` for the log store search.
- Blocking successful activations can still be stored in case the `X-OW-EXTRA-LOGGING` header is set to `on` when making the invocation.
## Related issue and scope
<!--- Please include a link to a related issue if there is one. -->
- [x] I opened an issue to propose and discuss this change (#4881)
## 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
- [ ] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- [x] Tests
- [ ] Deployment
- [ ] CLI
- [ ] 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/wiki/Contributing:-Git-guidelines#code-readiness) and followed the recommendations (Travis CI will check :).
- [x] I added tests to cover my changes.
- [ ] My changes require further changes to the documentation.
- [x] 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] codecov-io commented on issue #4885: Disable db record
store for successful blocking activations
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4885: Disable db record store for successful blocking activations
URL: https://github.com/apache/openwhisk/pull/4885#issuecomment-615072427
# [Codecov](https://codecov.io/gh/apache/openwhisk/pull/4885?src=pr&el=h1) Report
> Merging [#4885](https://codecov.io/gh/apache/openwhisk/pull/4885?src=pr&el=desc) into [master](https://codecov.io/gh/apache/openwhisk/commit/7113b733b6c8ade3b09b6808507e9b56a03158e4&el=desc) will **decrease** coverage by `47.53%`.
> The diff coverage is `6.81%`.
[](https://codecov.io/gh/apache/openwhisk/pull/4885?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4885 +/- ##
===========================================
- Coverage 83.24% 35.71% -47.54%
===========================================
Files 200 200
Lines 9307 9343 +36
Branches 392 394 +2
===========================================
- Hits 7748 3337 -4411
- Misses 1559 6006 +4447
```
| [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/4885?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../containerpool/logging/ElasticSearchLogStore.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9sb2dnaW5nL0VsYXN0aWNTZWFyY2hMb2dTdG9yZS5zY2FsYQ==) | `0.00% <0.00%> (-96.16%)` | :arrow_down: |
| [...sk/core/containerpool/logging/SplunkLogStore.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9sb2dnaW5nL1NwbHVua0xvZ1N0b3JlLnNjYWxh) | `0.00% <0.00%> (-84.62%)` | :arrow_down: |
| [...apache/openwhisk/core/controller/Activations.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9BY3RpdmF0aW9ucy5zY2FsYQ==) | `0.00% <0.00%> (-96.62%)` | :arrow_down: |
| [...rg/apache/openwhisk/core/controller/ApiUtils.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9BcGlVdGlscy5zY2FsYQ==) | `0.00% <0.00%> (-66.67%)` | :arrow_down: |
| [...isk/core/controller/actions/PrimitiveActions.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9hY3Rpb25zL1ByaW1pdGl2ZUFjdGlvbnMuc2NhbGE=) | `0.00% <0.00%> (-85.24%)` | :arrow_down: |
| [...hisk/core/controller/actions/SequenceActions.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9hY3Rpb25zL1NlcXVlbmNlQWN0aW9ucy5zY2FsYQ==) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [.../scala/org/apache/openwhisk/core/WhiskConfig.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvV2hpc2tDb25maWcuc2NhbGE=) | `94.92% <100.00%> (+0.03%)` | :arrow_up: |
| [.../openwhisk/core/containerpool/ContainerProxy.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9Db250YWluZXJQcm94eS5zY2FsYQ==) | `70.05% <100.00%> (-23.52%)` | :arrow_down: |
| [...a/org/apache/openwhisk/common/ConfigMapValue.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvbW1vbi9Db25maWdNYXBWYWx1ZS5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/openwhisk/core/controller/Namespaces.scala](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9OYW1lc3BhY2VzLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [138 more](https://codecov.io/gh/apache/openwhisk/pull/4885/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/openwhisk/pull/4885?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/openwhisk/pull/4885?src=pr&el=footer). Last update [7113b73...1242144](https://codecov.io/gh/apache/openwhisk/pull/4885?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] tysonnorris commented on a change in pull request
#4885: Disable db record store for successful blocking activations
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4885: Disable db record store for successful blocking activations
URL: https://github.com/apache/openwhisk/pull/4885#discussion_r410503723
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Activations.scala
##########
@@ -37,6 +38,7 @@ import org.apache.openwhisk.core.entity._
import org.apache.openwhisk.http.ErrorResponse.terminate
import org.apache.openwhisk.http.Messages
import org.apache.openwhisk.core.database.UserContext
+import pureconfig.loadConfig
Review comment:
I know some other configs are needing the additional import `import pureconfig.generic.auto._`, but I'm not sure the exact cases where this is required. It may come up in system test failures.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] tysonnorris commented on a change in pull request
#4885: Disable db record store for successful blocking activations
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4885: Disable db record store for successful blocking activations
URL: https://github.com/apache/openwhisk/pull/4885#discussion_r410504119
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Activations.scala
##########
@@ -72,6 +74,8 @@ object WhiskActivationsApi {
/** A trait implementing the activations API. */
trait WhiskActivationsApi extends Directives with AuthenticatedRouteProvider with AuthorizedRouteProvider with ReadOps {
+ protected val disableStoreResultConfig: Boolean = loadConfig[Boolean](ConfigKeys.disableStoreResult).getOrElse(false)
Review comment:
I think most other configs use the convention that a) defaults are in the application.conf and b) no defaults are in the code. Also I think `loadConfigOrThrow()` is preferred.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] tysonnorris commented on a change in pull request
#4885: Disable db record store for successful blocking activations
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4885: Disable db record store for successful blocking activations
URL: https://github.com/apache/openwhisk/pull/4885#discussion_r410506027
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/actions/PrimitiveActions.scala
##########
@@ -662,6 +668,9 @@ protected[actions] trait PrimitiveActions {
protected val controllerActivationConfig =
loadConfigOrThrow[ControllerActivationConfig](ConfigKeys.controllerActivation)
+
+ protected val disableStoreResultConfig: Boolean = loadConfig[Boolean](ConfigKeys.disableStoreResult).getOrElse(false)
Review comment:
Same as other config comment - use defaults from application.conf and `loadConfigOrThrow()`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] tysonnorris commented on a change in pull request
#4885: Disable db record store for successful blocking activations
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4885: Disable db record store for successful blocking activations
URL: https://github.com/apache/openwhisk/pull/4885#discussion_r410505633
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/ApiUtils.scala
##########
@@ -195,6 +201,72 @@ trait ReadOps extends Directives {
terminate(InternalServerError)
}
}
+
+ /**
+ * Waits on specified Future that returns an entity of type A from datastore.
+ * In case A entity is not stored, use the docId to search logstore
+ * Terminates HTTP request.
+ *
+ * @param entity future that returns an entity of type A fetched from datastore
+ * @param docId activation DocId
+ * @param disableStoreResultConfig configuration
+ * @param project a function A => JSON which projects fields form A
+ *
+ * Responses are one of (Code, Message)
+ * - 200 project(A) as JSON
+ * - 404 Not Found
+ * - 500 Internal Server Error
+ */
+ protected def getEntityAndProjectLog[A <: DocumentRevisionProvider, Au >: A](
+ entity: Future[A],
+ docId: DocId,
+ disableStoreResultConfig: Boolean,
+ project: A => Future[JsObject])(implicit transid: TransactionId, format: RootJsonFormat[A], ma: Manifest[A]) = {
+ onComplete(entity) {
+ case Success(entity) =>
+ logging.debug(this, s"[PROJECT] entity success")
+ onComplete(project(entity)) {
+ case Success(response: JsObject) =>
+ complete(OK, response)
+ case Failure(t: Throwable) =>
+ logging.error(this, s"[PROJECT] projection failed: ${t.getMessage}")
+ terminate(InternalServerError, t.getMessage)
+ }
+ case Failure(t: NoDocumentException) =>
+ // In case disableStoreResult configuration is active, persevere
+ // log might still be available even if entity was not
+ if (disableStoreResultConfig) {
+ val namespace = docId.asString.split("/")(0)
+ val id = docId.asString.split("/")(1)
+ val whiskActivation = WhiskActivation(
+ EntityPath(namespace),
+ EntityName(namespace),
+ Subject(),
+ ActivationId(id),
+ Instant.EPOCH,
+ Instant.now())
+ onComplete(project(whiskActivation.asInstanceOf[A])) {
+ case Success(response: JsObject) =>
+ logging.debug(this, s"[PROJECT] entity success")
+ complete(OK, response)
+ case Failure(t: Throwable) =>
+ logging.error(this, s"[PROJECT] projection failed: ${t.getMessage}")
Review comment:
may be useful to change this message from `[PROJECT]` to `[PROJECTLOG]`? (for both log messages)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] tysonnorris commented on a change in pull request
#4885: Disable db record store for successful blocking activations
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4885: Disable db record store for successful blocking activations
URL: https://github.com/apache/openwhisk/pull/4885#discussion_r410506631
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/actions/SequenceActions.scala
##########
@@ -175,7 +179,11 @@ protected[actions] trait SequenceActions {
case Failure(t) => logging.warn(this, s"activation event was not sent: $t")
}
}
- activationStore.storeAfterCheck(seqActivation, context)(transid, notifier = None)
+
+ // Don't store the record when activation is successful, blocking, not in debug mode and no disable store is configured
+ if (!(seqActivation.response.isSuccess && blockingSequence && !transid.meta.extraLogging && disableSequenceStoreResultConfig)) {
Review comment:
Is there a way to move this logic to a central location e.g. `shouldStoreActivation()`, so that it is more obvious what aspects are considered for this, and none are forgotten in some places where the logic is repeated?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] tysonnorris commented on a change in pull request
#4885: Disable db record store for successful blocking activations
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4885: Disable db record store for successful blocking activations
URL: https://github.com/apache/openwhisk/pull/4885#discussion_r410502871
##########
File path: common/scala/src/main/resources/application.conf
##########
@@ -153,7 +153,7 @@ whisk {
max-poll-interval-ms = 1800000 // 30 minutes
// The maximum amount of time the server will block before answering
- // the fetch request if there isn't sufficient data to immediately
+ // the fetch request if there isn`t sufficient data to immediately
Review comment:
smart quote?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services