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 2020/12/21 19:29:35 UTC
[GitHub] [airflow] XD-DENG opened a new pull request #13232: Enforce PID file path to be absolute when demonize a process (like scheduler, etc)
XD-DENG opened a new pull request #13232:
URL: https://github.com/apache/airflow/pull/13232
Closes #13200 (please refer to #13200 for detailed background and discussion.)
Currently, if the PID file path provided is relative, the process silently fails to launch.
We can choose to expand the relative path to an absolute one on users' behalf, but it will be a confusing decision to expand it to AIRFLOW_HOME or Current directory Or User Home dir.
So the most sensible choice is to enforce providing absolute PID file path, and fail explicitly if the PID file path provided is relative.
This PR also tries to increase coverage a little bit.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13232: Enforce PID file path to be absolute when deamonize a process (like scheduler, etc)
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#discussion_r546910814
##########
File path: airflow/utils/cli.py
##########
@@ -227,8 +227,12 @@ def setup_locations(process, pid=None, stdout=None, stderr=None, log=None):
stdout = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.out')
if not log:
log = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.log')
+
if not pid:
pid = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.pid')
+ else:
+ if not os.path.isabs(pid):
+ raise AirflowException("Path of PID file must be absolute")
Review comment:
That's what I thought of and proposed in the beginning (https://github.com/apache/airflow/issues/13200#issuecomment-749128925). Please check the discussion at #13200
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13232: Enforce PID file path to be absolute when deamonize a process (like scheduler, etc)
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#discussion_r546909644
##########
File path: airflow/utils/cli.py
##########
@@ -227,8 +227,12 @@ def setup_locations(process, pid=None, stdout=None, stderr=None, log=None):
stdout = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.out')
if not log:
log = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.log')
+
if not pid:
pid = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.pid')
+ else:
+ if not os.path.isabs(pid):
+ raise AirflowException("Path of PID file must be absolute")
Review comment:
We should make it relative to cwd instead. https://docs.python.org/3/library/os.path.html#os.path.abspath
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13232: Enforce PID file path to be absolute when deamonize a process (like scheduler, etc)
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#discussion_r546911935
##########
File path: airflow/utils/cli.py
##########
@@ -227,8 +227,12 @@ def setup_locations(process, pid=None, stdout=None, stderr=None, log=None):
stdout = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.out')
if not log:
log = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.log')
+
if not pid:
pid = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.pid')
+ else:
+ if not os.path.isabs(pid):
+ raise AirflowException("Path of PID file must be absolute")
Review comment:
My personal preference aligns with your suggestion.
The current implementation is a "mid-point agreed" after the discussion. Or we should further elaborate the discussion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #13232: Allow PID file path to be relative when deamonize a process (scheduler, kerberos, etc)
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#issuecomment-749700035
This PR is updated to adopt different solution, based on the latest discussion in #13200 .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG merged pull request #13232: Allow PID file path to be relative when daemonize a process (scheduler, kerberos, etc)
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #13232:
URL: https://github.com/apache/airflow/pull/13232
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13232: Enforce PID file path to be absolute when deamonize a process (like scheduler, etc)
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#discussion_r546910017
##########
File path: airflow/utils/cli.py
##########
@@ -227,8 +227,12 @@ def setup_locations(process, pid=None, stdout=None, stderr=None, log=None):
stdout = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.out')
if not log:
log = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.log')
+
if not pid:
pid = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.pid')
+ else:
+ if not os.path.isabs(pid):
+ raise AirflowException("Path of PID file must be absolute")
Review comment:
Current working dir is what every unix program treats a relative path as -- we should be no different.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #13232: Enforce PID file path to be absolute when deamonize a process (like scheduler, etc)
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#discussion_r546957406
##########
File path: airflow/utils/cli.py
##########
@@ -227,8 +227,12 @@ def setup_locations(process, pid=None, stdout=None, stderr=None, log=None):
stdout = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.out')
if not log:
log = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.log')
+
if not pid:
pid = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.pid')
+ else:
+ if not os.path.isabs(pid):
+ raise AirflowException("Path of PID file must be absolute")
Review comment:
As far as I see the "https://linux.die.net/man/1/daemonize" command prints `The 'path' parameter must be an absolute path name` error when passed a relative one. I am not very strong on that, but I think this is expected from a "daemon" program.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #13232: Enforce PID file path to be absolute when deamonize a process (like scheduler, etc)
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#discussion_r546957406
##########
File path: airflow/utils/cli.py
##########
@@ -227,8 +227,12 @@ def setup_locations(process, pid=None, stdout=None, stderr=None, log=None):
stdout = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.out')
if not log:
log = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.log')
+
if not pid:
pid = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.pid')
+ else:
+ if not os.path.isabs(pid):
+ raise AirflowException("Path of PID file must be absolute")
Review comment:
As far as I see the "https://linux.die.net/man/1/daemonize" commands returns `The 'path' parameter must be an absolute path name` when passed a relative one. I am not very strong on that, but I think this is expected from a "daemon" program.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13232: Allow PID file path to be relative when daemonize a process (scheduler, kerberos, etc)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#issuecomment-749798504
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #13232: Allow PID file path to be relative when daemonize a process (scheduler, kerberos, etc)
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#issuecomment-749795770
@ashb any further inputs before we merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13232: Enforce PID file path to be absolute when deamonize a process (like scheduler, etc)
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13232:
URL: https://github.com/apache/airflow/pull/13232#discussion_r546910814
##########
File path: airflow/utils/cli.py
##########
@@ -227,8 +227,12 @@ def setup_locations(process, pid=None, stdout=None, stderr=None, log=None):
stdout = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.out')
if not log:
log = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.log')
+
if not pid:
pid = os.path.join(settings.AIRFLOW_HOME, f'airflow-{process}.pid')
+ else:
+ if not os.path.isabs(pid):
+ raise AirflowException("Path of PID file must be absolute")
Review comment:
That's what I thought of and proposed in the beginning. Please check the discussion at #13200
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org