Register services in service registry during the service bootstrap phase #4548

Kami · 2019-02-13T13:24:20Z

This pull request updates service bootstrap phase (st2common.service_setup.common_setup function) to register every StackStorm service in the service registry.

Background and Context

Registering each service in a service segistry will allow us to have a better overview of the active and running services. This will allow us to implement things such as more dynamic (utilization and capability based) action execution scheduling / routing and more.

The goal was so it also works outside of the Kubernetes context in a traditional HA environment. It's also worth noting that Kubernetes has it's own service registry concept, but that's based on a slightly higher level Kubernetes view of a service (two related concepts, but not exactly the same).

Implementation Details

Implementation utilizes group membership primitives from the OpenStack tooz library.

There were multiple reasons (as discussed before) for utilizing tooz library:

Consistency

We already use it in other places for distributed locking.

Support for multiple drivers / adapters

We will officially probably only support a single backend (e.g. ZooKeeper or etcd), but people will still be able to user other tooz supported backends at their own risk and this will also help with testing.

Keep in mind that only the following tooz drivers support group membership primitives which we utilize: zookeeper, redis, etcd3, etcd3gw, memcached.

To be able to implement it, I needed to change coordinator_setup and get_coordinator function signature so we can pass start_heartbeat argument to it. This way coordination service automatically starts periodic heartbeats which tell the backend that a particular service (member) is still alive.

In addition to that, I added tools/list_group_members.py script which allows user to list active services.

Terminology

OpenStack tooz library exposes a couple of different terms. Here I describe how they fit into our implementation.

Group

Each service is part of a single group. In our case, group represents a service type (e.g. api, actionrunner, scheduler, etc.). To view all the running services of a particular type, we just need to list all the members in a particular group.

Member

A service is represented as a member which is part of a particular group.

Capabilities

Optional metadata associated with a service (service name, listen host / port, service type, hostname, pid and other info). In the future, action runners will also utilize this to advertise their capabilities (e.g. linux, windows, etc.).

Example Output

Example output of tools/list_group_members.py script when all the services are running using launchdev.sh script (to test it locally, I used the redis backend).

$ python ./tools/list_group_members.py --config-file conf/st2.dev.conf 
Available groups (13):
 - actionrunner
 - garbagecollector
 - workflow_engine
 - resultstracker
 - stream
 - timer_engine
 - auth
 - sensorcontainer
 - api
 - scheduler
 - rulesengine
 - notifier

Members in group "actionrunner" (1):
 - localhost_21837 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21837, 'name': 'actionrunner'})
Members in group "garbagecollector" (1):
 - localhost_21841 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21841, 'name': 'garbagecollector'})
Members in group "workflow_engine" (1):
 - localhost_21834 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21834, 'name': 'workflowengine'})
Members in group "resultstracker" (1):
 - localhost_21871 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21871, 'name': 'resultstracker'})
Members in group "stream" (1):
 - localhost_21930 (capabilities={'listen_host': '127.0.0.1', 'name': 'stream', 'listen_port': 9102, 'hostname': 'localhost', 'pid': 21930, 'type': 'active'})
Members in group "timer_engine" (1):
 - localhost_21853 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21853, 'name': 'timerengine'})
Members in group "auth" (1):
 - localhost_21927 (capabilities={'listen_host': '127.0.0.1', 'name': 'auth', 'listen_port': 9100, 'listen_ssl': False, 'hostname': 'localhost', 'pid': 21927, 'type': 'active'})
Members in group "sensorcontainer" (1):
 - localhost_21849 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21849, 'name': 'sensorcontainer'})
Members in group "api" (1):
 - localhost_21925 (capabilities={'listen_host': '127.0.0.1', 'name': 'api', 'listen_port': 9101, 'hostname': 'localhost', 'pid': 21925, 'type': 'active'})
Members in group "scheduler" (1):
 - localhost_21846 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21846, 'name': 'scheduler'})
Members in group "rulesengine" (1):
 - localhost_21850 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21850, 'name': 'rulesengine'})
Members in group "notifier" (1):
 - localhost_21870 (capabilities={'type': 'passive', 'hostname': 'localhost', 'pid': 21870, 'name': 'notifier'})

TODO

Changelog entry
Tests

