Zero-to-jupyterhub-k8s: kubespawners deprecation of singleuser_/user_ prefixes

[consideRatio]

Background

The c.KubeSpawner.singleuser_extra_pod_config traitlet is deprecated, along with all traitlets prefixed singleuser_. To remain backwards compatible kubespawner observes the fields with singleuser prefix and updates a new field instead using setattr(self, _new_name, change.new).

jupyterhub/kubespawner : spawner.py

from traitlets import Any, Unicode, List, Integer, Union, Dict, Bool, Any, observe

# ... ... ...

class KubeSpawner(Spawner):
    """
    Implement a JupyterHub spawner to spawn pods in a Kubernetes Cluster.
    """

    # ... ... ...

    extra_pod_config = Dict(
        None,
        config=True,
        help="""
        Extra configuration (e.g. tolerations) for the pod which is not covered by other attributes.

        This dict will be directly merge into pod,so you should use the same structure.
        Each item in the dict is field of pod configuration
        which follows spec at https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#podspec-v1-core.

        One usage is set dnsPolicy with configuration below::

            dnsPolicy: ClusterFirstWithHostNet

        The `key` could be either camelcase word (used by Kubernetes yaml, e.g. `dnsPolicy`)
        or underscore-separated word (used by kubernetes python client, e.g. `dns_policy`).
        """
    )

    # ... ... ...

    # deprecate redundant and inconsistent singleuser_ and user_ prefixes:
    _deprecated_traits = [
        "singleuser_working_dir",
        "singleuser_service_account",
        "singleuser_extra_labels",
        "singleuser_extra_annotations",
        "singleuser_image_spec",
        "singleuser_image_pull_policy",
        "singleuser_image_pull_secrets",
        "singleuser_node_selector",
        "singleuser_uid",
        "singleuser_fs_gid",
        "singleuser_supplemental_gids",
        "singleuser_privileged",
        "singleuser_lifecycle_hooks",
        "singleuser_extra_pod_config",
        "singleuser_init_containers",
        "singleuser_extra_container_config",
        "singleuser_extra_containers",
        "user_storage_class",
        "user_storage_pvc_ensure",
        "user_storage_capacity",
        "user_storage_extra_labels",
        "user_storage_access_modes",
    ]
    # define Any traits for deprecated names
    # so we can propagate their values to the new traits
    for _deprecated_name in _deprecated_traits:
        _new_name = _deprecated_name.split('_', 1)[1]
        exec(
            "{} = Any(config=True, help='DEPRECATED. Use {}.')".format(
                _deprecated_name, _new_name
            )
        )
    del _deprecated_name, _new_name

    @observe(*_deprecated_traits)
    def _deprecated_trait_changed(self, change):
        """Warn on use of deprecated config traits

        preserving behavior by propagating values to the new name
        """
        # new name without prefix:
        _new_name = change.name.split('_', 1)[1]
        # warn about the deprecated name
        warnings.warn(
            "KubeSpawner.{} is deprecated in 0.9. Use KubeSpawner.{}".format(
                change.name, _new_name,
            ),
            DeprecationWarning,
            stacklevel=2,
        )
        # assign to the real attribute
        setattr(self, _new_name, change.new)

In this helm chart, we (chart maintainers) setup the JupyterHub pod with a jupyterhub_config.py that will make some initial configuration. Then later a chart user (cluster administrators) might make additional configuration that are added. @kalugny realized (#740) that using singleuser_extra_pod_config worked in the additional configuration while extra_pod_config did not work, and that was unintentionally reproduced by me as well :sob:.

Theory 1

Perhaps the initial configuration on singleuser_extra_pod_config is @observeed by the traitlets library but the reaction will happen after the additional configuration, causing the value set on extra_pod_config in the additional configuration to be overrided by the value set to singleuser_extra_pod_config in the initial configuration.

# the last lines of the initial configuration in `jupyterhub_config.py` used in the z2jh's hub image
extra_configs = sorted(glob.glob('/etc/jupyterhub/config/hub.extra-config.*.py'))
for ec in extra_configs:
    load_subconfig(ec)

Theory 2

Perhaps the setattr command replaces the extra_pod_config traitlet objects with a normal Dict, causing some trouble somehow...

        setattr(self, _new_name, change.new)

Solution idea

We update the initial configuration to not use the singleuser_ prefix on kubespawners traitlets. If Theory 1 is correct, it means that the initial configuration using the deprecated syntax overruled things, and the change will simply allow additional configuration to work. I think it would be a safe change, but I'm not 100%.

Reference

The deprecated prefixes (singleuser_ and user_) is used in the initial configuration for...

# mashed code from the initial configuration
singleuser_image_spec = os.environ['SINGLEUSER_IMAGE']
singleuser_image_pull_policy = get_config('singleuser.image-pull-policy')
singleuser_extra_labels = get_config('singleuser.extra-labels', {})
singleuser_uid = get_config('singleuser.uid')
singleuser_fs_gid = get_config('singleuser.fs-gid')
singleuser_service_account = service_account_name
singleuser_node_selector = get_config('singleuser.node-selector')
singleuser_lifecycle_hooks = lifecycle_hooks
singleuser_init_containers.extend(init_containers)
singleuser_init_containers.append(ip_block_container)
singleuser_extra_pod_config = { ... }
user_storage_pvc_ensure = True
user_storage_class = storage_class
user_storage_access_modes = get_config('singleuser.storage.dynamic.storage-access-modes')
user_storage_capacity = get_config('singleuser.storage.capacity')

/cc: @minrk @yuvipanda - Does Theory 1 make sense? Does the solution idea seem safe to implement?

[betatim]

I think if config options have become deprecated in kubespawner we should update this chart to not use the deprecated versions any more. (Even if this doesn't fix the bug reported here.)

[kalugny]

That would leave us users totally stranded...
At least now we have a work-around (even if unintentionally)

[consideRatio]

@betatim thanks for helping me process this! I'm thinking it the change will be fine as long as Theory 1 is correct, I think I'll verify the behavior experimentally.

@kalugny I think the change will make you able to use singleuser_extra_pod_config along with extra_pod_config while previously you and me both ended up confused about extra_pod_config not working while singleuser_extra_pod_config ended up working. I really appreciate that you wrote #740, I would had spent so much more time stuck on this bug without understanding it was related to the singleuser_ prefix unless you had battled this issue already! :heart:

[minrk]

There was a bug in the helm chart, where it set singleuser_extra_pod_config unconditionally. This took higher precedence than your extra_pod_config and thus wiped it out. @betatim's proposal to stop using the deprecated config in the helm chart by default would result in both the deprecated and official traits working as intended.

I'll investigate a deprecation scheme that is less vulnerable to this (essentially, the deprecated config has higher priority than the new, official config, which is probably the opposite of what we want).

[minrk]

https://github.com/jupyterhub/zero-to-jupyterhub-k8s/pull/746 updates the helm chart to stop using the deprecated config values, which fixes the direct issue, where deprecated config was having higher priority than your non-deprecated config

https://github.com/jupyterhub/kubespawner/pull/199 adjusts how the deprecations work, so that when both deprecated and non-deprecated config are present, the non-deprecated config takes precedence. Additionally, there will be a visible warning when there is a collision.