Fix Volume/VolumeMount KPO DeprecationWarning

[dimon222]

What is this?
This is to resolve incomplete solution of this commit 42e13e1 (#17900)
The deprecation warning was previously shown always, but with above commit the scope was reduced. Unfortunately, its not doing what its intended to do - show this warning only when legacy Volume and VolumeMount classes are being used with KubernetesPodOperator. Instead, it now shows up always when you use Volume or VolumeMount of ANY implementation (old or new).

The reason for that is next:

1. The "conversion" methods are called on KPO always whenever there's any Volume/VolumeMount
   

   airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py

   Lines 246 to 247 in 1e57022

   	self.volume_mounts = [convert_volume_mount(v) for v in volume_mounts] if volume_mounts else []
   	self.volumes = [convert_volume(volume) for volume in volumes] if volumes else []

2. While those methods will convert only if necessary, inside of them we're doing imports of legacy class, which immediately shows message of deprecation upon import, rather than upon validation of underline class (so that we know if we have to import it in the first place...). In other words, we have a chicken-and-egg problem that might have been solved incorrectly.
   

   airflow/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py

   Lines 55 to 57 in 1e57022

   	from airflow.providers.cncf.kubernetes.backcompat.volume import Volume
   	return _convert_kube_model_object(volume, Volume, k8s.V1Volume)

   
   

   airflow/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py

   Lines 67 to 69 in 1e57022

   	from airflow.providers.cncf.kubernetes.backcompat.volume_mount import VolumeMount
   	return _convert_kube_model_object(volume_mount, VolumeMount, k8s.V1VolumeMount)

So what do we do about it?
One of the approaches that I decided to take was to inspect the method _convert_kube_model_object. To my surprise, it doesn't use old class for anything but basically print of exception in "catch-all-other-scenarios" way.

airflow/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py

Lines 28 to 35 in 1e57022

	def _convert_kube_model_object(obj, old_class, new_class):
	convert_op = getattr(obj, "to_k8s_client_obj", None)
	if callable(convert_op):
	return obj.to_k8s_client_obj()
	elif isinstance(obj, new_class):
	return obj
	else:
	raise AirflowException(f"Expected {old_class} or {new_class}, got {type(obj)}")

The minimal effort solution is to avoid import of legacy classes in backcompat altogether, while passing just the "string" name of those classes. This way the behavior of underline method is not changing, we still get the deprecation warnings when user is trying to import legacy classes separately in his code (while constructing the KPO), but there won't be warning shown during "processing" of KPO itself (on scheduler I assume?).

The proposed solution is potentially anti-pattern, as we pass literally the wrong type to defined function, but I'm ready to listen for constructive "better" solutions that will introduce minimal regressions to existing airflow codebase. That might come with extra overhead of validation of incoming type above the call to _convert_kube_model_object or such. In proposed solution should be no such extra overhead.

[potiuk]

cc: @dimberman ?

[jedcunningham]

I'm generally okay with this path, however, it should be done for everything, not just volumes and mounts. @dimon222, can you refactor the rest too? Probably worth renaming the kwarg in _convert_kube_model_object to be clear it is the name, not the actual class?

[dimon222]

@jedcunningham sure, done

[jedcunningham]

Shouldn't we also remove the imports for the backcompat Port, Resources and PodRuntimeInfoEnv?

[dimon222]

@jedcunningham this is where stuff might get sketchy.
I can apply those corrections, but because of inconsistency of current implementation, there will be.. corner case scenarios.

Specifically for Pod, Resources and PodRuntimeInfoEnv I don't see deprecation warnings in respective classes - I can add them, same as we do for others. However, the Resources logic even goes further by doing blind conversion without letting anyone know.
https://github.com/apache/airflow/blob/7b03678ec557a23017a6aa1fc225bbc84e5a92fb/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py#L75-L76

[dimon222]

@jedcunningham if possible, some recommendation would be preferable. Seems like this kind of import that I added in last commit is not passing one of static checks.

[jedcunningham]

Looks good. Let's handle adding deprecation warnings in a separate PR.

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

[dimon222]

Still same static check test is crashing

[dimon222]

I think CI broke

[dimon222]

@jedcunningham this is passing now, and I split the deprecation task to #20031 which might require same attention (i'm not sure how to solve the linter stuff without changing header)

@potiuk FYI if u plan to include it in this RC or future ones.

[dstandish]

Looks good to me 👍

[dimon222]

PS: don't merge until tests are done, there might be some broken after my last commit

[dstandish]

on second thought just want @jedcunningham's thoughts on whether we should also add deprecation here

[dstandish]

@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.

[dimon222]

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

[jedcunningham]

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.

[dstandish]

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

[dimon222]

@jedcunningham got any better suggestion?

[jedcunningham]

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.

[dimon222]

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?

[dimon222]

Can someone retrigger CI for this PR? I think it didn't finish afterall. I believe aside of that this PR is ready for merge (unless any other concerns?)

[jedcunningham]

Done. (Closing/reopening is an easy way to do it 👍)