Resolves #4015.

members with their capabilities.

"service_setup" function.

functionality.

Add or move the parsing of test configs to the top of affected test modules and make sure the scheduler default config options do not conflict with test configs.

m4dcoder

LGTM overall. Good Job @Kami. I assume unit tests are coming.

m4dcoder · 2019-02-13T19:15:28Z

tools/list_group_members.py

+    do_register_cli_opts(cli_opts)
+    config.parse_args()
+
+    main(group_id=cfg.CONF.group_id)


Should we add an admin only API endpoint and a command in the st2 CLI to list services, members and capabilities?

That's a good idea - I was thinking about it.

I will add it and also add a corresponding CLI command.

m4dcoder · 2019-02-13T19:20:13Z

st2common/st2common/service_setup.py

    metrics_initialize()

+    # Register service in the service registry
+    if service_registry:


Should we add a config check here? If user specifies a backend with no service registry support, we log and warn users and start st2 w/o service registry?

Did you address this? Or is this a bad idea?

We already log a warning when get_coordinator is called inside register_service_in_service_registry function.

1. ``GET /v1/service_registry/groups`` - list available groups. 2. ``GET /v1/service_registry/groups/<group id>/members`` - list available / active members in a particular group. NOTE: This functionality is only accessible to RBAC admins.

easier to test.

nmaludy · 2019-02-19T02:43:01Z

Should the heartbeat be started for the coordinator/locking driver instances? I believe in some implementations that if locks don't get a heartbeat then they are released.

Kami · 2019-03-03T03:33:46Z

@nmlaudy I think you are correct, good catch 👍

I will also make that change, it will likely require test changes though. I already have some issues with this change (tests are getting stuck, even though I correctly shut down the coordinator everywhere, could also be a bug in a test driver we use).

use coordinator backend for locking purpose. This way it's safe and also works correctly with drivers which are timeout based.

Kami · 2019-03-11T16:05:22Z

Just a heads up - I also pushed a change so we start heartbeat process also in places where we use coordination backend only for locking - 98b77d1.

This is a safer default since it means it will work correctly with backends which are timeout based.

…es where we" This reverts commit 98b77d1.

nmaludy · 2019-03-12T11:21:09Z

Closes #4015

… in places where we"" This reverts commit f530cbe.

m4dcoder

LGTM overall but got some minor things that I like you to address.

m4dcoder · 2019-03-14T17:55:39Z

CHANGELOG.rst

 * Fix improper CORS where request from an origin not listed in ``allowed_origins`` will be responded
  with ``null`` for the ``Access-Control-Allow-Origin`` header. The fix returns the first of our
-  allowed origins if the requesting origin is not a supported origin. Reported by Barak Tawily.
+  Bllowed origins if the requesting origin is not a supported origin. Reported by Barak Tawily.


Please fix typo here.

m4dcoder · 2019-03-14T18:00:02Z

st2api/st2api/controllers/v1/service_registry.py

+        coordinator = coordination.get_coordinator()
+
+        group_ids = list(coordinator.get_groups().get())
+        group_ids = [group_id_.decode('utf-8') for group_id_ in group_ids]


Is the underscore at the end of group_id_ mean something?

Yes, group_id variable is already defined in the method signature and I don't want to re-define / overwrite it in a list comprehension.

Practice wise, we don't usually put an underscore at the end of a variable. This is confusing and error prone. Can you rename group_id_ to something else? Maybe item just for this list comprehension.

m4dcoder · 2019-03-14T18:23:52Z

st2client/st2client/models/service_registry.py

+
+class ServiceRegistry(core.Resource):
+    _alias = 'service-registry'
+    _display_name = 'service registry'


Please capitalize first letters.

This attribute is really just a place holder, it's not used anywhere.

The class (model) is just needed to get st2 service-registry foo bar command structure to work since our, not so ideal CLI code, depends on it.

I can remove unused attributes all together.

Ok these attributes are still here. So, placeholder or not, please capitalize service registry to Service Registry. This is so the values are consistent with other models.

m4dcoder · 2019-03-14T18:35:17Z

st2common/st2common/service_setup.py

    metrics_initialize()

+    # Register service in the service registry
+    if service_registry:


Did you address this? Or is this a bad idea?

