You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "hussein-awala (via GitHub)" <gi...@apache.org> on 2023/02/12 22:15:10 UTC
[GitHub] [airflow] hussein-awala opened a new pull request, #29498: [WIP] add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
hussein-awala opened a new pull request, #29498:
URL: https://github.com/apache/airflow/pull/29498
closes: #29488
---
The async execute method of `KubernetesPodOperator` doesn't check if the `config_path` is provided in the connection extra, this PR fixes this by extracting the config path in order to read it and convert it to dictionary.
--
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] raphaelauv commented on pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "raphaelauv (via GitHub)" <gi...@apache.org>.
raphaelauv commented on PR #29498:
URL: https://github.com/apache/airflow/pull/29498#issuecomment-1438040543
Loading the config file from the env KUBECONFIG is deprecated in latest provider version
--
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] bjankie1 commented on a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "bjankie1 (via GitHub)" <gi...@apache.org>.
bjankie1 commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1116981921
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -565,7 +565,16 @@ def execute_async(self, context: Context):
def convert_config_file_to_dict(self):
"""Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+ config_file = None
Review Comment:
Please mind that all FS operations are blocking side effects. It's violating asyncio contract and can cause additional error logs informing about blocking code.
--
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] VladaZakharova commented on a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1105742245
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -565,7 +565,16 @@ def execute_async(self, context: Context):
def convert_config_file_to_dict(self):
"""Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+ config_file = None
Review Comment:
Hi Team!
This was implemented to lighten the credential management: the config file was converted to dict to be passed to trigger and then hook to establish connection.
--
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 #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #29498:
URL: https://github.com/apache/airflow/pull/29498#issuecomment-1436659985
> I guess you will be still changing the config access pattern on that one ? Do I understand correctly?
Yes, I'm testing loading the config file in the triggerer instead of loading it in the worker and pass it as a dict.
I convert the PR to draft until I finish testing
--
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] raphaelauv commented on a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "raphaelauv (via GitHub)" <gi...@apache.org>.
raphaelauv commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1104585339
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -565,7 +565,16 @@ def execute_async(self, context: Context):
def convert_config_file_to_dict(self):
"""Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+ config_file = None
Review Comment:
thanks @hussein-awala for proposing this fix.
why the async need the function `convert_config_file_to_dict`and not the sync ?
Look like the async was implemented not fully following this pattern -> https://github.com/apache/airflow/pull/20578
your PR fix the problems for the extra `config_path` , there is a risk that another is missing or new in the future would need "manual" fix like this
--
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 a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1167555764
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -560,18 +557,8 @@ def execute_async(self, context: Context):
pod_request_obj=self.pod_request_obj,
context=context,
)
- self.convert_config_file_to_dict()
self.invoke_defer_method()
- def convert_config_file_to_dict(self):
- """Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
- if config_file:
- with open(config_file) as f:
- self._config_dict = yaml.safe_load(f)
- else:
- self._config_dict = None
Review Comment:
In my opinion, this method is used as a private method since it only updates some attributes in the class instances without returning any value. However, it's possible that someone could extend the operator class and use it. Should we deprecate it and remove it in the next major release, or should we add a breaking change note?
--
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] github-actions[bot] commented on pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29498:
URL: https://github.com/apache/airflow/pull/29498#issuecomment-1502498247
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1167624973
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -560,18 +557,8 @@ def execute_async(self, context: Context):
pod_request_obj=self.pod_request_obj,
context=context,
)
- self.convert_config_file_to_dict()
self.invoke_defer_method()
- def convert_config_file_to_dict(self):
- """Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
- if config_file:
- with open(config_file) as f:
- self._config_dict = yaml.safe_load(f)
- else:
- self._config_dict = None
Review Comment:
So lets deprecate first. Just to be on the safe side.
--
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] VladaZakharova commented on a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1106341027
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -565,7 +565,16 @@ def execute_async(self, context: Context):
def convert_config_file_to_dict(self):
"""Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+ config_file = None
Review Comment:
We needed a way to pass config file to the trigger to create a client for kubernetes, but using file system to communicate with trigger was not a good solution. So then we added a possibility to pass all config file parameters as a dict.
--
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] raphaelauv commented on pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "raphaelauv (via GitHub)" <gi...@apache.org>.
raphaelauv commented on PR #29498:
URL: https://github.com/apache/airflow/pull/29498#issuecomment-1506630940
@hussein-awala the PR have conflicts , could you rebase on main , thank you :+1:
--
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 a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1106458479
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -565,7 +565,16 @@ def execute_async(self, context: Context):
def convert_config_file_to_dict(self):
"""Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+ config_file = None
Review Comment:
To respect the pattern mentioned by @raphaelauv, I will try loading the config file in the async hook, this should work where the triggerer is initiated once.
--
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 #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1167490656
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -560,18 +557,8 @@ def execute_async(self, context: Context):
pod_request_obj=self.pod_request_obj,
context=context,
)
- self.convert_config_file_to_dict()
self.invoke_defer_method()
- def convert_config_file_to_dict(self):
- """Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
- if config_file:
- with open(config_file) as f:
- self._config_dict = yaml.safe_load(f)
- else:
- self._config_dict = None
Review Comment:
is removing this function considered a breaking change?
--
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 merged pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29498:
URL: https://github.com/apache/airflow/pull/29498
--
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 #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29498:
URL: https://github.com/apache/airflow/pull/29498#issuecomment-1436627597
@hussein-awala I guess you will be still changing the config access pattern on that one ? Do I understand correctly?
--
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 #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #29498:
URL: https://github.com/apache/airflow/pull/29498#issuecomment-1437703231
> Hi! May i ask in which format you will pass the config file to trigger? So it will be just a file passed as a parameter to trigger? Or how?
@VladaZakharova - Yes, I pass the file path and let the triggerer loads it. Can you check my last commit?
BTW, I am not sure if loading the config file from the env var `KUBECONFIG` is a good idea or not, because it's difficult to decide when we need to load it and when we don't.
--
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 #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29498:
URL: https://github.com/apache/airflow/pull/29498#issuecomment-1518711435
LGTM. @eladkal ?
--
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] VladaZakharova commented on pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on PR #29498:
URL: https://github.com/apache/airflow/pull/29498#issuecomment-1436702119
Hi!
May i ask in which format you will pass the config file to trigger?
--
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 a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1105145411
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -565,7 +565,16 @@ def execute_async(self, context: Context):
def convert_config_file_to_dict(self):
"""Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+ config_file = None
Review Comment:
I am not sure about the initial reason to convert the file into dictionary before creating the trigger, it may be to avoid copying the config file to the triggerer, where the pod is created on the worker using the sync hook and the waiting task is running on the triggerer and it uses the async hook.
> here is a risk that another is missing or new in the future would need "manual" fix like this
With this fix, we cover all options currently available to provide the configuration file, and yes, if we add a new one in the future, we must add it on the sync hook and in this method.
@VladaZakharova can you please explain what was the motivation to convert the config file to a dictionary before creating the trigger?
--
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] raphaelauv commented on a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "raphaelauv (via GitHub)" <gi...@apache.org>.
raphaelauv commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1105921373
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -565,7 +565,16 @@ def execute_async(self, context: Context):
def convert_config_file_to_dict(self):
"""Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+ config_file = None
Review Comment:
what do you mean by `lighten the credential management` ?
the hook is not re instantiate at every run of the trigger ?
--
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] VladaZakharova commented on a diff in pull request #29498: add missing read for K8S config file from conn in deferred `KubernetesPodOperator`
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on code in PR #29498:
URL: https://github.com/apache/airflow/pull/29498#discussion_r1105742245
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -565,7 +565,16 @@ def execute_async(self, context: Context):
def convert_config_file_to_dict(self):
"""Converts passed config_file to dict format."""
- config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+ config_file = None
Review Comment:
Hi Team!
This was implemented to that config file was converted to dict to be passed to trigger and then hook to establish connection.
--
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