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.

Sign up for GitHub

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

Jump to bottom

Fix Volume/VolumeMount KPO DeprecationWarning #19726

Merged
merged 7 commits into from
Dec 16, 2021
Merged

Fix Volume/VolumeMount KPO DeprecationWarning #19726

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift click to select a range
8b1ef59
Fix Volume/VolumeMount DeprecationWarning
dimon222 Nov 20, 2021
1c94032
Adjust all calls to be specific
dimon222 Nov 29, 2021
d35e763
Clean imports
dimon222 Nov 29, 2021
d6024da
Update airflow/providers/cncf/kubernetes/backcompat/backwards_compat_…
dimon222 Dec 2, 2021
e09651f
Update airflow/providers/cncf/kubernetes/backcompat/backwards_compat_…
dimon222 Dec 2, 2021
bce1a89
Black
dimon222 Dec 3, 2021
27619c6
Recommend only new classes
dimon222 Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 21,16 @@
from kubernetes.client import ApiClient, models as k8s

from airflow.exceptions import AirflowException
from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources
from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv


def _convert_kube_model_object(obj, old_class, new_class):
def _convert_kube_model_object(obj, new_class):
convert_op = getattr(obj, "to_k8s_client_obj", None)
if callable(convert_op):
return obj.to_k8s_client_obj()
Copy link
Contributor

@dstandish dstandish Dec 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedcunningham, do you think we should add deprecation warning here (i.e. before calling to_k8s_client_obj) in addition to here?

Users who import the backcompat modules will get warnings when they do so; but maybe we should also signal here that this logic to convert will be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: Isn't it implied by definition of "Deprecation"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to show 2 deprecation warnings, and I agree with @dimon222 that it's implied the backcompat conversion will eventually be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: Isn't it implied by definition of "Deprecation"?

Removal is implied by the word deprecation, yes. However, KPO is not making any deprecation warning (which is my concern) yet the KPO will eventually remove this logic. I think 2 deprecations warnings is the lesser of 2 evils here (one for "backcompat is gonna be removed" the other for "we aren't gonna convert forever"), but not a hill to die on and i don't stand in the way.

Further, the logic here does not even assume a backcompat class -- it is logic to accept kwargs from a dict. So in that case the user won't have a deprecation warning at all. Again no dying on hills for me here either and i defer to @jedcunningham

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedcunningham got any better suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in my eyes, "backcompat" and "convert" is the same thing 🤷‍♂️. I don't think we need to warn about both, but the nuance may be more important for end users.

In cases where there users aren't using the "old" classes directly, like your resources example (which will
actually get a deprecation warning in #20031), I think we should swallow the deprecation warning and emit a more helpful one for the user.

For example, for users using dict resources, instead of saying:

This module is deprecated. Please use `kubernetes.client.models.V1ResourceRequirements` and `kubernetes.client.models.V1ContainerPort`."

say (or something like it):

Setting resources as a dict is deprecated. Please use `kubernetes.client.models.V1ResourceRequirements`

env_vars also has this issue, not sure if there are others though.

Either way, I think the new deprecation warning stuff should be handled in #20031.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess thats a problem of the fact that Resources and Port are combined together under not straightforward named "pod.py", otherwise the filename itself should describe the exact concerning class (ala resources.py and port.py). But you're right, its better be handled separately in #20031. So, should we merge current PR then as is?

elif isinstance(obj, new_class):
return obj
else:
raise AirflowException(f"Expected {old_class} or {new_class}, got {type(obj)}")
raise AirflowException(f"Expected {new_class}, got {type(obj)}")


def _convert_from_dict(obj, new_class):
Expand All @@ -52,9 50,7 @@ def convert_volume(volume) -> k8s.V1Volume:
:param volume:
:return: k8s.V1Volume
"""
from airflow.providers.cncf.kubernetes.backcompat.volume import Volume

return _convert_kube_model_object(volume, Volume, k8s.V1Volume)
return _convert_kube_model_object(volume, k8s.V1Volume)


def convert_volume_mount(volume_mount) -> k8s.V1VolumeMount:
Expand All @@ -64,9 60,7 @@ def convert_volume_mount(volume_mount) -> k8s.V1VolumeMount:
:param volume_mount:
:return: k8s.V1VolumeMount
"""
from airflow.providers.cncf.kubernetes.backcompat.volume_mount import VolumeMount

return _convert_kube_model_object(volume_mount, VolumeMount, k8s.V1VolumeMount)
return _convert_kube_model_object(volume_mount, k8s.V1VolumeMount)


def convert_resources(resources) -> k8s.V1ResourceRequirements:
Expand All @@ -77,8 71,10 @@ def convert_resources(resources) -> k8s.V1ResourceRequirements:
:return: k8s.V1ResourceRequirements
"""
if isinstance(resources, dict):
from airflow.providers.cncf.kubernetes.backcompat.pod import Resources
dimon222 marked this conversation as resolved.
Show resolved Hide resolved

resources = Resources(**resources)
return _convert_kube_model_object(resources, Resources, k8s.V1ResourceRequirements)
return _convert_kube_model_object(resources, k8s.V1ResourceRequirements)


def convert_port(port) -> k8s.V1ContainerPort:
Expand All @@ -88,7 84,7 @@ def convert_port(port) -> k8s.V1ContainerPort:
:param port:
:return: k8s.V1ContainerPort
"""
return _convert_kube_model_object(port, Port, k8s.V1ContainerPort)
return _convert_kube_model_object(port, k8s.V1ContainerPort)


def convert_env_vars(env_vars) -> List[k8s.V1EnvVar]:
Expand Down Expand Up @@ -116,7 112,7 @@ def convert_pod_runtime_info_env(pod_runtime_info_envs) -> k8s.V1EnvVar:
:param pod_runtime_info_envs:
:return:
"""
return _convert_kube_model_object(pod_runtime_info_envs, PodRuntimeInfoEnv, k8s.V1EnvVar)
return _convert_kube_model_object(pod_runtime_info_envs, k8s.V1EnvVar)


def convert_image_pull_secrets(image_pull_secrets) -> List[k8s.V1LocalObjectReference]:
Expand Down