Enable extending environment variables of Controller and Invoker #3689

chetanmeh · 2018-05-23T08:01:29Z

Provides a way to extend the environment variable set for Controller and Invoker

Description

As mentioned in #3477 for some set of deployments like those using CosmosDB we need to add some extra environment variables like CONFIG_whisk_spi_ArtifactStoreProvider. To support that ansible playbook now supports add a set of environment variables conditionally.

Usage

To add extra environment variables user need to define them under group_vars/all for specific environment

cosmos:
    CONFIG_whisk_spi_ArtifactStoreProvider: whisk.core.database.cosmosdb.CosmosDBArtifactStoreProvider
    CONFIG_whisk_cosmosdb_endpoint : "https://<account name>.documents.azure.com:443/"
    CONFIG_whisk_cosmosdb_key : "some secret"
    CONFIG_whisk_cosmosdb_db : openwhisk

Here cosmos is the name of the environment variable set related to CosmosDB. To enable this set user needs to set an environment vairable

OPENWHISK_EXTRA_ENV=cosmos

Here OPENWHISK_EXTRA_ENV refers to a comma separated list of environment variable set which need to be added. If configured then all variables defined under cosmos key would be added to env of controller and invoker

Related issue and scope

I opened an issue to propose and discuss this change (Support extending the environment variables for docker in ansible #3477)

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.

chetanmeh · 2018-05-23T08:03:20Z

ansible/roles/controller/tasks/deploy.yml

+    controller_env: "{{ controller_env | default({}) | combine(item) }}"
+  with_items: "{{ extraEnvSets.split(',') | map('trim') | map('extract', vars)| list }}"
+  loop_control:
+    label: redacted


Ansible would by default emit the item value to stdout. As the values may contain secrets set the label to a constant

codecov-io · 2018-05-23T08:35:32Z

Codecov Report

Merging #3689 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3689      +/-   ##
==========================================
+ Coverage      75%   75.02%   +0.01%     
==========================================
  Files         127      127              
  Lines        6030     6030              
  Branches      388      388              
==========================================
+ Hits         4523     4524       +1     
+ Misses       1507     1506       -1

Impacted Files	Coverage Δ
...rc/main/scala/whisk/common/ForcableSemaphore.scala	`88.46% <0%> (+3.84%)`	⬆️

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 0134f78...8fec3f5. Read the comment docs.

csantanapr · 2018-05-23T17:33:13Z

ansible/environments/local/group_vars/all

 # license agreements; and to You under the Apache License, Version 2.0.

 whisk_version_name: local
+extraEnvSets: "{{ lookup('env', 'OPENWHISK_EXTRA_ENV_SETS')|default(false, true) }}"


We should not assume that is one top level set and force on both controller and invoker
Maybe user wants to set certain config or secret that is intended for controller but not invoker.
Or you want to set the same variable ie apikey to the db and is different for both controller and invoker.

What about having namespace with option to override

invoker: extraEnv: "{{ invoker_extra_env | lookup('env', 'OPENWHISK_EXTRA_ENV') | default(false, true) }}" controller: extraEnv: "{{ controller_extra_env | lookup('env', 'OPENWHISK_EXTRA_ENV') | default(false, true) }}"

Makes sense. Update PR to support specific env for controller and invoker. By default they both rely on OPENWHISK_EXTRA_ENV. However one can change the setting in environment specific file.

Not sure on best practices for placing the vars in ansible ... for now I have added them to local/group_vars/all such that one can use a different setting in specific env

ansible/environments/custom/group_vars controller: extraEnv: "cosmos,splunk"

csantanapr · 2018-05-26T00:19:53Z

Hey @chetanmeh please review the commit I just pushed fd35871
It's was what I was proposing we do is much simpler, it allows override from an environment.
Also it does proper combine using dict in ansible

Now you are able to edit your own environment like environment/local and add something like

controller_extraEnv:
  CONFIG_whisk_cosmosdb_endpoint : "https://<account name>.documents.azure.com:443/"
  CONFIG_whisk_cosmosdb_key : "some secret"
  CONFIG_whisk_cosmosdb_db : openwhisk
invoker_extraEnv:
  CONFIG_whisk_cosmosdb_endpoint : "https://<account name>.documents.azure.com:443/"
  CONFIG_whisk_cosmosdb_key : "some secret"
  CONFIG_whisk_cosmosdb_db : openwhisk

You can do it like this ^^ or in your environment load an environment variable into the environment file, or override using ansible-playbook -e

chetanmeh · 2018-05-26T01:48:27Z

@csantanapr I needed this support for travis specially. There I want to have a way where just with a env variable switch I can make test run against Cosmos. So earlier plan was to define pre configured env set in local

cosmos:
    CONFIG_whisk_spi_ArtifactStoreProvider: whisk.core.database.cosmosdb.CosmosDBArtifactStoreProvider
    CONFIG_whisk_cosmosdb_endpoint : {{ lookup('env','COSMOSDB_ENDPOINT') }}
    CONFIG_whisk_cosmosdb_key : "{{ lookup('env','COSMOSDB_KEY') }}"
    CONFIG_whisk_cosmosdb_db : "{{ lookup('env','COSMOSDB_KEY') }}"

These env variables would then be configured via travis ui. And then in cosmos db stage of travis set

export OPENWHISK_EXTRA_ENV=cosmos

This way with minimal changes specific to Cosmos I can run test against it. Any pointers on how to achieve that with approach you proposed?

csantanapr · 2018-05-30T15:37:41Z

to bring back the discussion offline with @chetanmeh
This PR adds the ability to add extra env variables to controller and invoker

He has a seperate PR for the Cosmos DB that will leverage this but adding a way to overload the spi for db in ansible in global group vars:

controller:
  db:
    spi: "{{ controller_db_spi | default('') }}"
invoker:
  db:
    spi: "{{ invoker_db_spi | default('') }}"

Then in travis the openwhisk.yml playbook can be run with -e invoker_db_spi=whisk.core.database.cosmosdb.CosmosDBArtifactStoreProvider for the cosmos db stage.

csantanapr · 2018-05-30T15:47:57Z

PG 3 2312 🏃

markusthoemmes · 2018-05-30T19:06:11Z

ansible/roles/controller/tasks/deploy.yml

+- name: merge extra env variables
+  set_fact:
+    controller_env: "{{ controller_env | default({}) | combine(controller.extraEnv) }}"
+  when: controller.extraEnv | default(false)


Why is the when needed in the first place? Shouldn't we be able to specify: extraEnv: "{{ controller_extraEnv | default({}) }}" and run the combine step no matter what?

I assumed we need it the extra safety, your proposing just dropping the when line?
since in group_vars we already default to default({}) and extra check is not need it.

I see define the default {} and then drop the when I will give this a test and update

csantanapr · 2018-05-31T19:55:20Z

PG1 2974 ⌛️

csantanapr · 2018-06-01T17:43:13Z

PG 1 2976 🔵

…che#3689)

chetanmeh added 2 commits May 23, 2018 12:35

Enabled extending the env for controller and invoker

d9fa4ba

Add variable for env set names

dd40675

chetanmeh changed the title ~~Extend env 3477~~ Enable extending environment variables of Controller and Invoker May 23, 2018

chetanmeh commented May 23, 2018

View reviewed changes

csantanapr requested changes May 23, 2018

View reviewed changes

Use separate env for controller and invoker

b96f76f

chetanmeh mentioned this pull request May 24, 2018

ArtifactStore implementation for CosmosDB #3562

Merged

14 tasks

chetanmeh and others added 6 commits May 25, 2018 19:27

Enabled extending the env for controller and invoker

2fddd87

Add variable for env set names

4f70bce

Use separate env for controller and invoker

153e6f8

have variable in global group_vars

f8787f6

Merge remote-tracking branch 'upstream/master' into extend-env-3477

4117c6d

Merge branch 'pr-3689' into extend-env-3477

fd35871

csantanapr approved these changes May 30, 2018

View reviewed changes

dubee approved these changes May 30, 2018

View reviewed changes

markusthoemmes reviewed May 30, 2018

View reviewed changes

csantanapr added 2 commits May 31, 2018 12:34

Merge remote-tracking branch 'upstream/master' into extend-env-3477

9f3fb3e

simplify and remove when

8fec3f5

csantanapr merged commit 8b6681a into apache:master Jun 1, 2018

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019

Enable extending environment variables of Controller and Invoker (apa…

3c84317

…che#3689)

Enable extending environment variables of Controller and Invoker #3689

Enable extending environment variables of Controller and Invoker #3689

Uh oh!

Conversation

chetanmeh commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Usage

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

chetanmeh May 23, 2018

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

csantanapr May 23, 2018

Choose a reason for hiding this comment

Uh oh!

chetanmeh May 24, 2018

Choose a reason for hiding this comment

Uh oh!

csantanapr commented May 26, 2018

Uh oh!

chetanmeh commented May 26, 2018

Uh oh!

csantanapr commented May 30, 2018

Uh oh!

csantanapr commented May 30, 2018

Uh oh!

markusthoemmes May 30, 2018

Choose a reason for hiding this comment

Uh oh!

csantanapr May 31, 2018

Choose a reason for hiding this comment

Uh oh!

csantanapr May 31, 2018 • 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!

csantanapr commented May 31, 2018

Uh oh!

csantanapr commented Jun 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chetanmeh commented May 23, 2018 •

edited

Loading

codecov-io commented May 23, 2018 •

edited

Loading

csantanapr May 31, 2018 •

edited

Loading