Implement an ElasticSearchActivationStore by jiangpengcheng · Pull Request #4724 · apache/openwhisk

jiangpengcheng · 2019-11-14T05:11:50Z

Implement an ElasticSearchActivationStore

Description

Although currently we can use ArtifactWithFileStorageActivationStore to store activations into ES, but this need configure logstash manually, and it will still save activations in CouchDb

This PR will save activations into ElasticSearch directly, an then CouchDb will only serve for subjects&actions, someone may need this

It also provides ansible scripts to deploy a simple ES cluster for test/dev purpose

The activation stored in ES is like below(the _source field):

{
  "_index": "openwhisk-test_@shared_1911",
  "_type": "_doc",
  "_id": "jiangpengcheng/6138d67781a34b9fb8d67781a35b9f80",
  "_version": 1,
  "_score": 11.635652,
  "_source": {
    "@timestamp": "2019-11-20T06:57:04.282Z",
    "activationId": "6138d67781a34b9fb8d67781a35b9f80",
    "annotations": {
      "path": "jiangpengcheng/hello",
      "waitTime": 78,
      "kind": "nodejs:6",
      "timeout": false,
      "limits": {
        "concurrency": 1,
        "logs": 1,
        "memory": 256,
        "timeout": 60000
      },
      "initTime": 42
    },
    "duration": 57,
    "end": 1574233024339,
    "entityType": "activation",
    "logs": [
      "2019-11-20T06:57:04.337733554Z stdout: undefined"
    ],
    "name": "hello",
    "namespace": "jiangpengcheng",
    "path": "jiangpengcheng/hello",
    "publish": false,
    "response": {
      "result": "{}",
      "statusCode": 0
    },
    "start": 1574233024282,
    "subject": "jiangpengcheng",
    "updated": 1574233024340,
    "version": "0.0.1"
  },
  "fields": {
    "@timestamp": [
      "2019-11-20T06:57:04.282Z"
    ]
  },
  "highlight": {
    "namespace": [
      "@kibana-highlighted-field@jiangpengcheng@/kibana-highlighted-field@"
    ]
  }
}

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

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.

style95 · 2019-11-14T05:20:33Z

FTR, with this change, we can take advantage of ElasticSearch.
So we can do full-text searching against Activation including logs, drawing graphs or diagrams with Kibana.
And users can have their own custom dashboards with Kibana and so on.

style95 · 2019-11-14T06:20:13Z

+    dir:
+      become: "{{ elastic_dir_become | default(false) }}"
+    base_volume: "{{ elastic_base_volume | default('esdata') }}"
+    cluster_name: "{{ elastic_cluster_name | default('lambda') }}"


this should be changed.

style95 · 2019-11-14T06:25:48Z

@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more contributor


We don't use the short version of the Apache license header anymore.

jiangpengcheng · 2019-11-14T06:37:14Z

