[Part 3 of #205] Added utils for k8s model verification #232
Conversation
kubespawner/utils.py
Outdated
| @@ -27,3 +27,112 @@ def generate_hashed_slug(slug, limit=63, hash_length=6): | |||
| prefix=slug[:limit - hash_length - 1], | |||
| hash=slug_hash[:hash_length], | |||
| ).lower() | |||
|
|
|||
|
|
|||
| def update_k8s_model(target, source, logger=None, origin=None): | |||
There was a problem hiding this comment.
rename origin to target_name, it would make it easier to understand the parameters function
kubespawner/utils.py
Outdated
|
|
||
| return target | ||
|
|
||
| def get_k8s_model(model, model_dict): |
There was a problem hiding this comment.
rename model to model_type for clarity that model isn't an instance of the some kubernetes.client.model.<modelname> but actually the type of such model.
kubespawner/utils.py
Outdated
| else: | ||
| raise AttributeError("Expected object of type 'dict' (or '{}') but got '{}'.".format(model.__type__.__name__, model_dict.__type__.__name__)) | ||
|
|
||
| def _get_k8s_model_dict(model, obj): |
There was a problem hiding this comment.
rename model to model_type for clarity that model isn't an instance of the some kubernetes.client.model.<modelname> but actually the type of such model.
kubespawner/utils.py
Outdated
| Note that the function will not influence nested object's keys. | ||
| """ | ||
|
|
||
| for key in list(model_dict.keys()): |
There was a problem hiding this comment.
Since i modify the model_dict while looping over its keys, I better loop over a copy of the keys rather than evaluating then while modifying them. So, I'm creating a new list from model_dict.keys() by passing it to list(), perhaps i should use copy() to make it more obvious?
kubespawner/utils.py
Outdated
|
|
||
| return model_dict | ||
|
|
||
| def _get_k8s_model_attribute(model, field_name): |
There was a problem hiding this comment.
make it clear that this parameter actually is a model instance.
consideRatio
force-pushed
the
pr3-utils-for-model-verification
branch
from
83538fa to
c44ee1a
Compare
kubespawner/utils.py
Outdated
| # it is important to iterate over a copy of the dictionaries keys, as they | ||
| # will be updated in the loop | ||
| for key in list(model_dict.keys()): | ||
| model_dict[_get_k8s_model_attribute(model_type, key)] = model_dict.pop(key) |
There was a problem hiding this comment.
I think this should create a new dictionary instead of mutating one in-place
There was a problem hiding this comment.
Thanks for considering this @minrk.
I'm intending to use this helper twice as written out below, this code is from PR Part 4.
# the assignments to notebook_container and pod.spec is pointless atm, but won't be if I update the utility function update_k8s_model to be get_updated_copy_of_k8s_model or similar.
if extra_container_config:
notebook_container = update_k8s_model(
target=notebook_container,
changes=extra_container_config,
logger=logger,
target_name="extra_container_config",
)
# ...
if extra_pod_config:
pod.spec = update_k8s_model(
target=pod.spec,
changes=extra_pod_config,
logger=logger,
target_name="extra_pod_config",
)I reasoned that we are already making a lot of changes to notebook_container and pod.spec, and pod.spec will be a huge object relatively so I was thinking I wasn't sure that I could do a deepcopy and retain good enough performance. Also note that anything else than a deepcopy of objects like pod.spec containing a lot of nested reference objects would be misleading I think.
There was a problem hiding this comment.
I trust your experience on this, so I'll go with whatever you think is best and be right on it, the change won't be causing much work at all if we do it!
There was a problem hiding this comment.
Hmmm... target_name isn't right, it's really changes_name
Thank you @minrk for helping me!
consideRatio
force-pushed
the
pr3-utils-for-model-verification
branch
from
cdfd552 to
a2cb445
Compare
About
Adding two utility functions that is not used yet, they are simply made available and tested in some unit tests. Perhaps this shouldn't been extracted as a separate PR as it is pointless if it is not used... Hmm...
get_k8s_modelupdate_k8s_modelAbout
get_k8s_model(model_type, model_dict)In KubeSpawner we handle dictionaries representing segments of resources strictly defined by the Kubernetes API. The user of KubeSpawner most often gives us dictionaries, but internally we could take these and create objects defined in the python kubernetes package such as
V1Containerfor example, which can be importat withfrom kubernetes.client.models import V1Container.get_k8s_modelwill take either a dict or kubernetes model object, but always returns a model object or fail attempting to create one. In this way, we will verify and fail fast if we provide faulty fields to kubespawner.update_k8s_model(target, changes, logger=None, target_name=None, changes_name=None)This function updates a model (
target) in place with some fields that can be set on such model (source). It will also warn if thesource, which is a model object of the same kind as target or a dict, is overriding a field in the target. When it performs the optional logging, it will use thetarget_nameandchanges_nameparameters for some helpful tracing.