m4dcoder · 2019-03-14T18:39:08Z

st2common/st2common/services/coordination.py

        return True


+class NoOpAsyncResult(object):


Can you provide a short description why we need to wrap result with this class?

It's to correctly mimic tooz API - most of the tooz methods return an async result (future).

I can add that as a docstring.

m4dcoder · 2019-03-14T18:41:40Z

st2common/st2common/services/coordination.py


+    if not use_cache:
+        coordinator = coordinator_setup(start_heart=start_heart)
+        return coordinator


return in one line? return coordinator_setup(start_heart=start_heart)

That's a personal preference - I can prefer return in a separate line.

Ok. My suggestion here is because we are not doing anything with the variable coordinator so why not just return here. This is also 1 line of code less. What's the reasoning for your preference?

m4dcoder · 2019-03-14T18:43:55Z

st2common/st2common/service_setup.py

+    # Include common capabilities such as hostname and process ID
+    proc_info = system_info.get_process_info()
+    capabilities['hostname'] = proc_info['hostname']
+    capabilities['pid'] = proc_info['pid']


Doesn't get_member_id already included this info? Maybe doesn't matter much, but seems like we're calling system_info.get_process_info twice.

Nope, this function doesn't include member info. Just hostname and pid.

m4dcoder · 2019-03-14T18:46:59Z

tools/launchdev.sh

    fi

    VIRTUALENV=${VIRTUALENV_DIR:-${ST2_REPO}/virtualenv}
+    VIRTUALENV=$(readlink -f ${VIRTUALENV})


I assume this is to accommodate symlink in your dev env. Will this break existing setup for us?

It's so an absolute path is used everywhere (just resolves relative path to an absolute one). Everything should still work the same.

m4dcoder · 2019-03-14T18:47:08Z

tools/launchdev.sh

        ST2_CONF=${ST2_REPO}/conf/st2.dev.conf
    fi
+
+    ST2_CONF=$(readlink -f ${ST2_CONF})


I assume this is to accommodate symlink in your dev env. Will this break existing setup for us?

Resolved.

instance.

when we create a new instance.

m4dcoder · 2019-03-18T19:24:59Z

st2api/st2api/controllers/v1/service_registry.py

+        coordinator = coordination.get_coordinator()
+
+        group_ids = list(coordinator.get_groups().get())
+        group_ids = [group_id_.decode('utf-8') for group_id_ in group_ids]


Practice wise, we don't usually put an underscore at the end of a variable. This is confusing and error prone. Can you rename group_id_ to something else? Maybe item just for this list comprehension.

m4dcoder · 2019-03-18T19:32:01Z

st2client/st2client/models/service_registry.py

+
+class ServiceRegistry(core.Resource):
+    _alias = 'service-registry'
+    _display_name = 'service registry'


Ok these attributes are still here. So, placeholder or not, please capitalize service registry to Service Registry. This is so the values are consistent with other models.

m4dcoder · 2019-03-18T19:35:23Z

st2common/st2common/services/coordination.py


+    if not use_cache:
+        coordinator = coordinator_setup(start_heart=start_heart)
+        return coordinator


Ok. My suggestion here is because we are not doing anything with the variable coordinator so why not just return here. This is also 1 line of code less. What's the reasoning for your preference?

Kami · 2019-03-18T20:17:53Z

@m4dcoder

Practice wise, we don't usually put an underscore at the end of a variable. This is confusing and error prone. Can you rename group_id_ to something else? Maybe item just for this list comprehension.

That's a relatively common pattern in Python world. Also when variable names would clash with built-ins such as id, etc.

Kami added 7 commits February 13, 2019 10:43

Add utility script for listing tooz / service registry groups and

3625ecd

members with their capabilities.

Add support for registering a service in service registry inside

b95e7c8

"service_setup" function.

Fix lint and Python 3 compatibility.

eb56273

Make sure we register all the services in the service registry.

293757a

Update config with sample configuration for coordination section.

079448c

Update NoOpCoordinationBackend so it correctly works with group

6d2d7d9

functionality.

Fix lint.

46d7f56

Kami added feature HA StackStorm in High Availability labels Feb 13, 2019

Kami added this to the 3.0.0 milestone Feb 13, 2019

