You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2023/01/11 08:18:45 UTC
[GitHub] [airflow] lwyszomi opened a new pull request, #28848: Use default connection id for KubernetesPodOperator
lwyszomi opened a new pull request, #28848:
URL: https://github.com/apache/airflow/pull/28848
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] lwyszomi commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "lwyszomi (via GitHub)" <gi...@apache.org>.
lwyszomi commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1489965410
@eladkal rebased
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536042992
> Ok but apparently it was bundled with 2.6.0 https://raw.githubusercontent.com/apache/airflow/constraints-2.6.0/constraints-3.10.txt And in the docker image as I'm using that.
From you message it's not clear if you have problem with or not. and what you are asking for. But no matter what - if you think you have a problem and needs older version of the provider, you can alwas build your own custom image where the provider is [downgraded](https://airflow.apache.org/docs/apache-airflow/stable/installation/installing-from-pypi.html#installing-upgrading-downgrading-providers-separately-from-airflow-core). The image we publish is the reference image and you are free to [extend it](https://airflow.apache.org/docs/docker-stack/build.html#extending-the-image) as you see fit.
What's your ask really @iJanki-gr ? Since you've already checked the constraints? Is your question answered or do you expect something else here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iJanki-gr commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "iJanki-gr (via GitHub)" <gi...@apache.org>.
iJanki-gr commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540161515
Yes that should fix it
kubernetes_conn_id=None
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1435733785
> If we want to release a version before that contains a deprecation warning/write a warning somewhere just in case I'm not against that, but I'd love to bring the KPO more in line with the rest of the operators.
That is preferred
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1379126018
> @dstandish, do you remember why we didn't do this initially?
My question exactly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] karunpoudel-chr commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "karunpoudel-chr (via GitHub)" <gi...@apache.org>.
karunpoudel-chr commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540255739
I am getting this error too.
Should it include logic to fallback to None if conn_id `kubernetes_default` doesn't exists. Otherwise we will have to include `kubernetes_conn_id=None` in 100s of our tasks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540249837
And your error tells exactly this:
```
airflow.exceptions.AirflowNotFoundException: The conn_id `kubernetes_default` isn't defined
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1379214988
> > @dstandish, do you remember why we didn't do this initially?
>
> My question exactly.
Backward compatibility. Previously, KPO did not use airflow conn. If we now use default, this will break things for users. We can do but must be major release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] flinz commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "flinz (via GitHub)" <gi...@apache.org>.
flinz commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540030107
@iJanki-gr don't be sorry, i also just hit the same point exactly. My deployment broke updating the bundled image to airflow==2.6.0 and i didn't figure out directly checking in the cncf provider release notes.
In defense of @iJanki-gr, the [the breaking change announcement on the release notes](https://airflow.apache.org/docs/apache-airflow-providers-cncf-kubernetes/stable/index.html#breaking-changes) does not announce the change needed to make older dags compatible, or at least i couldn't directly find it.
Since i got here only via a few corners, for the sake of google results:
My dags were breaking for KubernetesPodOperator with
```
The conn_id `kubernetes_default` isn't defined
```
Adding `kubernetes_conn_id=None` "fixed" the "issue".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iJanki-gr commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "iJanki-gr (via GitHub)" <gi...@apache.org>.
iJanki-gr commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536006703
Has this change actually got into 2.6.0 release? I had to change KPO conf to include `kubernetes_conn_id=None,` or I would get The conn_id `kubernetes_default` isn't defined
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iJanki-gr commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "iJanki-gr (via GitHub)" <gi...@apache.org>.
iJanki-gr commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536050924
I will thank you and sorry for the bother.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28848:
URL: https://github.com/apache/airflow/pull/28848#discussion_r1066693582
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -229,7 +229,7 @@ class KubernetesPodOperator(BaseOperator):
def __init__(
self,
*,
- kubernetes_conn_id: str | None = None, # 'kubernetes_default',
+ kubernetes_conn_id: str = "kubernetes_default",
Review Comment:
Actually, it’s probably better to use `KubernetesHook.default_conn_name` as the default instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1379483688
Hard no with falling back to get_kube_client -- no reason to bring that back. The kube hook with works with no conn Id and it's fully back compat with get kube client
Now should we ever disregard the default connection? Not sure about that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540268769
@karunpoudel-chr breaking change is something which stops the code from compiling / running, so this is normal.
If you want to avoid adding `kubernetes_conn_id=None` to all the tasks, you can create the connection `kubernetes_default` and configure it to access the cluster, it's the recommended way to configure the hooks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bmoon4 commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "bmoon4 (via GitHub)" <gi...@apache.org>.
bmoon4 commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540262839
i added `kubernetes_conn_id=None` explicitly in `KubernetesPodOperator` parameter and seems working :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iJanki-gr commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "iJanki-gr (via GitHub)" <gi...@apache.org>.
iJanki-gr commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536045851
I fixed my issue, next time i'll check the providers release notes too. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bmoon4 commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "bmoon4 (via GitHub)" <gi...@apache.org>.
bmoon4 commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540158250
Here's the error log
```
[2023-05-09, 08:54:45 EDT] {pod.py:905} ERROR - The conn_id `kubernetes_default` isn't defined
Traceback (most recent call last):
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/operators/pod.py", line 533, in execute_sync
self.pod_request_obj = self.build_pod_request_obj(context)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/operators/pod.py", line 859, in build_pod_request_obj
"airflow_kpo_in_cluster": str(self.hook.is_in_cluster),
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 249, in is_in_cluster
self.api_client # so we can determine if we are in_cluster or not
File "/usr/local/lib/python3.8/functools.py", line 967, in __get__
val = self.func(instance)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 257, in api_client
return self.get_conn()
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 171, in get_conn
in_cluster = self._coalesce_param(self.in_cluster, self._get_field("in_cluster"))
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 164, in _get_field
if field_name in self.conn_extras:
File "/usr/local/lib/python3.8/functools.py", line 967, in __get__
val = self.func(instance)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 147, in conn_extras
connection = self.get_connection(self.conn_id)
File "/usr/local/lib/python3.8/site-packages/airflow/hooks/base.py", line 72, in get_connection
conn = Connection.get_connection_from_secrets(conn_id)
File "/usr/local/lib/python3.8/site-packages/airflow/models/connection.py", line 434, in get_connection_from_secrets
raise AirflowNotFoundException(f"The conn_id `{conn_id}` isn't defined")
airflow.exceptions.AirflowNotFoundException: The conn_id `kubernetes_default` isn't defined
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/operators/pod.py", line 745, in patch_already_checked
self.client.patch_namespaced_pod(
File "/usr/local/lib/python3.8/functools.py", line 967, in __get__
val = self.func(instance)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/operators/pod.py", line 473, in client
return self.hook.core_v1_client
File "/usr/local/lib/python3.8/functools.py", line 967, in __get__
val = self.func(instance)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 261, in core_v1_client
return client.CoreV1Api(api_client=self.api_client)
File "/usr/local/lib/python3.8/functools.py", line 967, in __get__
val = self.func(instance)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 257, in api_client
return self.get_conn()
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 171, in get_conn
in_cluster = self._coalesce_param(self.in_cluster, self._get_field("in_cluster"))
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 164, in _get_field
if field_name in self.conn_extras:
File "/usr/local/lib/python3.8/functools.py", line 967, in __get__
val = self.func(instance)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/hooks/kubernetes.py", line 147, in conn_extras
connection = self.get_connection(self.conn_id)
File "/usr/local/lib/python3.8/site-packages/airflow/hooks/base.py", line 72, in get_connection
conn = Connection.get_connection_from_secrets(conn_id)
File "/usr/local/lib/python3.8/site-packages/airflow/models/connection.py", line 434, in get_connection_from_secrets
raise AirflowNotFoundException(f"The conn_id `{conn_id}` isn't defined")
airflow.exceptions.AirflowNotFoundException: The conn_id `kubernetes_default` isn't defined
```
@iJanki-gr
Im going to test `Adding kubernetes_conn_id=None` in `/usr/local/lib/python3.8/site-packages/airflow/providers/cncf/kubernetes/operators/pod.py`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bmoon4 commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "bmoon4 (via GitHub)" <gi...@apache.org>.
bmoon4 commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540138642
2.6.0 KubernetesPodOperator is not working when running my dag..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1379342737
> i think quite likely:
>
> https://github.com/apache/airflow/blob/6d09fc7b88623919b01cfbdee5566598cefba021/airflow/utils/db.py#L364-L370
Let me modify it **slightly**. Why don't we fall-back to `get_kube_client` when either the connection is not defined or when it is not **configured** (i.e no extras defined). I believe the only used connection fields are "extras" and it would require an effort to edit the connection to add any extra there - the default one will have no extras at all and we can easily detect it. A bit long shot, but maybe it is the most sensible thing to do? (just wild thought)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a diff in pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #28848:
URL: https://github.com/apache/airflow/pull/28848#discussion_r1155622590
##########
airflow/providers/cncf/kubernetes/CHANGELOG.rst:
##########
@@ -24,6 +24,13 @@
Changelog
---------
+6.0.0
+.....
+
+Breaking changes
+~~~~~~~~~~~~~~~~
+Use ``kubernetes_default`` connection by default in the KubernetesPodOperator. (#28848)
Review Comment:
```suggestion
Breaking changes
~~~~~~~~~~~~~~~~
Use ``kubernetes_default`` connection by default in the KubernetesPodOperator.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28848:
URL: https://github.com/apache/airflow/pull/28848#discussion_r1071018424
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -141,7 +141,9 @@ def teardown(self) -> None:
client = hook.core_v1_client
client.delete_collection_namespaced_pod(namespace="default", grace_period_seconds=0)
- def test_do_xcom_push_defaults_false(self):
+ @mock.patch(f"{HOOK_CLASS}.get_connection")
+ def test_do_xcom_push_defaults_false(self, get_connection_mock):
+ get_connection_mock.return_value = Connection(conn_id="kubernetes_default")
Review Comment:
```suggestion
@mock.patch(f"{HOOK_CLASS}.get_connection", return_value=Connection(conn_id="kubernetes_default"))
def test_do_xcom_push_defaults_false(self, get_connection_mock):
```
I think you can do this instead.
Since this is repeated so may times, it may be a good idea to make this a fixture instead:
```python
@pytest.fixture()
def mock_get_connection():
with mock.patch(f"{HOOK_CLASS}.get_connection", return_value=Connection(conn_id="kubernetes_default")):
yield
@pytest.mark.usefixtures("mock_get_connection")
def test_do_xcom_push_defaults_false(self):
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1379268292
> Backward compatibility. Previously, KPO did not use airflow conn. If we now use default, this will break things for users. We can do but must be major release.
So it was using the bult-in "local" kubernetes (basically whatever kubectl sees ?) if None is used?
Maybe we can check for existence of the connection and fallback to default behaviour when it is not available? What's the likelihood someone will have default k8s connection set and would not want to use it ? (still likely requring major version bump and technically breaking but with far less radius blast)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1379294932
> So it was using the bult-in "local" kubernetes (basically whatever kubectl sees ?) if None is used?
it was using the settings from core, i.e. `get_kube_client`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1487982374
@lwyszomi can you rebase and resolve conflicts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536043466
This is a breaking change that replaces the default value of the parameter. hence why it was released in a major version (6.0.0) so yet if you don't have `kubernetes_default` define it will break your dags. You need to go trough the change log when you are updating major versions of providers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iJanki-gr commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "iJanki-gr (via GitHub)" <gi...@apache.org>.
iJanki-gr commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536037816
Ok but apparently it was bundled with 2.6.0 https://raw.githubusercontent.com/apache/airflow/constraints-2.6.0/constraints-3.10.txt
And in the docker image as I'm using that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28848:
URL: https://github.com/apache/airflow/pull/28848#discussion_r1067815408
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -147,6 +147,7 @@ def test_do_xcom_push_defaults_false(self):
old_config_path = get_kubeconfig_path()
shutil.copy(old_config_path, new_config_path)
k = KubernetesPodOperator(
+ kubernetes_conn_id=None,
Review Comment:
Doesn’t feel like a good idea to do this. We should instead modify the tests (maybe use mocks) to actually test the conn id.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1435754945
So if decided to go with major release directly (which is OK)
please update provider.yaml to version 6.0.0 and add entry in the provider change log
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540252216
The whole discussion above explaines all about this in case you have not read it @bmoon4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1542586980
I offer a solution in https://github.com/apache/airflow/pull/31187
If kubernetes_default doesn't exist, ignore it. If other conn id doesn't exist, fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #28848:
URL: https://github.com/apache/airflow/pull/28848#discussion_r1153535938
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -220,27 +219,14 @@ class PodManager(LoggingMixin):
def __init__(
self,
kube_client: client.CoreV1Api = None,
Review Comment:
```suggestion
kube_client: client.CoreV1Api,
```
Should this be required now?
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -220,27 +219,14 @@ class PodManager(LoggingMixin):
def __init__(
self,
kube_client: client.CoreV1Api = None,
Review Comment:
```suggestion
kube_client: client.CoreV1Api,
```
Shouldn't this be required now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #28848:
URL: https://github.com/apache/airflow/pull/28848#discussion_r1160452907
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -140,6 +146,7 @@ def _get_labels_selector(self) -> str | None:
return None
return ",".join([f"{key}={value}" for key, value in enumerate(self.labels)])
+ @pytest.mark.usefixtures("mock_get_connection")
Review Comment:
Usefixtures makes little sense when put as method decorator (more typing, less explicit, only saves "unused parameter" in IDE. But it makes whole lot of sense when put on a class - can we please remove it it from the methods and put it once on TestKubernetesPodOperatorSystem instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536046740
> Yes that's ok, but in the release notes of airflow 2.6.0 there was no mention of cncf kubernetes provider upgrade.
Because the image bundles airflow 2.6.0 AND providers. The image is a reference one that contains airflow and latest provders, but you are in control on which providers you want to keep or use. As @eladkal mentioned, if you chose to use the image, you need to check the release notes of the providers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536035255
> Has this change actually got into 2.6.0 release?
This is not a core PR It's not released with apache-airlfow it's released with Kubernetes provider: `apache-airflow-providers-cncf-kubernetes=6.0.0`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1379095186
@dstandish, do you remember why we didn't do this initially?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] lwyszomi commented on a diff in pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
lwyszomi commented on code in PR #28848:
URL: https://github.com/apache/airflow/pull/28848#discussion_r1066708862
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -229,7 +229,7 @@ class KubernetesPodOperator(BaseOperator):
def __init__(
self,
*,
- kubernetes_conn_id: str | None = None, # 'kubernetes_default',
+ kubernetes_conn_id: str = "kubernetes_default",
Review Comment:
@uranusjr I changed and now we using `KubernetesHook.default_conn_name`, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1379293959
> > Backward compatibility. Previously, KPO did not use airflow conn. If we now use default, this will break things for users. We can do but must be major release.
>
> So it was using the bult-in "local" kubernetes (basically whatever kubectl sees ?) if None is used?
>
> Maybe we can check for existence of the connection and fallback to default behaviour when it is not available? What's the likelihood someone will have default k8s connection set and would not want to use it ? (still likely requring major version bump and technically breaking but with far less radius blast)
i think quite likely: https://github.com/apache/airflow/blob/6d09fc7b88623919b01cfbdee5566598cefba021/airflow/utils/db.py#L364-L370
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "dimberman (via GitHub)" <gi...@apache.org>.
dimberman commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1428129892
Hey @dstandish @potiuk can we restart this conversation? I wouldn't want this PR to fall into the abyss just because we're unsure.
FWIW I think it's fine for us to make this breaking change and then do a major update. to me this is where the magic of providers exists. If we want to release a version before that contains a deprecation warning/write a warning somewhere just in case I'm not against that, but I'd love to bring the KPO more in line with the rest of the operators.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1454822064
Needs conflict resplution now, I am afraid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal merged pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #28848:
URL: https://github.com/apache/airflow/pull/28848
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a diff in pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #28848:
URL: https://github.com/apache/airflow/pull/28848#discussion_r1155622590
##########
airflow/providers/cncf/kubernetes/CHANGELOG.rst:
##########
@@ -24,6 +24,13 @@
Changelog
---------
+6.0.0
+.....
+
+Breaking changes
+~~~~~~~~~~~~~~~~
+Use ``kubernetes_default`` connection by default in the KubernetesPodOperator. (#28848)
Review Comment:
```suggestion
Breaking changes
~~~~~~~~~~~~~~~~
Use kubernetes_default connection by default in the KubernetesPodOperator.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1435744430
I'm fine with using `kubernetes_default` by default, just should be in a major. And i don't have an opinion re whether we need to introduce warning first so I defer to y'all on that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iJanki-gr commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "iJanki-gr (via GitHub)" <gi...@apache.org>.
iJanki-gr commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1536045168
Yes that's ok, but in the release notes of airflow 2.6.0 there was no mention of cncf kubernetes provider upgrade.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iJanki-gr commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "iJanki-gr (via GitHub)" <gi...@apache.org>.
iJanki-gr commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540150084
It works for me. Can you provide more details?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28848: Use default connection id for KubernetesPodOperator
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #28848:
URL: https://github.com/apache/airflow/pull/28848#issuecomment-1540247064
The kubernetes provider as of 6.0.0 releases uses kubernetes connection to be defined https://airflow.apache.org/docs/apache-airflow-providers-cncf-kubernetes/stable/index.html#breaking-changes and you should switch to it. That was a breaking change in the provider. Have you checked the release notes ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org