Changes to runtimes variables in support of retiring 'whisk/*' runtimes #3412

jonpspri · 2018-03-08T01:46:21Z

Changes to ansible processes for determining and maybe pulling runtime images, as a preliminary to eliminating the hardcoding of the 'whisk' prefix for runtime docker images.

Description

As per discussion in #3407 , this PR contains changes to ansible processes that will be needed to support the removal of the need for 'whisk/*' images to be in place for builtin runtimes. Instead,
runtimes may (do we eventually want should or must?) be specified in the runtimes.json file.

A pleasant side effect of this change is that all variables (I think) impacting runtimes have now been consolidated into the 'runtimes' YAML object in ansible/group_vars/all, and all overrides are tested and defaulted at that point. There my be some redundant defaulting remaining in ansible/roles/(controller|invoker)/tasks/deploy.yml, but I'm trying to manage the number of changes and other maintenance risks that may creep in.

Review and PG requested -- there may be breakage in the IBM Functions build.

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.

jonpspri · 2018-03-08T13:42:30Z

Yep, I see the Travis failure. Something funky going on with how the templating of 'whisk.properties' works with my changes. Need to set up a regression environment to cross-test against. Gotta go do the day job now, but will likely sort this out during the breaks.

jonpspri · 2018-03-08T22:43:17Z

^^^ Rebased in case a fix to the Kafka heisenbug has been committed.

dgrove-oss · 2018-03-20T12:18:21Z

I thought we decided to move the misc defaults to pureconfig (https://github.com/apache/incubator-openwhisk/blob/8fb3a6fda1700b8cde1de300492b1461a176df15/common/scala/src/main/resources/application.conf#L131-#L137) and removed them from ansible. Why do they need to move back to ansible? (we don't use ansible to deploy on kube, which was why moving the defaults out of ansible was desirable)

jonpspri · 2018-03-20T12:32:22Z

Hi @dgrove-oss, I missed that memo, so forgive me if I’m catching up. Currently there’s logic in the ansible deploy that pre-fetches the images listed in ‘runtime_manifest.json’ and therefore needs to know the defaults as defined in application.conf. Unfortunately, ansible doesn’t natively support parsing application.conf (one of several reasons I dislike the pure config approach). For the time being, that means I do have to keep that separate list of overrides in group_vars/all. Ideally, I’d want to do away with the pre-loading because Controller/Invoker were smart enough to initialize their own runtimes space, but I don’t think we’re there yet.

chetanmeh · 2018-03-20T13:06:11Z

Unfortunately, ansible doesn’t natively support parsing application.conf (one of several reasons I dislike the pure config approach)

@jonpspri One possible workaround to address that may be to convert the application.conf to JSON using typesafe api (see serialization)

val config = ConfigFactory.load(); 
val configJSON : String = config.root().render( ConfigRenderOptions.concise() )

And then consume that in ansible logic. We can add script to dev tools which can then be invoked from ansible via gradle command like gradlew -p tools/dev confToJson

dgrove-oss · 2018-03-20T13:58:29Z

I'd like to have just one place to get these defaults. I like @chetanmeh's suggestion of parsing application.conf into ansible so we could make application.conf available to the non-Scala parts of the system.
@markusthoemmes any thoughts?

jonpspri · 2018-03-20T14:54:51Z

I was thinking about something along @chetanmeh ‘s way of thinking. Although I’d rather go the other way... a JSON or YAML config file that is used for configuration of the scala code. Then we’re less likely to run into synchronization problems and edits/overrides are understood. I expect that’s a non-starter from an effort standpoint.

Can we leave gradle out of this solution? Ideally in this scenery, one would use ‘shell:’ to invoke an entry point in the invoker jar to dump the JSON to stdout. That way we avoid all sorts of build out-of-sync issues.

chetanmeh · 2018-03-20T15:17:53Z

Ideally in this scenery, one would use ‘shell:’ to invoke an entry point in the invoker jar to dump the JSON to stdout.

Makes sense. We can add a utility main class in common (implemented in java to avoid scala dependency in classpath) to dump the config. And in ansible just invoke

java -cp typesafe-config.jar:openwhisk-common.jar whisk.utils.ConfigDumper /path/to/output.json

jonpspri · 2018-03-22T17:11:40Z

I like that approach, assuming we need to continue to pre-pull Docker images in ansible. My preferred approach would be for Invoker to pre-pull its needed images on startup. I’m opening the covers on invoker in my next PR, so can we table this conversation until then? I’m referring #3407 here so we track this conversation. I’ll also add a TODO to the Ansible files where the suspect variables are defined and used so we don’t build in more dependencies. We can disappear them over the next few weeks while the JARs are open. This PRs intention was to do pre-JAR work. Deal?

dgrove-oss · 2018-03-22T17:25:39Z

As long as we leave the redundant defaults in application.conf so that kube deploy won't suddenly break, I'm ok with staging things and having duplication for a couple of weeks while the changes flow through.

fwiw, in the kube deployments, the pulls are done by an init container that runs before the main invoker container starts. it does the pulls by reading (a duplicated... 😬 ) runtimes.json file.

rabbah · 2018-03-20T03:17:42Z

ansible/templates/whisk.properties.j2

@@ -33,7 +33,7 @@ whisk.api.host.name={{ whisk_api_host_name | default(groups['edge'] | first) }}
 whisk.api.localhost.name={{ whisk_api_localhost_name | default(whisk_api_host_name) | default(whisk_api_localhost_name_default) }}
 whisk.api.vanity.subdomain.parts=1

-runtimes.manifest={{ runtimesManifest | to_json }}
+runtimes.manifest={{ runtimes.manifest | to_json }}


i think... we can drop this... gulp, i didn't try.

Oh that's scary. Let me dig through the invoker code -- I'm not sure how much indirection it uses to get the manifest.

This file is used for tests only not the invoker. It might still be needed to run tests actually.

I'm trying to slowly factor it out of tests. I'll take a look there. In the meantime, I guess I'll have to raise an issue so we don't lose track of it.

feel free to ignore my comment it's inconsequential for this pr.

jonpspri · 2018-03-24T19:40:37Z

@dgrove-oss Yeah, ansible is doing the pulls based on runtimes.json as well, hence some of this stuff. If we get invoker to do its own pulls, does that let you cut out a container? I'm working on the extra Java class now -- probably will have something on Monday.

dgrove-oss · 2018-03-24T20:27:23Z

it won't help with the KubernetesContainerFactory because in that architecture we don't run an invoker on every node. (We run a smaller number of invokers as a StatefulSet + an agent (small & slim; written in Go) on every worker node that handles the interactions with docker.

jonpspri · 2018-03-26T12:21:56Z

@dgrove-oss Hmm... so maybe we should eventually factor out an invoker-helper that runs in the worker pods and manages the associated image-space. Or perhaps we defer the whole docker-caching problem up to kube? We can't be the only kube users with that concern...

codecov-io · 2018-05-05T14:30:12Z

Codecov Report

Merging #3412 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #3412   +/-   ##
======================================
  Coverage    74.4%   74.4%           
======================================
  Files         125     125           
  Lines        5951    5951           
  Branches      384     384           
======================================
  Hits         4428    4428           
  Misses       1523    1523

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 6fef5c4...e2163c8. Read the comment docs.

rabbah · 2018-06-11T04:07:04Z

We've settled on #3680.

jonpspri force-pushed the runtime-prefix-tag branch from 0a9ab41 to d0b05d1 Compare March 8, 2018 22:42

jonpspri mentioned this pull request Mar 15, 2018

Create a multi-architecture Docker image for the runtime apache/openwhisk-runtime-nodejs#28

Closed

rabbah requested review from dgrove-oss and csantanapr March 20, 2018 03:16

rabbah added deployment review Review for this PR has been requested and yet needs to be done. labels Mar 20, 2018

rabbah assigned dgrove-oss Mar 20, 2018

jonpspri force-pushed the runtime-prefix-tag branch from 1de490c to d615b6e Compare March 22, 2018 17:17

rabbah reviewed Mar 22, 2018

View reviewed changes

jonpspri force-pushed the runtime-prefix-tag branch from d615b6e to fee1d4f Compare April 8, 2018 13:45

jonpspri mentioned this pull request Apr 9, 2018

Add abstraction to controller role in ansible #3534

Merged

21 tasks

jonpspri force-pushed the runtime-prefix-tag branch from fee1d4f to 8a32e2e Compare May 5, 2018 12:07

jonpspri force-pushed the runtime-prefix-tag branch from 8a32e2e to 426f56a Compare May 5, 2018 12:27

Changes to runtimes variables in support of retiring 'whisk/*' runtimes

e2163c8

jonpspri force-pushed the runtime-prefix-tag branch from 426f56a to e2163c8 Compare May 5, 2018 13:37

rabbah closed this Jun 11, 2018

jonpspri deleted the runtime-prefix-tag branch April 19, 2020 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to runtimes variables in support of retiring 'whisk/*' runtimes #3412

Changes to runtimes variables in support of retiring 'whisk/*' runtimes #3412

jonpspri commented Mar 8, 2018

jonpspri commented Mar 8, 2018

jonpspri commented Mar 8, 2018

dgrove-oss commented Mar 20, 2018 •

edited

Loading

jonpspri commented Mar 20, 2018

chetanmeh commented Mar 20, 2018

dgrove-oss commented Mar 20, 2018

jonpspri commented Mar 20, 2018

chetanmeh commented Mar 20, 2018 •

edited

Loading

jonpspri commented Mar 22, 2018

dgrove-oss commented Mar 22, 2018

rabbah Mar 20, 2018

jonpspri Mar 24, 2018

rabbah Mar 24, 2018

jonpspri Mar 26, 2018

rabbah Mar 26, 2018

jonpspri commented Mar 24, 2018

dgrove-oss commented Mar 24, 2018

jonpspri commented Mar 26, 2018

codecov-io commented May 5, 2018

rabbah commented Jun 11, 2018

Changes to runtimes variables in support of retiring 'whisk/*' runtimes #3412

Changes to runtimes variables in support of retiring 'whisk/*' runtimes #3412

Conversation

jonpspri commented Mar 8, 2018

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

jonpspri commented Mar 8, 2018

jonpspri commented Mar 8, 2018

dgrove-oss commented Mar 20, 2018 • edited Loading

jonpspri commented Mar 20, 2018

chetanmeh commented Mar 20, 2018

dgrove-oss commented Mar 20, 2018

jonpspri commented Mar 20, 2018

chetanmeh commented Mar 20, 2018 • edited Loading

jonpspri commented Mar 22, 2018

dgrove-oss commented Mar 22, 2018

rabbah Mar 20, 2018

Choose a reason for hiding this comment

jonpspri Mar 24, 2018

Choose a reason for hiding this comment

rabbah Mar 24, 2018

Choose a reason for hiding this comment

jonpspri Mar 26, 2018

Choose a reason for hiding this comment

rabbah Mar 26, 2018

Choose a reason for hiding this comment

jonpspri commented Mar 24, 2018

dgrove-oss commented Mar 24, 2018

jonpspri commented Mar 26, 2018

codecov-io commented May 5, 2018

Codecov Report

rabbah commented Jun 11, 2018

dgrove-oss commented Mar 20, 2018 •

edited

Loading

chetanmeh commented Mar 20, 2018 •

edited

Loading