Kami requested a review from m4dcoder February 13, 2019 13:24

m4dcoder and others added 2 commits February 13, 2019 14:58

Fix scheduler test configs in unit tests

b7e618f

Add or move the parsing of test configs to the top of affected test modules and make sure the scheduler default config options do not conflict with test configs.

Add a changelog entry.

09ed1b5

m4dcoder reviewed Feb 13, 2019

View reviewed changes

Kami added 10 commits February 14, 2019 16:58

Merge branch 'master' of github.com:StackStorm/st2 into service_registry

9e2fb82

Refactor it in a separate function so it's more self contained and

5141fc7

easier to test.

Add test cases for new service registry related API endpoint.

b4f1f13

Add changelog entry.

ed9b270

Add new service registry endpoints to RBAC API endpoint test cases.

af231a0

Only tear down the coordination service.

cead842

Add RBAC test cases for the new service regstry API endpoints.

de605d8

Re-generate openapi config.

76406ff

Update comment, fix lint.

4963266

Kami changed the title ~~[WIP] Register services in service registry during the service bootstrap phase~~ Register services in service registry during the service bootstrap phase Feb 15, 2019

Merge branch 'master' of github.com:StackStorm/st2 into service_registry

9f94334

Kami added 2 commits March 2, 2019 20:34

Always refer to the same coordinator instance.

57ad129

Use NoOp driver for tests since zake one causes some tests go hang.

7fa0934

Kami added 5 commits March 11, 2019 14:51

Make sure we correctly tear down the coordinator.

dd27733

Fix Python 3 related test failures.

2c113b9

Use full absolute paths in launchdev script.

32a4d46

Make sure we also start coordinator heartbeat process in places where we

98b77d1

use coordinator backend for locking purpose. This way it's safe and also works correctly with drivers which are timeout based.

Use more compatible function.

f5f5116

Kami added 3 commits March 11, 2019 17:28

Revert "Make sure we also start coordinator heartbeat process in plac…

f530cbe

…es where we" This reverts commit 98b77d1.

Update affected test.

5ccf4ba

Update sample config.

4ebcdc3

nmaludy mentioned this pull request Mar 12, 2019

Coordinator errors if session expires #4015

Closed

Revert "Revert "Make sure we also start coordinator heartbeat process…

ed37c02

… in places where we"" This reverts commit f530cbe.

m4dcoder previously requested changes Mar 14, 2019

View reviewed changes

Kami added 3 commits March 18, 2019 14:28

Merge branch 'master' of github.com:StackStorm/st2 into service_registry

fc5150a

Fix typo.

2f481e0

Add docstring which clarifies the behavior.

893cfcc

Kami added 3 commits March 18, 2019 15:15

Only teardown coordinator if one has been initialized.

e4cfa82

Make sure we start heartbeat process everywhere we retrieve coordinator

639e708

instance.

To be on the safe side default start_heart to True and log a message

3c82ff4

when we create a new instance.

m4dcoder requested changes Mar 18, 2019

View reviewed changes

Use consistent casing.

5c6cc5d

Remove assignment.

100ad9f

m4dcoder approved these changes Mar 18, 2019

View reviewed changes

Use item variable name instead of group_id_.

7fffa52

Kami merged commit a2f356a into master Mar 19, 2019

Kami deleted the service_registry branch March 19, 2019 08:14

arm4b mentioned this pull request May 19, 2020

Proposed design to address Polling Sensor HA StackStorm/community#21

Open

arm4b mentioned this pull request Jan 6, 2022

Add required dependency to support etcd3gw driver. #5528

Open

Uh oh!

Register services in service registry during the service bootstrap phase #4548

Register services in service registry during the service bootstrap phase #4548

Uh oh!

Conversation

Kami commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background and Context

Implementation Details

Terminology

Example Output

TODO

Uh oh!

m4dcoder left a comment

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!

nmaludy commented Feb 19, 2019

Uh oh!

Kami commented Mar 3, 2019

Uh oh!

Kami commented Mar 11, 2019

Uh oh!

nmaludy commented Mar 12, 2019

Uh oh!

m4dcoder left a comment

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!

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!

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!

Kami commented Mar 18, 2019

Kami commented Feb 13, 2019 •

edited

Loading