Skip to content

add system config options for success / failure levels to write blocking / non-blocking activations to db #5169

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Feb 1, 2022
Merged

add system config options for success / failure levels to write blocking / non-blocking activations to db #5169

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
af7cb02
add config option to disable successful non-blocking activation write…
Oct 18, 2021
5881bc6
fix check
Oct 19, 2021
7916d90
feedback
Jan 13, 2022
5e2b36f
Merge branch 'master' into add-config-to-not-save-successful-nonblocking
Jan 13, 2022
a3db5d3
fix tests
Jan 13, 2022
e43ee00
Merge branch 'master' into add-config-to-not-save-successful-nonblocking
Jan 13, 2022
823e3e9
rebase
Jan 13, 2022
7bbac30
feedback
Jan 19, 2022
4a4a083
update config precedence
Jan 26, 2022
666b707
Apply suggestions from code review
bdoyle0182 Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions common/scala/src/main/resources/application.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,10 @@ whisk {
# invocations made with `X-OW-EXTRA-LOGGING: on` header, will force the activation to be stored
disable-store-result = false

# Disables database store for non-blocking + successful activations
# invocations made with `X-OW-EXTRA-LOGGING: on` header, will force the activation to be stored
disable-store-non-blocking-result = false

# Enable metadata logging of activations not stored in the database
unstored-logs-enabled = false
}
Expand Down
1 change: 1 addition & 0 deletions common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ object ConfigKeys {
val swaggerUi = "whisk.swagger-ui"

val disableStoreResult = s"$activation.disable-store-result"
val disableStoreNonBlockingResult = s"$activation.disable-store-non-blocking-result"
val unstoredLogsEnabled = s"$activation.unstored-logs-enabled"

val apacheClientConfig = "whisk.apache-client"
Expand Down
16 changes: 11 additions & 5 deletions common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationStore.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,25 @@ case class UserContext(user: Identity, request: HttpRequest = HttpRequest())
trait ActivationStore {

protected val disableStoreResultConfig = loadConfigOrThrow[Boolean](ConfigKeys.disableStoreResult)
protected val disableStoreNonBlockingResultConfig = loadConfigOrThrow[Boolean](ConfigKeys.disableStoreNonBlockingResult)
protected val unstoredLogsEnabledConfig = loadConfigOrThrow[Boolean](ConfigKeys.unstoredLogsEnabled)

/**
* Checks if an activation should be stored in database and stores it.
*
* @param activation activation to store
* @param isBlockingActivation is activation blocking
* @param disableBlockingStore do not store activation if successful and blocking
* @param disableNonBlockingStore do not store activation if successful and non-blocking
Copy link
Member

Choose a reason for hiding this comment

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

Widening the signature here this way strikes me as confusing - couldn't this be better done with an enum: always store, never store, store for non blocking only, etc?

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 guess my question would be how could you do that without breaking the config that already exists for disableBlockingStore that's presumably already being used?

The other thing is I think people still want to distinguish between successful and failures so they only want to turn off writes for successes. I'm not sure what the best solution is or what options should be offered.

I think I understand your suggestion in that there could be an enum set with values ALWAYS_STORE_SUCCESSES, STORE_BLOCKING_SUCCESSES, STORE_NONBLOCKING_SUCCESSES, STORE_NO_SUCCESSES. However I think doing this would break deployments unless we kept it as two booleans in the config as currently is in the pr and built the enum from the boolean values. But I think that defeats the purpose of setting up the enum at all.

And then on our discussion for our use case specifically with only needing specific failure types written (developer and system). I think that should be a separate check which could either be an enum or just a boolean of storeApplication Errors since I think developer and system errors both are very similar in what they mean. The divider in failures to me is really just whether it's a user application error or not.

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 there are two orthogonal issues - one is configuration and making that backward compatible. The other is the internal interface and implementation.

I think the distinctions you want to make suggests this alternate/simpler enum:

  • STORE_ALWAYS (default)
  • STORE_ON_FAILURE
  • STORE_NON_BLOCKING

From a configuration point of view, you can continue to accept the current configuration property (perhaps with a deprecation warning) and introduce a new configuration that maps to the enum. It would be an error to specify both configurations properties.

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 agree with that setup. The one case that I think is confusing is the existing boolean and wanting failed blocking activations to still be stored. Because setting the current boolean means store failed blocking activations and non-blocking activations. So that falls between STORE_ON_FAILURE and STORE_NON_BLOCKING

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that STORE_ON_FAILURE applies to everything. I suppose we can have two modes for this: STORE_ON_FAILURE_BLOCKING_AND_NON_BLOCKING for sync/async and STORE_ON_FAILURE_NON_BLOCKING_ONLY (and so OK there are four again :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I have. I can't think of a scenario where someone would want a subset of non blocking activations written, but not a larger subset for blocking. I can only think of wanting a subset of blocking and wanting a larger subset for non-blocking. I'll update the pr if you're good with this

STORE_ALWAYS
STORE_NON_BLOCKING_OR_ON_BLOCKING_FAILURE
STORE_ON_FAILURE
STORE_ON_NON_APPLICATION_FAILURE

Copy link
Member

@rabbah rabbah Jan 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

STORE_NON_BLOCKING_OR_ON_BLOCKING_FAILURE and STORE_ON_FAILURE are redundant for blocking invokes, no? That is, to store all non blocking I'd use STORE_NON_BLOCKING_OR_ON_BLOCKING_FAILURE and to only store non blocking failures, I'd use STORE_ON_FAILURE and to store blocking failures, i can use either. (I think the parameters should be orthogonal.)

I like the addition of STORE_ON_NON_APPLICATION_FAILURE - It further refines the on failure mode - so this would capture system errors and uncaught exceptions in the end user code.

Another crank on your suggestions nets us here, perhaps?

STORE_ALWAYS (default)
STORE_NON_BLOCKING
STORE_FAILURES
STORE_FAILURES_NOT_APPLICATION_ERRORS

Copy link
Contributor Author

@bdoyle0182 bdoyle0182 Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yea so what I was originally going to say as well is that I think we're just getting close to having a separate config for each invocation type, blocking and non-blocking. In your list, the original person that created the blocking config wanted only blocking failures and all non-blocking; which is why I had STORE_NON_BLOCKING_OR_ON_BLOCKING_FAILURE. Almost everyone would want this because not storing any non-blocking means you can't run sequences I believe, we don't have any need for that feature which is why we can turn off non-blocking. But you are correct that would mean the parameters aren't orthogonal, which is why I think we're at the point of just having two separate params with the same enum.

STORE_ALWAYS
STORE_FAILURES
STORE_FAILURES_NOT_APPLICATION_ERRORS

* @param context user and request context
* @param transid transaction ID for request
* @param notifier cache change notifier
* @return Future containing DocInfo related to stored activation
*/
def storeAfterCheck(activation: WhiskActivation,
isBlockingActivation: Boolean,
disableStore: Option[Boolean],
disableBlockingStore: Option[Boolean],
disableNonBlockingStore: Option[Boolean],
context: UserContext)(implicit transid: TransactionId,
notifier: Option[CacheChangeNotification],
logging: Logging): Future[DocInfo] = {
Expand All @@ -58,7 +62,8 @@ trait ActivationStore {
activation.response.isSuccess,
isBlockingActivation,
transid.meta.extraLogging,
disableStore.getOrElse(disableStoreResultConfig))) {
disableBlockingStore.getOrElse(disableStoreResultConfig),
disableNonBlockingStore.getOrElse(disableStoreNonBlockingResultConfig))) {

store(activation, context)
} else {
Expand Down Expand Up @@ -186,14 +191,15 @@ trait ActivationStore {
* @param isSuccess is successful activation
* @param isBlocking is blocking activation
* @param debugMode is logging header set to "on" for the invocation
* @param disableStore is disable store configured
* @param disableBlockingStore is disable store configured
* @return Should the activation be stored to the database
*/
private def shouldStoreActivation(isSuccess: Boolean,
isBlocking: Boolean,
debugMode: Boolean,
disableStore: Boolean): Boolean = {
!isSuccess || !isBlocking || debugMode || !disableStore
disableBlockingStore: Boolean,
disableNonBlockingStore: Boolean): Boolean = {
!isSuccess || debugMode || (!isBlocking && !disableNonBlockingStore) || (isBlocking && !disableBlockingStore)
}
}

Expand Down
2 changes: 1 addition & 1 deletion core/controller/src/main/scala/org/apache/openwhisk/core/controller/Triggers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ trait WhiskTriggersApi extends WhiskCollectionAPI {
triggerActivation
}
.map { activation =>
activationStore.storeAfterCheck(activation, false, None, context)
activationStore.storeAfterCheck(activation, false, None, None, context)
}

respondWithActivationIdHeader(triggerActivationId) {
Expand Down
2 changes: 1 addition & 1 deletion ...roller/src/main/scala/org/apache/openwhisk/core/controller/actions/PrimitiveActions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ protected[actions] trait PrimitiveActions {
}
}

activationStore.storeAfterCheck(activation, blockingComposition, None, context)(transid, notifier = None, logging)
activationStore.storeAfterCheck(activation, blockingComposition, None, None, context)(transid, notifier = None, logging)

activation
}
Expand Down
2 changes: 1 addition & 1 deletion ...troller/src/main/scala/org/apache/openwhisk/core/controller/actions/SequenceActions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ protected[actions] trait SequenceActions {
}
}

activationStore.storeAfterCheck(seqActivation, blockingSequence, None, context)(
activationStore.storeAfterCheck(seqActivation, blockingSequence, None, None, context)(
transid,
notifier = None,
logging)
Expand Down
2 changes: 1 addition & 1 deletion core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ class InvokerReactive(
/** Stores an activation in the database. */
private val store = (tid: TransactionId, activation: WhiskActivation, isBlocking: Boolean, context: UserContext) => {
implicit val transid: TransactionId = tid
activationStore.storeAfterCheck(activation, isBlocking, None, context)(tid, notifier = None, logging)
activationStore.storeAfterCheck(activation, isBlocking, None, None, context)(tid, notifier = None, logging)
}

/** Creates a ContainerProxy Actor when being called. */
Expand Down
4 changes: 2 additions & 2 deletions ...c/test/scala/org/apache/openwhisk/core/controller/test/ActionsApiWithDbPollingTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class ActionsApiWithDbPollingTests extends ControllerTestCommon with WhiskAction
// storing the activation in the db will allow the db polling to retrieve it
// the test harness makes sure the activation id observed by the test matches
// the one generated by the api handler
storeActivation(activation, false, false, context)
storeActivation(activation, false, false, false, context)
try {
Post(s"$collectionPath/${action.name}?blocking=true") ~> Route.seal(routes(creds)) ~> check {
status should be(OK)
Expand Down Expand Up @@ -114,7 +114,7 @@ class ActionsApiWithDbPollingTests extends ControllerTestCommon with WhiskAction
// storing the activation in the db will allow the db polling to retrieve it
// the test harness makes sure the activation id observed by the test matches
// the one generated by the api handler
storeActivation(activation, false, false, context)
storeActivation(activation, false, false, false, context)
try {
Post(s"$collectionPath/${action.name}?blocking=true") ~> Route.seal(routes(creds)) ~> check {
status should be(InternalServerError)
Expand Down
49 changes: 34 additions & 15 deletions tests/src/test/scala/org/apache/openwhisk/core/controller/test/ActivationsApiTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
end = Instant.now)
}.toList
try {
(notExpectedActivations ++ activations).foreach(storeActivation(_, false, false, context))
(notExpectedActivations ++ activations).foreach(storeActivation(_, false, false, false, context))
waitOnListActivationsInNamespace(namespace, 2, context)

org.apache.openwhisk.utils.retry {
Expand Down Expand Up @@ -179,7 +179,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
}.toList

try {
(notExpectedActivations ++ activations).foreach(storeActivation(_, false, false, context))
(notExpectedActivations ++ activations).foreach(storeActivation(_, false, false, false, context))
waitOnListActivationsInNamespace(namespace, 2, context)
checkCount("", 2)

Expand Down Expand Up @@ -254,7 +254,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
end = now.plusSeconds(30))) // should match

try {
(notExpectedActivations ++ activations).foreach(storeActivation(_, false, false, context))
(notExpectedActivations ++ activations).foreach(storeActivation(_, false, false, false, context))
waitOnListActivationsInNamespace(namespace, activations.length, context)

{ // get between two time stamps
Expand Down Expand Up @@ -363,7 +363,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
annotations = Parameters("path", s"${namespace.asString}/pkg/xyz"))
}.toList
try {
(notExpectedActivations ++ activations ++ activationsInPackage).foreach(storeActivation(_, false, false, context))
(notExpectedActivations ++ activations ++ activationsInPackage).foreach(storeActivation(_, false, false, false, context))
waitOnListActivationsMatchingName(namespace, EntityPath("xyz"), activations.length, context)
waitOnListActivationsMatchingName(
namespace,
Expand Down Expand Up @@ -479,7 +479,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
}.toList

try {
activations.foreach(storeActivation(_, false, false, context))
activations.foreach(storeActivation(_, false, false, false, context))
waitOnListActivationsInNamespace(namespace, activations.size, context)

Get(s"$collectionPath?skip=1") ~> Route.seal(routes(creds)) ~> check {
Expand All @@ -503,7 +503,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
}.toList

try {
activations.foreach(storeActivation(_, false, false, context))
activations.foreach(storeActivation(_, false, false, false, context))
waitOnListActivationsInNamespace(namespace, activations.size, context)

Get(s"$collectionPath?limit=1") ~> Route.seal(routes(creds)) ~> check {
Expand Down Expand Up @@ -533,7 +533,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
start = Instant.now,
end = Instant.now)
try {
storeActivation(activation, false, false, context)
storeActivation(activation, false, false, false, context)

Get(s"$collectionPath/${activation.activationId.asString}") ~> Route.seal(routes(creds)) ~> check {
status should be(OK)
Expand Down Expand Up @@ -570,7 +570,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
start = Instant.now,
end = Instant.now)
try {
storeActivation(activation, false, false, context)
storeActivation(activation, false, false, false, context)

Get(s"$collectionPath/${activation.activationId.asString}/result") ~> Route.seal(routes(creds)) ~> check {
status should be(OK)
Expand All @@ -583,7 +583,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
}

//// GET /activations/id/result when db store is disabled
it should "return activation empty when db store is disabled" in {
it should "return activation empty when db store is disabled for blocking" in {
implicit val tid = transid()
val activation =
WhiskActivation(
Expand All @@ -594,7 +594,26 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
start = Instant.now,
end = Instant.now)

storeActivation(activation, true, true, context)
storeActivation(activation, true, true, false, context)

Get(s"$collectionPath/${activation.activationId.asString}/result") ~> Route.seal(routes(creds)) ~> check {
status should be(NotFound)
}
}

//// GET /activations/id/result when db store is disabled
it should "return activation empty when db store is disabled for non-blocking" in {
implicit val tid = transid()
val activation =
WhiskActivation(
namespace,
aname(),
creds.subject,
ActivationId.generate(),
start = Instant.now,
end = Instant.now)

storeActivation(activation, true, true, true, context)

Get(s"$collectionPath/${activation.activationId.asString}/result") ~> Route.seal(routes(creds)) ~> check {
status should be(NotFound)
Expand All @@ -613,7 +632,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
start = Instant.now,
end = Instant.now)
try {
storeActivation(activation, false, true, context)
storeActivation(activation, false, true, false, context)

Get(s"$collectionPath/${activation.activationId.asString}/result") ~> Route.seal(routes(creds)) ~> check {
status should be(OK)
Expand All @@ -638,7 +657,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
end = Instant.now,
response = ActivationResponse.whiskError("activation error"))
try {
storeActivation(activation, true, true, context)
storeActivation(activation, true, true, true, context)

Get(s"$collectionPath/${activation.activationId.asString}/result") ~> Route.seal(routes(creds)) ~> check {
status should be(OK)
Expand All @@ -662,7 +681,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
start = Instant.now,
end = Instant.now)
try {
storeActivation(activation, false, false, context)
storeActivation(activation, false, false, false, context)

Get(s"$collectionPath/${activation.activationId.asString}/logs") ~> Route.seal(routes(creds)) ~> check {
status should be(OK)
Expand All @@ -685,7 +704,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi
ActivationId.generate(),
start = Instant.now,
end = Instant.now)
storeActivation(activation, false, false, context)
storeActivation(activation, false, false, false, context)
try {

Get(s"$collectionPath/${activation.activationId.asString}/bogus") ~> Route.seal(routes(creds)) ~> check {
Expand Down Expand Up @@ -758,7 +777,7 @@ class ActivationsApiTests extends ControllerTestCommon with WhiskActivationsApi

val activation =
new BadActivation(namespace, aname(), creds.subject, ActivationId.generate(), Instant.now, Instant.now)
storeActivation(activation, false, false, context)
storeActivation(activation, false, false, false, context)

Get(s"$collectionPath/${activation.activationId}") ~> Route.seal(routes(creds)) ~> check {
status should be(InternalServerError)
Expand Down
3 changes: 2 additions & 1 deletion tests/src/test/scala/org/apache/openwhisk/core/controller/test/ControllerTestCommon.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,9 @@ protected trait ControllerTestCommon
activation: WhiskActivation,
isBlockingActivation: Boolean,
disableStore: Boolean,
disableNonBlockingStore: Boolean,
context: UserContext)(implicit transid: TransactionId, timeout: Duration = 10 seconds): DocInfo = {
val docFuture = activationStore.storeAfterCheck(activation, isBlockingActivation, Some(disableStore), context)(
val docFuture = activationStore.storeAfterCheck(activation, isBlockingActivation, Some(disableStore), Some(disableNonBlockingStore), context)(
transid,
notifier = None,
logging)
Expand Down