+    with ActivationStoreBehavior {
+
+  override def checkDeleteActivation(activation: WhiskActivation)(implicit transid: TransactionId): Unit = {
+    retry(super.checkDeleteActivation(activation), 10)


sometimes delete will get a not found error even we can get a 200 response with a get, didn't know the reason

jiangpengcheng · 2019-11-14T06:39:58Z

+export ELASTIC_INDEX_PATTERN="unittest-%s"
+export ELASTIC_USERNAME="admin"
+export ELASTIC_PASSWORD="admin"
+


I'm not sure whether it is ok to add codes here, but without these lines, tests for ElasticSearchActivationStore will be skipped and it can introduce some risk to the system

For test we can also look into using TestContainers ElasticSearch support. Then you would not need to deploy an ES container via ansible just for test runs

thanks, this looks great! I will look into it

while using TestContainers, the gradle process will be time out cause of GC overhead limit exceed issue

@jiangpengcheng could you please give more details? Even better if you can push a branch.
Testcontainers is not very memory greedy, so I believe this is something else

@bsideup sorry to misleading you, actually the error is related to my environment, after do ./gradlew clean, it worked

jiangpengcheng · 2019-11-14T06:42:21Z

+    new Batcher(500, maxOpenDbRequests)(doStore(_)(TransactionId.dbBatcher))
+
+  private val minStart = 0L
+  private val maxStart = Instant.now.toEpochMilli + 100L * 365 * 24 * 60 * 60 * 1000 //100 years from now


used for range query when since or upto is a None

jiangpengcheng · 2019-11-14T06:52:26Z

+  }
+
+  private def generateIndex(namespace: String): String = {
+    elasticSearchConfig.indexPattern.dropWhile(_ == '/') format namespace.toLowerCase


we can provide a custom pattern to store activations in special indices

for example, with pattern "openwhisk-%s", activations in different namespaces will be saved in to different indices

or use "openwhisk" with out a "%s", then all activations will be saved into the "openwhisk" index

we can even use index alias like:

openwhisk-whisk.system(this is an alias and below are indices it connects to)

openwhisk-whisk.system_201911(write index)(activations will be write to this index only while list/get activations search all indices)

openwhisk-whisk.system_201910

openwhisk-whisk.system_201909

openwhisk-ns1

openwhisk-ns1_201911(write index)

openwhisk-ns1_201910

openwhisk-ns1_201909

openwhisk-ns2(we can use shared indices for namespaces which are not very active)

openwhisk@shared_201911(write index)

openwhisk@shared_201910

openwhisk@shared_201909

openwhisk-ns3

openwhisk@shared_201911(write index)

openwhisk@shared_201910

openwhisk@shared_201909

codecov-io · 2019-11-15T10:22:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@809514e). Click here to learn what that means.
The diff coverage is 70%.

@@            Coverage Diff            @@
##             master    #4724   +/-   ##
=========================================
  Coverage          ?   78.62%           
=========================================
  Files             ?      198           
  Lines             ?     9156           
  Branches          ?      631           
=========================================
  Hits              ?     7199           
  Misses            ?     1957           
  Partials          ?        0

Impacted Files	Coverage Δ
...openwhisk/common/tracing/OpenTracingProvider.scala	`21.15% <ø> (ø)`
...che/openwhisk/core/yarn/YARNContainerFactory.scala	`87.5% <ø> (ø)`
...e/loadBalancer/ShardingContainerPoolBalancer.scala	`88.7% <ø> (ø)`
...che/openwhisk/core/loadBalancer/LeanBalancer.scala	`0% <ø> (ø)`
...apache/openwhisk/core/entitlement/Collection.scala	`87.5% <ø> (ø)`
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`7.69% <ø> (ø)`
...isk/core/controller/actions/PrimitiveActions.scala	`87.24% <ø> (ø)`
...rg/apache/openwhisk/core/controller/Triggers.scala	`94.16% <ø> (ø)`
.../openwhisk/core/entity/ActivationEntityLimit.scala	`100% <ø> (ø)`
.../scala/org/apache/openwhisk/core/entity/Exec.scala	`90.62% <ø> (ø)`
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 809514e...34b7e27. Read the comment docs.

chetanmeh

Did a first pass on the code part (excluding the ansible setup). Looks good! Wanted to confirm if caching support is actually needed for activation store?

chetanmeh · 2019-11-19T05:23:19Z

+          ErrorLevel))
+  }
+
+  override def delete(activationId: ActivationId, context: UserContext)(


Later we should review the usage for delete as activation records are immutable and removed via expiry instead of explicit removal. So technically we can drop the delete support

chetanmeh · 2019-11-19T05:35:22Z

Would be useful to have the actual record structure in ElasticSearch documented as part of PR description

style95 · 2019-11-29T09:22:01Z

@jiangpengcheng Could you address @chetanmeh's comment?
#4724 (comment)

style95 · 2019-12-10T03:37:47Z

@chetanmeh Any other opinion on this?

rabbah · 2019-12-20T22:01:20Z

This is fantastic!

chetanmeh · 2019-12-23T05:03:57Z

@chetanmeh Any other opinion on this?

Missed reviewing it. Added few more comments

style95 · 2019-12-23T05:11:25Z

@chetanmeh Thank you for the comments.

…cisearch

style95 · 2020-01-16T02:26:18Z

@jiangpengcheng Could you handle the review points?

jiangpengcheng · 2020-01-19T09:19:28Z

already updated @style95

style95

LGTM

chetanmeh

Mostly looks good. Added few more comments.

Looking at coverage report one thing looks odd

2 lines which do not have any coverage somehow indicates that flow hits the Try part first but due to some exception fallbacks to else part. Is that some issue on coverage side or test only exercise one part of the flow?

chetanmeh · 2020-01-20T06:08:49Z

+            start,
+            s"[PUT] 'activations' failed to put document: '${activation.docid}'; http status: '${res.status}'",
+            ErrorLevel)
+          throw new Exception("Unexpected http response code: " + res.status)


Better to use a specific exception here. In general when store impls throw exception they throw exception extending ArtifactStoreException such that in StoreUtils.reportFailure those exception are not rehandled. In current impl transid.failed would be invoked twice and thus would result in wrong metrics being recorded

oh, great point, sorry I missed it

about the odd coverage report, currently it only apply unittests from org.apache.openwhisk.core.database.test.behavior.ActivationStoreBehavior, in which mock activations' annotations and response.result fields are all null, so Try will failed and fallbacks to else part, I will add some fake result and annotations to mock activations

chetanmeh · 2020-01-24T05:08:00Z

@jiangpengcheng Thanks for taking care of the review feedback. Overall (barring ansible part which I was not able to have a closer look) it looks good to me.

Feel free to merge once you are done with this

rabbah

do you mind adding a short set of instructions for how to try this with a local deployment?

rabbah · 2020-01-25T16:50:28Z

 {{ docker_machine_ip }}     ansible_host={{ docker_machine_ip }}

+[elasticsearch:children]
+db


what's the meaning of db here?

That means elasticsearch nodes will be deployed on the same hosts specified in the db section.

jiangpengcheng · 2020-02-11T05:40:04Z

do you mind adding a short set of instructions for how to try this with a local deployment?

sorry for the late response, shall I write the instruction to a document or in this PR's description?

style95 · 2020-02-11T05:42:05Z

@jiangpengcheng I think we anyway need a document to describe how to use it.

rabbah · 2020-02-19T13:54:26Z

Thanks for adding the instructions. I have restarted Travis.

rabbah

LGTM. Remove vagrant change since we removed all vagrant mentions in the project. Also did you consider adding a new target for wskdev to start es?

style95 · 2020-02-27T02:05:11Z

It seems it is ready to merge.

Implement an ElasticSearchActivationStore

9a46d82

style95 reviewed Nov 14, 2019

View reviewed changes

Remove short headers and mismatched comments

7206fa5

jiangpengcheng commented Nov 14, 2019

View reviewed changes

jiangpengcheng closed this Nov 15, 2019

jiangpengcheng reopened this Nov 15, 2019

jiangpengcheng added 2 commits November 18, 2019 10:00

typo fix

959b822

Use TestContainers

7d3f894

jiangpengcheng closed this Nov 18, 2019

jiangpengcheng reopened this Nov 18, 2019

chetanmeh reviewed Nov 19, 2019

View reviewed changes

jiangpengcheng and others added 6 commits November 20, 2019 14:53

Neat changes

782e2c1

Merge branch 'master' into feature/add_elastcisearch

36db83f

Update build.gradle

0f280fa

Fix TestContainers

93a38fb

fix mistake

17d973b

Merge branch 'master' into feature/add_elastcisearch

2fb586f

chetanmeh reviewed Dec 23, 2019

View reviewed changes

Comment thread ansible/roles/invoker/tasks/deploy.yml

jiangpengcheng added 3 commits December 23, 2019 14:45

Remove cache and add configuration example

809514e

Merge remote-tracking branch 'upstream/master' into feature/add_elast…

1a77e22

…cisearch

Fix pureconfig

747e4fd

style95 approved these changes Jan 20, 2020

View reviewed changes

chetanmeh approved these changes Jan 20, 2020

View reviewed changes

jiangpengcheng added 2 commits January 23, 2020 09:49

Apply review comments

f36b835

Add result and annotations to mock WhiskActivations

811b602

rabbah reviewed Jan 25, 2020

View reviewed changes

Add instructions about how to use it

047e7a3

rabbah reviewed Feb 19, 2020

View reviewed changes

Comment thread ansible/README.md Outdated

Comment thread ansible/README.md Outdated

Comment thread ansible/README.md Outdated

Comment thread ansible/README.md Outdated

Comment thread ansible/README.md Outdated

rabbah approved these changes Feb 19, 2020

View reviewed changes

Comment thread ansible/environments/vagrant/hosts Outdated

jiangpengcheng added 2 commits February 20, 2020 10:15

Update ansible/README.md

3f38f19

Remove vagrant changes

541fce1

jiangpengcheng force-pushed the feature/add_elastcisearch branch from b8e392e to 541fce1 Compare February 20, 2020 02:22

jiangpengcheng added 2 commits February 20, 2020 11:45

Add elasticsearch to redo and remove whitespace

46957d7

Fix tests

34b7e27

style95 merged commit 05ed4e1 into apache:master Mar 3, 2020

ningyougang mentioned this pull request Jul 16, 2020

Add elasticsearch apache/openwhisk-deploy-kube#620

Merged

3 tasks

		@@ -0,0 +1,20 @@
		# Licensed to the Apache Software Foundation (ASF) under one or more contributor

Uh oh!

Conversation

jiangpengcheng commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

style95 commented Nov 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangpengcheng Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chetanmeh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chetanmeh commented Nov 19, 2019

Uh oh!

style95 commented Nov 29, 2019

Uh oh!

style95 commented Dec 10, 2019

Uh oh!

rabbah commented Dec 20, 2019

Uh oh!

Uh oh!

chetanmeh commented Dec 23, 2019

Uh oh!

style95 commented Dec 23, 2019

Uh oh!

style95 commented Jan 16, 2020

Uh oh!

jiangpengcheng commented Jan 19, 2020

Uh oh!

style95 left a comment

Choose a reason for hiding this comment

Uh oh!

chetanmeh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangpengcheng Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jiangpengcheng commented Nov 14, 2019 •

edited

Loading

jiangpengcheng Nov 14, 2019 •

edited

Loading

codecov-io commented Nov 15, 2019 •

edited

Loading

jiangpengcheng Jan 23, 2020 •

edited

Loading