Add limit to not store activations for a limitted namespace. #4234
Conversation
With this PR, operators will be able to disable storing of activations into activations store. The flag is implemented like the per-namespace-limits. It can be set with wskadmin. This PR was the initial idea of apache#4078. In the meantime, the idea in the other PR changed, to implement a throttle instead of a switch. But as this is a completely new type of rate-limit (which does not only allow or deny requests) that's a bit bigger to implement. So I'll go with a staged approach here and implement it as switch first. If someone needs a throttle instead of completely switching off the writes to the DB, this can be brought up again and the throttle can be built up on the solution we already have.
| .map { activation => | ||
| activationStore.store(activation, context) | ||
| } | ||
| if (user.limits.storeActivations.getOrElse(true)) { |
There was a problem hiding this comment.
The true default is scattered throughout the codebase now. I'm wondering if there is a good way of centralizing that.
One way is to add a val/def to the UserLimits class, which defaults to true always.
The other way is to add a system "limit" which makes it possible to enable/disable this for the whole system, therefore acting as the default.
I realize though that the second approach might be a bigger undertaking. Maybe it's fine as-is and we're better off having the default at the actual caller side to later get rid off.
More generally speaking: We should think about building a more generalized cascading system for our limits. I envision something like this:
- Load the user-limits from the database
- Merge them with the system limits
- Pass the merged versions through the code (maybe as part of the identity class)
That'd help us in cases as those and potentially transforms all the Option to actual values before we hit the location of usage.
You can ignore this comment if you want to, just wanted to leave the thoughts here.
There was a problem hiding this comment.
I very much like the idea of consolidating the defaults --- for this and other annotations we use.
| @@ -569,7 +569,9 @@ protected[actions] trait PrimitiveActions { | |||
| } | |||
| } | |||
|
|
|||
| activationStore.store(activation, context)(transid, notifier = None) | |||
| if (user.limits.storeActivations.getOrElse(true)) { | |||
There was a problem hiding this comment.
Can/should we move this into the ActivationStore itself? The user's identity is available in there as well.
I guess the thought was to not do that, because then each store needs to make sure to implement this if? Can we instead maybe come up with a wrapper function for all activationStore.store calls that takes care of a general concern like this?
There was a problem hiding this comment.
Now, I introduced the method storeAfterCheck, to do exactly that in ActivationStore, independant of the implementation.
tools/admin/wskadmin
Outdated
| @@ -132,6 +132,8 @@ def parseArgs(): | |||
| subcmd.add_argument('--firesPerMinute', help='trigger fires per minute allowed', type=int) | |||
| subcmd.add_argument('--concurrentInvocations', help='concurrent invocations allowed for this namespace', type=int) | |||
| subcmd.add_argument('--allowedKinds', help='list of runtime kinds allowed in this namespace', nargs='+', type=str) | |||
| subcmd.add_argument('--enableStoreActivations', help='enable storing of activations to datastore for this namespace', default=None, dest='storeActivations', action='store_true') | |||
| subcmd.add_argument('--disableStoreActivations', help='disable storing of activations to datastore for this namespace', default=None, dest='storeActivations', action='store_false') | |||
There was a problem hiding this comment.
Thinking about scriptability here: Would it be easier to just use a value storeActivations=true/false instead of two distinct flags? If I were to use wskadmin in a script, I much rather do wskadmin --storeActivations=$VALUE than replacing the whole flag if that makes sense?
There was a problem hiding this comment.
I can have another look on that one 👍
Codecov Report
@@ Coverage Diff @@
## master #4234 +/- ##
==========================================
- Coverage 86.01% 81.06% -4.95%
==========================================
Files 153 154 +1
Lines 7349 7358 +9
Branches 487 499 +12
==========================================
- Hits 6321 5965 -356
- Misses 1028 1393 +365
Continue to review full report at Codecov.
|
| .map { activation => | ||
| activationStore.store(activation, context) | ||
| } | ||
| if (user.limits.storeActivations.getOrElse(true)) { |
There was a problem hiding this comment.
I very much like the idea of consolidating the defaults --- for this and other annotations we use.
…4234) Operators will be able to disable storing of activations into activations store. The flag is implemented like the per-namespace-limits. It can be set with wskadmin. This commit was the initial idea of apache#4078. In the meantime, the idea in the other PR changed, to implement a throttle instead of a switch. But as this is a completely new type of rate-limit (which does not only allow or deny requests) that's a bit bigger to implement. So I'll go with a staged approach here and implement it as switch first. If someone needs a throttle instead of completely switching off the writes to the DB, this can be brought up again and the throttle can be built upon the solution we already have.
With this PR, operators will be able to disable storing of activations into activations store.
The flag is implemented like the per-namespace-limits. It can be set with wskadmin.
This PR was the initial idea of #4078. In the meantime, the idea in the other PR changed, to implement a throttle instead of a switch. But as this is a completely new type of rate-limit (which does not only allow or deny requests) that's a bit bigger to implement. So I'll go with a staged approach here and implement it as switch first.
If someone needs a throttle instead of completely switching off the writes to the DB, this can be brought up again and the throttle can be built up on the solution we already have.
Related issue and scope
My changes affect the following components
Types of changes
Checklist: