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

Add support for dynamic configuration updates #521

Closed
wants to merge 4 commits into from

Conversation

kevin-bates
Copy link
Contributor

@kevin-bates kevin-bates commented May 2, 2019

Long-running, service-oriented, applications should be able to have their configurations updated without requiring restarts. These changes add methods that support periodic checks of previously loaded configuration files for changes and updates configuration-based traitlets of registered Configurable instances. Examples where dynamic updates can be useful is in toggling log levels or updated whitelists.

This PR will remain a WIP until the following have been completed (hoping for some feedback prior to moving on):

  • Functional code completed, ready for review
  • Test code completed
  • Documentation added

Applications wishing to utilize this must first register the Configurable instances that are participating in dynamic updates using Application.add_dynamic_configurable(name, Configurable). The name is for informational purposes but it is suggested to use the traitlets class prefix for its configuration entry. For example, if c.MappingKernelManager.cull_idle_timeout = 300 is in the configuration file and the application wishes to monitor traitlets on its multi kernel manager class instance, they would use a name of MappingKernelManager and register the Configurable instance using:

    self.add_dynamic_configurable('MappingKernelManager', self.kernel_manager)

It is expected that Application.update_dynamic_configurations() be called from a periodicCallback, but that's up to the application.

Caveats:

  1. Command-line options (or those set via environment variables) will always override file-based options. As a result, those options will not have their values updated when configuration file changes are detected. Therefore, applications wishing to utilize this feature should encourage the use of configuration files.
  2. Some configurable traitlets, although updated, will not have their updated values reflected in the application if those values are used to initialize other entities or the traitlet's value is copied and used via another variable. For example, c.MappingKernelManager.cull_interval is used to configure a periodic callback of that interval. Updates to cull_interval will not be reflected unless the periodic callback is stopped and re-created. As a result, applications must be aware and implement the @observe callback to reset or reinitialize such entities in order to make those options dynamic.

Example usage:
Here's an example usage from Jupyter Enterprise Gateway. This method registers the configurables of interest and initializes a periodic callback for update_dynamic_configurables(). It also logs the set of CLI-based options that will not be privy to dynamic updates.

    def init_dynamic_updates(self):
        """
        Initialize the set of configurables that should participate in dynamic updates.  We should
        also log that we're performing dynamic configration updates, along with the list of CLI
        options - that are not privy to dynamic updates.
        :return:
        """
        if self.dynamic_config_interval > 0:
            self.add_dynamic_configurable('EnterpriseGatewayApp', self)
            self.add_dynamic_configurable('MappingKernelManager', self.kernel_manager)
            self.add_dynamic_configurable('KernelSpecManager', self.kernel_spec_manager)
            self.add_dynamic_configurable('KernelSessionManager', self.kernel_session_manager)

            self.log.info("Dynamic updates have been configured.  Checking every {} seconds.".
                          format(self.dynamic_config_interval))

            self.log.info("The following configuration options will not be subject to dynamic updates:")
            for config, options in self.cli_config.items():
                for option, value in options.items():
                    self.log.info("    '{}.{}': '{}'".format(config, option, value))

            if self.dynamic_config_poller is None:
                self.dynamic_config_poller = ioloop.PeriodicCallback(self.update_dynamic_configurables,
                                                                     self.dynamic_config_interval * 1000)
            self.dynamic_config_poller.start()

Output at application startup:

[I 2019-05-02 06:45:46.835 EnterpriseGatewayApp] Dynamic updates have been configured.  Checking every 60 seconds.
[I 2019-05-02 06:45:46.836 EnterpriseGatewayApp] The following configuration options will not be subject to dynamic updates:
[I 2019-05-02 06:45:46.837 EnterpriseGatewayApp]     'EnterpriseGatewayApp.ip': '0.0.0.0'
[I 2019-05-02 06:45:46.838 EnterpriseGatewayApp]     'EnterpriseGatewayApp.port': '8889'
[I 2019-05-02 06:45:46.839 EnterpriseGatewayApp]     'EnterpriseGatewayApp.port_retries': '0'
[I 2019-05-02 06:45:46.839 EnterpriseGatewayApp]     'EnterpriseGatewayApp.yarn_endpoint': 'http://node-1:8088/ws/v1/cluster'
[I 2019-05-02 06:45:46.841 EnterpriseGatewayApp]     'EnterpriseGatewayApp.remote_hosts': '['node-1','node-2','node-3']'
[I 2019-05-02 06:45:46.843 EnterpriseGatewayApp]     'KernelSessionManager.enable_persistence': 'True'

Because user's may want to adjust the polling interval for dynamic config updates, here's an example of an @observe callback that adjusts the period (again from Enterprise Gateway).

    @observe('dynamic_config_interval')
    def dynamic_config_interval_changed(self, event):
        prev_val = event['old']
        self.dynamic_config_interval = event['new']
        if self.dynamic_config_interval != prev_val:
            # Values are different.  Stop the current poller.  If new value is > 0, start a poller.
            if self.dynamic_config_poller:
                self.dynamic_config_poller.stop()
                self.dynamic_config_poller = None

            if self.dynamic_config_interval <= 0:
                self.log.warning("Dynamic configuration updates have been disabled and cannot be re-enabled "
                                 "without restarting Enterprise Gateway!")
            elif prev_val > 0:  # The interval has been changed, but still positive
                self.init_dynamic_configurables()  # Restart the poller

Long-running, service-oriented, applications should be able to have their
configurations updated without requiring restarts.  These changes add methods
that support periodic checks of previously loaded configuration files for
changes and updates configuration-based traitlets of registered Configurable
instances.
@kevin-bates kevin-bates changed the title [WIP] Add support for dynamic configuration updates Add support for dynamic configuration updates May 3, 2019
@rmorshea rmorshea requested a review from minrk May 3, 2019 22:05
@rmorshea
Copy link
Contributor

rmorshea commented May 3, 2019

@kevin-bates would it be possible to mark configurable traits as dynamic via the tag method?

class MyApp:
    attr = Int().tag(config=True, sync_config=True)

@kevin-bates
Copy link
Contributor Author

@rmorshea - TBH I'm not intimately familiar with traitlets. Are you trying to make explicit which configurable traits should be included in dynamic updates as opposed to all? I would not want to end up in a situation where some configurable trait requires dynamic behavior and users would be prevented from doing that w/o a code change.

@rmorshea
Copy link
Contributor

rmorshea commented May 3, 2019

@kevin-bates Could you inherit from the application and override the trait with a new one that has the sync_config=True tag? To get all traits that should be synced with a config you could then do:

app.traits(sync_config=True)

Maybe related to: #429

@rmorshea
Copy link
Contributor

rmorshea commented May 3, 2019

BTW calling self.add_dynamic_configurable('EnterpriseGatewayApp', self) adds a circular reference. You should probably wrap self in a weakref.proxy to avoid that.

@kevin-bates
Copy link
Contributor Author

@rmorshea - thanks for your comments. I think they've created more questions for me.

  1. sync_config. Does that exist already? I'm not seeing it in the same places I see config. Is the name sync_config arbitrary and, itself dynamic - because it's a tag and that's how tag work? Or are you suggesting a new tag value get introduced?
  2. I'm only interested in the configurable class instances because its on those instances the configurable traits reside and require updates. I understand that Configurables are traitlets themselves, but I'd like the application to perform the registration.
  3. I like the idea that app.traits(sync_config=True) returns the interesting traits but, again, I'd like existing applications to be able to leverage this for all config=True traits. Doesn't use of sync_config imply that each trait of the application and its managed configurables be explicitly updated in order to participate? Or am I missing something.
  4. Good point on the weakref comment. I'll look into adding that. Since I believe this applies to the configurable instances as well, that weakref wrapping could be applied in the registration method.

I appreciate you taking the time to guide me through this portion of things.

@kevin-bates
Copy link
Contributor Author

FWIW, I can move these changes to outside the traitlets package if others feel these are not a fit. I would only need to create method for returning self._loaded_config_files. Since these changes are targeting Jupyter applications, JupyterApp may be a better fit.

@rmorshea
Copy link
Contributor

rmorshea commented May 8, 2019

@kevin-bates I think I understand what you're trying to do now, and it may go beyond the scope of Traitlets. That said I think this is a very useful feature and may make sense in a downstream library (e.g. https://github.com/jupyter/notebook).

If we were to do this in Traitlets I think you'd want to implement it at the level of the Configurable class instead of the Application - it seems like there are Configurable classes which could benefit from this feature that might be instantiated somewhere besides a Traitlets Application.

@rmorshea
Copy link
Contributor

rmorshea commented May 8, 2019

@kevin-bates that said, I'm also not entirely sure what it would look like to implement this configuration file syncing in the Configurable class. Maybe you could:

  • Get a callback from a set of configurable class which you could trigger?
config_syncer = ConfigFileSyncer(configurable_1, configurable_2, ...)
  • Implement a custom config file loader that occasionally poles the file?

  • A combination of a callback hook and a custom config file loader?

@rmorshea
Copy link
Contributor

rmorshea commented May 8, 2019

@kevin-bates another thing to consider is that Traitlets is in a bit of a holding pattern at the moment:

  • There aren't many maintainers.

  • No new features have been added in the last year or so.

  • We don't have plans to do a release any time soon (though we probably should).

Given all these things, even if we could figure out how to do this in Traitlets, your work has a better chance of seeing the light of day if it were made in a different package within the Jupyter stack.

@kevin-bates
Copy link
Contributor Author

@rmorshea - everything you said makes sense and I appreciate your responses! Yes, this is specifically targeting Jupyter projects that tend to be multi-tenant and service oriented - like the Enterprise Gateway project.

The more I had been thinking about this, the more it made sense to perhaps relocate it as well. However, I would like to add the one method to access the "private" attribute self._loaded_config_files (in the name of good citizenship 😃). Once added, I'm hoping a release could be cut soon. In the meantime, I'd probably just go ahead and (incorrectly) access the private attribute from a subclass so as to not block progress.

I'll go ahead and submit a different PR for the accessor.

Thank you for your time!

@kevin-bates
Copy link
Contributor Author

@rmorshea - I'm closing this in favor of the "lighter" #522. However, the idempotent portion of that PR is required for any external applications to make use of periodic loads, so I'm really hoping for a traitlets release in the near future. Thanks again, for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants