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

[New Scheduler] Etcd installation & Implements EtcdClient #5031

Merged
merged 7 commits into from
Feb 9, 2021
Merged

[New Scheduler] Etcd installation & Implements EtcdClient #5031

merged 7 commits into from
Feb 9, 2021

Conversation

KeonHee
Copy link
Member

@KeonHee KeonHee commented Nov 23, 2020

Description

This pr covers the etcd installation and client implementation. And the etcd will be the coordinator of cluster members and the metadata store in the new scheduler.

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.

@KeonHee KeonHee changed the title [New Scheduler] Implements EtcdClient [New Scheduler] Etcd installation & Implements EtcdClient Nov 23, 2020
}
}

trait KeyValueStore {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be used as an SPI? If so should it be more specific than KeyValueStore and related to the scheduler with the function templates

Copy link
Member

Choose a reason for hiding this comment

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

While this trait can be used to have a new store, but most of the methods and their signatures conform to ETCD.
I feel like it is highly unlikely that we can replace it with another key-value store as it should support all functionalities such as TTL with lease/keepalive, Transaction, prefix, etc.

@@ -272,6 +272,11 @@

"CONFIG_whisk_controller_activation_pollingFromDb": "{{ controller_activation_pollingFromDb | default(true) | lower }}"

"CONFIG_whisk_etcd_hosts": "{{ etcd_connect_string }}"
Copy link
Member

Choose a reason for hiding this comment

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

Since ShardingContainerPoolBalancer does not use etcd, should we selectively configure these configurations?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the case of configuration, it would be good to add only what is needed when etcd is used in the controller & invoker. I will exclude it from this PR.

}
}

trait KeyValueStore {
Copy link
Member

Choose a reason for hiding this comment

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

While this trait can be used to have a new store, but most of the methods and their signatures conform to ETCD.
I feel like it is highly unlikely that we can replace it with another key-value store as it should support all functionalities such as TTL with lease/keepalive, Transaction, prefix, etc.

import pureconfig.loadConfigOrThrow

@RunWith(classOf[JUnitRunner])
class EtcdKvTests extends FlatSpec with ScalaFutures with Matchers {
Copy link
Member

Choose a reason for hiding this comment

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

Should we need to selectively run these tests?

@@ -73,10 +73,11 @@ ext.testSets = [
"org/apache/openwhisk/core/cli/test/**",
"org/apache/openwhisk/core/limits/**",
"org/apache/openwhisk/core/scheduler/queue/test/ElasticSearchDurationCheck*",
"org/apache/openwhisk/common/etcd/**",
Copy link
Member Author

Choose a reason for hiding this comment

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

Excluded etcd from basic unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

So we would add tests for the scheduler when all contribution is over?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. One concern is that if the test is excluded, the code is merged without going through the CI test, so I would like to see if the CI also needs a pipeline for the scheduler.

@@ -197,6 +198,14 @@ task testUnit(type: Test) {
exclude couchDbExcludes
}

task testUnitEtcd(type: Test) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The etcd test can be run with ./gradlew tests:testUnitEtcd.

@codecov-io
Copy link

codecov-io commented Feb 5, 2021

Codecov Report

Merging #5031 (ca1e806) into master (6686820) will decrease coverage by 7.66%.
The diff coverage is 3.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5031      +/-   ##
==========================================
- Coverage   82.52%   74.85%   -7.67%     
==========================================
  Files         206      210       +4     
  Lines       10006    10167     +161     
  Branches      445      440       -5     
==========================================
- Hits         8257     7611     -646     
- Misses       1749     2556     +807     
Impacted Files Coverage Δ
.../org/apache/openwhisk/core/entity/CreationId.scala 0.00% <0.00%> (ø)
...la/org/apache/openwhisk/core/etcd/EtcdClient.scala 0.00% <0.00%> (ø)
...ala/org/apache/openwhisk/core/etcd/EtcdUtils.scala 0.00% <0.00%> (ø)
...cala/org/apache/openwhisk/http/ErrorResponse.scala 89.79% <50.00%> (-0.83%) ⬇️
.../scala/org/apache/openwhisk/core/WhiskConfig.scala 95.39% <100.00%> (+0.09%) ⬆️
...core/database/cosmosdb/RxObservableImplicits.scala 0.00% <0.00%> (-100.00%) ⬇️
...ore/database/cosmosdb/cache/CacheInvalidator.scala 0.00% <0.00%> (-100.00%) ⬇️
...e/database/cosmosdb/cache/ChangeFeedConsumer.scala 0.00% <0.00%> (-100.00%) ⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala 0.00% <0.00%> (-95.85%) ⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala 0.00% <0.00%> (-93.90%) ⬇️
... and 28 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 6686820...ca1e806. Read the comment docs.

@KeonHee KeonHee closed this Feb 5, 2021
@KeonHee KeonHee reopened this Feb 5, 2021
Copy link
Member

@style95 style95 left a comment

Choose a reason for hiding this comment

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

LGTM

@style95
Copy link
Member

style95 commented Feb 8, 2021

Many subsequent modules are dependent on this module.
I would merge this in 24 hours if there is no more comment.

@style95 style95 merged commit 5eda221 into apache:master Feb 9, 2021
falkzoll added a commit to falkzoll/runtime-python that referenced this pull request Feb 10, 2021
- The scheduler PR apache/openwhisk#5031 introduced changes that required adaption in the setup of the openwhisk environment used for the automated tests.
falkzoll added a commit to falkzoll/runtime-nodejs that referenced this pull request Feb 10, 2021
- The scheduler PR apache/openwhisk#5031 introduced changes that required adaption in the setup of the openwhisk environment used for the automated tests.
somaya10 pushed a commit to ibm-functions/runtime-python that referenced this pull request Feb 10, 2021
- The scheduler PR apache/openwhisk#5031 introduced changes that required adaption in the setup of the openwhisk environment used for the automated tests.
somaya10 pushed a commit to ibm-functions/runtime-nodejs that referenced this pull request Feb 10, 2021
- The scheduler PR apache/openwhisk#5031 introduced changes that required adaption in the setup of the openwhisk environment used for the automated tests.
falkzoll added a commit to falkzoll/runtime-swift that referenced this pull request Feb 11, 2021
- The scheduler PR apache/openwhisk#5031 introduced changes that required adaption in the setup of the openwhisk environment used for the automated tests.
somaya10 pushed a commit to ibm-functions/runtime-swift that referenced this pull request Feb 11, 2021
- The scheduler PR apache/openwhisk#5031 introduced changes that required adaption in the setup of the openwhisk environment used for the automated tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants