-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Allow Remote logging providers to load connections from the API Server #53719
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.
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
Allow Remote logging providers to load connections from the API Server #53719
Conversation
Often remote logging is down using automatic instance profiles, but not always. If you tried to configure a logger by a connection defined in the metadata DB it would have not worked (it either caused the supervise job to fail early, or to just behave as if the connection didn't exist, depending on the hook's behaviour) Unfortunately, the way of knowing what the default connection ID various hooks use is not easily discoverable, at least not easily from the outside (we can't look at `remote.hook` as for most log providers that would try to load the connection, failing in the way we are trying to fix) so I updated the log config module to keep track of what the default conn id is for the modern log providers. Once we have the connection ID we know (or at least have a good idea that we've got the right one) we then pre-emptively check the secrets backends for it, if not found there load it from the API server, and then either way. if we find a connection we put it in the env variable so that it is available. The reason we use this approach, is that are running in the supervisor process itself, so SUPERVISOR_COMMS is not and cannot be set yet.
|
This is messier and more complex than I like -- any idea on w a way of cleaning this up greatly appreciated. |
We should change remote logging templates for loggers to be part of provider's packages as separate files that should be dropped to the same directory where "generic" airflow config is - and our logging configuration should discover the remote logging file dropped there and read it from there. Such remote logging config that should be copy*pasteable from provider's sources (and docs) should have everything needed for the core to be able to configure it in a generic way. That should remove the coupling of airlow core from essentially provider feature and make it a bit less messy. |
amoghrajesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would see solving this problem in a multiphase way. For unblocking users from immediately using remote logging to load connections and as a short term / mid term fix, I see this solution to be OK.
Over a longer term, I would like to propose and agree with @potiuk's suggestion here, which is asking every provider to have their own logging config files setup to decouple core from knowing what should be present in a provider's logging config, but just knowing how to discover it.
For ex, google could define a providers/google/logging/gcs_remote_logging.yaml with details such as:
logging_schemes:
- scheme: "gs://"
handler_class: "airflow.providers.google.cloud.log.gcs_task_handler.GCSRemoteLogIO"
default_connection_id: "google_cloud_default"
required_config:
- "logging.remote_base_log_folder"
- "logging.google_key_path"
and get done with it. Core can define a discovery mechanism to be ok with it.
For now, I am ok with this PR.
|
@potiuk the suggestions from you are thoughts that are relevant more to the process as you said. This PR addresses the fact that the connection to be used still comes from configuration and even with better discovery etc, nothing changes on that part. |
|
Yep. I was just responding to @ashb call to suggestions :) "any idea on w a way of cleaning this up greatly appreciated.". Generally speaking that PR looks good to me as well - besides (as Ash mentioned) being messy :) |
|
And the messiness is because of legacy "embedding" of provider things in core - not because the PR is messy on its own :D |
…he API Server (#53719) Often remote logging is down using automatic instance profiles, but not always. If you tried to configure a logger by a connection defined in the metadata DB it would have not worked (it either caused the supervise job to fail early, or to just behave as if the connection didn't exist, depending on the hook's behaviour) Unfortunately, the way of knowing what the default connection ID various hooks use is not easily discoverable, at least not easily from the outside (we can't look at `remote.hook` as for most log providers that would try to load the connection, failing in the way we are trying to fix) so I updated the log config module to keep track of what the default conn id is for the modern log providers. Once we have the connection ID we know (or at least have a good idea that we've got the right one) we then pre-emptively check the secrets backends for it, if not found there load it from the API server, and then either way. if we find a connection we put it in the env variable so that it is available. The reason we use this approach, is that are running in the supervisor process itself, so SUPERVISOR_COMMS is not and cannot be set yet. (cherry picked from commit e4fb686) Co-authored-by: Ash Berlin-Taylor <ash@apache.org>
…he API Server (#53719) Often remote logging is down using automatic instance profiles, but not always. If you tried to configure a logger by a connection defined in the metadata DB it would have not worked (it either caused the supervise job to fail early, or to just behave as if the connection didn't exist, depending on the hook's behaviour) Unfortunately, the way of knowing what the default connection ID various hooks use is not easily discoverable, at least not easily from the outside (we can't look at `remote.hook` as for most log providers that would try to load the connection, failing in the way we are trying to fix) so I updated the log config module to keep track of what the default conn id is for the modern log providers. Once we have the connection ID we know (or at least have a good idea that we've got the right one) we then pre-emptively check the secrets backends for it, if not found there load it from the API server, and then either way. if we find a connection we put it in the env variable so that it is available. The reason we use this approach, is that are running in the supervisor process itself, so SUPERVISOR_COMMS is not and cannot be set yet. (cherry picked from commit e4fb686) Co-authored-by: Ash Berlin-Taylor <ash@apache.org>
…he API Server (#53719) (#53761) Often remote logging is down using automatic instance profiles, but not always. If you tried to configure a logger by a connection defined in the metadata DB it would have not worked (it either caused the supervise job to fail early, or to just behave as if the connection didn't exist, depending on the hook's behaviour) Unfortunately, the way of knowing what the default connection ID various hooks use is not easily discoverable, at least not easily from the outside (we can't look at `remote.hook` as for most log providers that would try to load the connection, failing in the way we are trying to fix) so I updated the log config module to keep track of what the default conn id is for the modern log providers. Once we have the connection ID we know (or at least have a good idea that we've got the right one) we then pre-emptively check the secrets backends for it, if not found there load it from the API server, and then either way. if we find a connection we put it in the env variable so that it is available. The reason we use this approach, is that are running in the supervisor process itself, so SUPERVISOR_COMMS is not and cannot be set yet. (cherry picked from commit e4fb686) Co-authored-by: Ash Berlin-Taylor <ash@apache.org>
apache#53719) Often remote logging is down using automatic instance profiles, but not always. If you tried to configure a logger by a connection defined in the metadata DB it would have not worked (it either caused the supervise job to fail early, or to just behave as if the connection didn't exist, depending on the hook's behaviour) Unfortunately, the way of knowing what the default connection ID various hooks use is not easily discoverable, at least not easily from the outside (we can't look at `remote.hook` as for most log providers that would try to load the connection, failing in the way we are trying to fix) so I updated the log config module to keep track of what the default conn id is for the modern log providers. Once we have the connection ID we know (or at least have a good idea that we've got the right one) we then pre-emptively check the secrets backends for it, if not found there load it from the API server, and then either way. if we find a connection we put it in the env variable so that it is available. The reason we use this approach, is that are running in the supervisor process itself, so SUPERVISOR_COMMS is not and cannot be set yet.
apache#53719) Often remote logging is down using automatic instance profiles, but not always. If you tried to configure a logger by a connection defined in the metadata DB it would have not worked (it either caused the supervise job to fail early, or to just behave as if the connection didn't exist, depending on the hook's behaviour) Unfortunately, the way of knowing what the default connection ID various hooks use is not easily discoverable, at least not easily from the outside (we can't look at `remote.hook` as for most log providers that would try to load the connection, failing in the way we are trying to fix) so I updated the log config module to keep track of what the default conn id is for the modern log providers. Once we have the connection ID we know (or at least have a good idea that we've got the right one) we then pre-emptively check the secrets backends for it, if not found there load it from the API server, and then either way. if we find a connection we put it in the env variable so that it is available. The reason we use this approach, is that are running in the supervisor process itself, so SUPERVISOR_COMMS is not and cannot be set yet.
Often remote logging is down using automatic instance profiles, but not
always. If you tried to configure a logger by a connection defined in the
metadata DB it would have not worked (it either caused the supervise job to
fail early, or to just behave as if the connection didn't exist, depending on
the hook's behaviour)
Unfortunately, the way of knowing what the default connection ID various hooks
use is not easily discoverable, at least not easily from the outside (we can't
look at
remote.hookas for most log providers that would try to load theconnection, failing in the way we are trying to fix) so I updated the log
config module to keep track of what the default conn id is for the modern log
providers.
Once we have the connection ID we know (or at least have a good idea that
we've got the right one) we then pre-emptively check the secrets backends for
it, if not found there load it from the API server, and then either way. if we
find a connection we put it in the env variable so that it is available.
The reason we use this approach, is that are running in the supervisor process
itself, so SUPERVISOR_COMMS is not and cannot be set yet.
Discovered when digging in to #52501 -- it might fix the problem
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.