[DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Daniel Standish <da...@astronomer.io.INVALID>]

Currently KPO uses "core" kubernetes utils for client generation.

*(I'm working on migrating it to use the hook. The effort was waiting on a
few other changes and i'm picking it back up now.)*

In addition to using the core *utils*, it also uses the "core" airflow
kubernetes *settings* from airflow.cfg.  E.g. default namespace, "in
cluster" etc.

My thinking is, we should deprecate consideration of "core" k8s settings in
KPO and only allow (after a major release of the provider) configuration
using the hook.  This way, your KPO tasks are configured by KPO params and
the connection (if provided) and that's it.

But because this could be somewhat disruptive, want to see what people
think.

Does anyone feel very strongly that we should indefinitely let KPO continue
to consider the core k8s settings, and just merge them in with conn
settings if provided?

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Jarek Potiuk <ja...@potiuk.com>]

Actually I thought about it - just do it Daniel. I think it makes no sense
to hold it now. It might take quite a long time to resolve the K8S <> Core
dependency and might even lead to breaking changes so even if we do it
wrong now, we might simply redo it later.

I have no reservations.

J,




On Tue, Apr 26, 2022 at 12:55 AM Jarek Potiuk <ja...@potiuk.com> wrote:

> Sure :) lets discuss on slack; I am just about to cleanup some
> back-log of tasks and happy to talk about it  :).
>
> On Mon, Apr 25, 2022 at 2:32 PM Daniel Standish
> <da...@astronomer.io.invalid> wrote:
> >
> > Appreciate it, Jarek
> >
> > Re your last point
> >
> >> 5) Or MAYBE we should simply incorporate cncf.kubernetes provider
> >> entirely in the core of Airlfow? Maybe there should be NO
> >> "cncf.kubernetes" provider?
> >>
> >> My Answer: This is the point which is the real reason for me being
> >> reluctant here. I see it as quite possible scenario, that we will drop
> >> the provider and all kubernetes code will be simply embedded in
> >> Airlfow Core. I think this is a very interesting and probably least
> >> distruptive scenario. Yes it means that bugfixes will only be
> >> releaseable together with whole Airflow, but K8S is so tightly
> >> connected with Airflow Core via K8S executor that it might be the best
> >> choice. And if we do choose this path, this means that likely the core
> >> settings should simply ... stay in core rather than be moved out.
> >
> >
> > My thought is that even if we went this route (as with any other route),
> it would be desirable for KPO to have its configuration taken in the normal
> way (init params or airflow connection) like any other operator, and let
> the core settings apply just to the executor.
> >
> > But I understand your point about limiting the frequency of breaking
> releases.
> >
> > And yeah I'd be happy too to hop on a call and try and help figure out a
> path forward.
>

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Jarek Potiuk <ja...@potiuk.com>]

Sure :) lets discuss on slack; I am just about to cleanup some
back-log of tasks and happy to talk about it  :).

On Mon, Apr 25, 2022 at 2:32 PM Daniel Standish
<da...@astronomer.io.invalid> wrote:
>
> Appreciate it, Jarek
>
> Re your last point
>
>> 5) Or MAYBE we should simply incorporate cncf.kubernetes provider
>> entirely in the core of Airlfow? Maybe there should be NO
>> "cncf.kubernetes" provider?
>>
>> My Answer: This is the point which is the real reason for me being
>> reluctant here. I see it as quite possible scenario, that we will drop
>> the provider and all kubernetes code will be simply embedded in
>> Airlfow Core. I think this is a very interesting and probably least
>> distruptive scenario. Yes it means that bugfixes will only be
>> releaseable together with whole Airflow, but K8S is so tightly
>> connected with Airflow Core via K8S executor that it might be the best
>> choice. And if we do choose this path, this means that likely the core
>> settings should simply ... stay in core rather than be moved out.
>
>
> My thought is that even if we went this route (as with any other route), it would be desirable for KPO to have its configuration taken in the normal way (init params or airflow connection) like any other operator, and let the core settings apply just to the executor.
>
> But I understand your point about limiting the frequency of breaking releases.
>
> And yeah I'd be happy too to hop on a call and try and help figure out a path forward.

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Daniel Standish <da...@astronomer.io.INVALID>]

Appreciate it, Jarek

Re your last point

5) Or MAYBE we should simply incorporate cncf.kubernetes provider
> entirely in the core of Airlfow? Maybe there should be NO
> "cncf.kubernetes" provider?

My Answer: This is the point which is the real reason for me being
> reluctant here. I see it as quite possible scenario, that we will drop
> the provider and all kubernetes code will be simply embedded in
> Airlfow Core. I think this is a very interesting and probably least
> distruptive scenario. Yes it means that bugfixes will only be
> releaseable together with whole Airflow, but K8S is so tightly
> connected with Airflow Core via K8S executor that it might be the best
> choice. And if we do choose this path, this means that likely the core
> settings should simply ... stay in core rather than be moved out.


My thought is that even if we went this route (as with any other route), it
would be desirable for KPO to have its configuration taken in the normal
way (init params or airflow connection) like any other operator, and let
the core settings apply just to the executor.

But I understand your point about limiting the frequency of breaking
releases.

And yeah I'd be happy too to hop on a call and try and help figure out a
path forward.

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Jarek Potiuk <ja...@potiuk.com>]

Just to give a bit of context - why I think it is important.

It had never ever happened that we had to yank 4 versions of a
provider because of incompatibilities we learned after the fact. And
it's not anyone's fault - i't just learning that we should take into
account. And Cncf.kubernetes is a very special case that had bitten us
in the past several times because of it's tight coupling to the core.
And I think if we do any breaking change we should at least think how
to avoid similar problems in the future. Those are not academic
questions - it already happened (we had similar problems around
Airflow 2 migration in the early Airflow 2 days - so this is an
indication that this is currently "property" of the relation of the
provider with the core so we need to rethink
it)

This change introduces a breaking change in the future, which might
need another breaking change when (or if) we fix the "real" issue we
had and experienced (and which resulted in yanking 4 versions of
cncf.kubernetes provider).

My points are:

* we are introducing a change that will (eventually) make it into a
breaking change (and as you mentioned - potentially disruptive).
* which will likely need another breaking change in the way how KPO is defined
* which means that our users will likely have to go through
incompatibility pains more than once.

I am not against this change - I am just asking for a bit more forward
thinking (and happy to brain-storm in some doc? aip? something more
substantial and suitable than just email thread).

I have some questions (and my current answers are below):

1) Are we sure we want to do it without even attempting to define (not
implementing) how the (maybe imagined) target setup will look like?

My answer: I think not because it might lead to confusion with our
users. K8S for many of our users is important and any change required
will be amplified by a number of people having to do it so we should
limit the number of times they have to do it.

2) Are we sure (or at least it is very likely) that the change we
introduce now will also hold when we solve the real problem ?

I am not sure. I do not know answer to 3) 4) questions to be sure this
change is going to hold.

3)  Are we going to have some fixed version relationship between
Airflow and Cncf.kubernetes? (like we have now: Airflow 2.1 and 2.2 ->
use cncf.kubernetes 3*. Airflow 2.3 -> use cncf.kubernetes 4.*.

My answer: That's one of the options, but it's a bit limiting in terms
of releasing bug-fixes. Likely it will lead to us having to maintain
two branches of cncf.kubernetes provider if we do (when we find a
critical issue). And people who are using 2.1 will have to migrate to
2.3 in order to use any new features in K8S we developed. This is the
current situation we are in and is in a stark contrast with the way
how it works for other providers.
We might deliberately choos that path though - maybe it is better to
keep it this way - with potential price to pay to maintain critical
fixes in two or more branches for the provider. But it should be
deliberate choice knowing the consequences not an accidental
by-product of the versioning approach we choose. Maybe we make a
pledge that there will be no incompatible changes and we will keep 4.0
for as long as we can (but due to changes in kubernetes libarary it
might be not possible - as we already experienced in 3.0 -> 4.0 move).

4) Alternatively - do we know the changes needd to having "true
decoupling" in-place - so that "provider 4.0 and 5.0 will be able to
use ? What needs to be done to get there?

My answer: I do not really know - I do not know details too much and
why the changes we implemented last time were so disruptive and
whether we could keep backwards compatibility if we really wanted. Was
it deliberated braking compatibility because we had no other choice?
Or was it  accidental and we **could** keep the compatibility if we
really wanted? I am not sure. Of course we cannot anticipate what
future kubernetes library will bring but just maybe deciding of what
is our "goal" here and whether it seems to be achievable or not is
something we should do.

5) Or MAYBE we should simply incorporate cncf.kubernetes provider
entirely in the core of Airlfow? Maybe there should be NO
"cncf.kubernetes" provider?

My Answer: This is the point which is the real reason for me being
reluctant here. I see it as quite possible scenario, that we will drop
the provider and all kubernetes code will be simply embedded in
Airlfow Core. I think this is a very interesting and probably least
distruptive scenario. Yes it means that bugfixes will only be
releaseable together with whole Airflow, but K8S is so tightly
connected with Airflow Core via K8S executor that it might be the best
choice. And if we do choose this path, this means that likely the core
settings should simply ... stay in core rather than be moved out.

I am happy to collaborate on that - but I think we at least need to
have a document and discussion on where we are going with this  to
decide on any breaking changes in Kubernetes settings.

J.

On Fri, Apr 15, 2022 at 6:44 PM Daniel Standish
<da...@astronomer.io.invalid> wrote:
>
> Thanks Jarek
>
> I think the settings intermingling is independent from the problem you're concerned with.  I can see how it would be desirable to define the executor interface more robustly, and to allow core to not care about k8s version (so that provider can use whatever k8s version it likes).  But this is really a separate issue.
>
> The issue I'm concerned with is that we have a defined way to configure hooks and operators Airflow: (1) the Airflow Connection or (2) direct config through operator or hook params.  We do not do this via the `airflow.cfg` file.  Resolving this inconsistency does not solve the problem you are concerned with; but it rectifies a user-facing inconsistency and a source of confusion.
>
> Whether the K8s executor is ever moved out of core or not, it will remain desirable that KPO only takes configuration from Airflow Connection or direct params, because that's how things are done in Airflow.  The core `[kubernetes]` settings should apply to the executor but not the operator or the hook.  And indeed, by and large, this is the case already; there are just a few `airflow.cfg` settings that affect KPO and the vast majority do not.
>
> WDYT?

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Daniel Standish <da...@astronomer.io.INVALID>]

Thanks Jarek

I think the settings intermingling is independent from the problem you're
concerned with.  I can see how it would be desirable to define the
executor interface more robustly, and to allow core to not care about k8s
version (so that provider can use whatever k8s version it likes).  But
this is really a separate issue.

The issue I'm concerned with is that we have a defined way to configure
hooks and operators Airflow: (1) the Airflow Connection or (2) direct
config through operator or hook params.  We do not do this via the
`airflow.cfg` file.  Resolving this inconsistency does not solve the
problem you are concerned with; but it rectifies a user-facing
inconsistency and a source of confusion.

Whether the K8s executor is ever moved out of core or not, it will remain
desirable that KPO only takes configuration from Airflow Connection or
direct params, because that's how things are done in Airflow.  The core
`[kubernetes]` settings should apply to the executor but not the operator
or the hook.  And indeed, by and large, this is the case already; there are
just a few `airflow.cfg` settings that affect KPO and the vast majority do
not.

WDYT?

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Jarek Potiuk <ja...@potiuk.com>]

I just (when reviewing PRs) found a good example to illustrate my concerns:

https://github.com/apache/airflow/pull/22253

I commented there why I think we need to have a good plan on what to
do with the kubernetes provider before we decide what to do with the
settings:

https://github.com/apache/airflow/pull/22253#issuecomment-1096643179

Otherwise we might need to revert the decision if it turns out that
complete decoupling is not possible/feasible/we do not want it and
revert to pulling the cncf.kubernetes code back to the core as an
"easier" solution.

I think coming up with a plan of what we should do with
`cncf.kubernetes` will give us clarity on what we should do with the
settings.

On Tue, Apr 12, 2022 at 10:58 AM Jarek Potiuk <ja...@potiuk.com> wrote:
>
> What I am saying is that we should deprecate those settings in core -
> but only if it is part of decoupling Kubrenetes Executor from the core
> - otherwise this change gives us pretty much no improvement.
>
> In order to do that - we need to at least know what changes will be
> needed in  the core - standardizing Executor's behaviours to make them
> pluggable - and see if any of the changes will be breaking
> compatibility and need Airflow 3 (possibly we can do it in the way
> that it won't be needed).
>
> This change might come in stages - for example deprecating  k8s core
> settings might be the first change (or maybe not - but we need to have
> a plan).
>
> I think just deprecating  settings gives very little if it is not
> accompanied with at least a plan on how to decouple the
> cncf.kubernetes provider (and especially Kubernetes Executor) from the
> core.
>
> So what I **think** should happen now:
>
> 1) We should have a proposal (AIP ? ) on how to decouple Executors
> (including K8S Executor) from the core (which should contain some kind
> of inventory of what should happen to decouple it).
> 2) We should propose a staged approach on how to do it and what we can
> do without actually breaking compatibility (maybe after the analysis
> we will find out that we can do it without breaking the
> compatibility).
> 3) We should see how deprecation of core settings fits into this plan.
>
> I think otherwise we might simply move the problem elsewhere rather
> than plan to solve it. **Just** moving settings out simply masks the
> problem.
>
> The problem is that no matter where the settings are, the core K8S
> executor has a very close binding with the provider and one cannot
> leave without the other in the right versions and changing one impacts
> the other in unforeseen ways (as we experienced recently when we had
> to yank few versions of the cncf.kubernetes provider)
>
> This is the really problem to solve, not where the settings are.
>
> J.
>
> On Mon, Apr 4, 2022 at 6:38 PM Daniel Standish
> <da...@astronomer.io.invalid> wrote:
> >
> > Thanks Jarek
> >
> > I register a bit of a mixed message.
> >
> > Re this
> >
> >> Maybe I am wrong but I think the separation should be solved together,
> >> there is a good reason why some k8s settings are in core.
> >
> >
> > Are you saying I should not proceed with this plan (add warnings in KPO that push users to start using K8s Hook connection for config instead of "core" k8s config settings before 3.0) until we figure out the other things you referenced, or that you are in favor of doing this deprecation but you also think that we should resolve those other concerns too by the time we actually remove support?
> >
> > Thanks
> >

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Jarek Potiuk <ja...@potiuk.com>]

What I am saying is that we should deprecate those settings in core -
but only if it is part of decoupling Kubrenetes Executor from the core
- otherwise this change gives us pretty much no improvement.

In order to do that - we need to at least know what changes will be
needed in  the core - standardizing Executor's behaviours to make them
pluggable - and see if any of the changes will be breaking
compatibility and need Airflow 3 (possibly we can do it in the way
that it won't be needed).

This change might come in stages - for example deprecating  k8s core
settings might be the first change (or maybe not - but we need to have
a plan).

I think just deprecating  settings gives very little if it is not
accompanied with at least a plan on how to decouple the
cncf.kubernetes provider (and especially Kubernetes Executor) from the
core.

So what I **think** should happen now:

1) We should have a proposal (AIP ? ) on how to decouple Executors
(including K8S Executor) from the core (which should contain some kind
of inventory of what should happen to decouple it).
2) We should propose a staged approach on how to do it and what we can
do without actually breaking compatibility (maybe after the analysis
we will find out that we can do it without breaking the
compatibility).
3) We should see how deprecation of core settings fits into this plan.

I think otherwise we might simply move the problem elsewhere rather
than plan to solve it. **Just** moving settings out simply masks the
problem.

The problem is that no matter where the settings are, the core K8S
executor has a very close binding with the provider and one cannot
leave without the other in the right versions and changing one impacts
the other in unforeseen ways (as we experienced recently when we had
to yank few versions of the cncf.kubernetes provider)

This is the really problem to solve, not where the settings are.

J.

On Mon, Apr 4, 2022 at 6:38 PM Daniel Standish
<da...@astronomer.io.invalid> wrote:
>
> Thanks Jarek
>
> I register a bit of a mixed message.
>
> Re this
>
>> Maybe I am wrong but I think the separation should be solved together,
>> there is a good reason why some k8s settings are in core.
>
>
> Are you saying I should not proceed with this plan (add warnings in KPO that push users to start using K8s Hook connection for config instead of "core" k8s config settings before 3.0) until we figure out the other things you referenced, or that you are in favor of doing this deprecation but you also think that we should resolve those other concerns too by the time we actually remove support?
>
> Thanks
>

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Daniel Standish <da...@astronomer.io.INVALID>]

Thanks Jarek

I register a bit of a mixed message.

Re this

Maybe I am wrong but I think the separation should be solved together,
> there is a good reason why some k8s settings are in core.
>

Are you saying I should not proceed with this plan *(add warnings in KPO
that push users to start using K8s Hook connection for config instead of
"core" k8s config settings before 3.0)* until we figure out the other
things you referenced, or that you are in favor of doing this deprecation
but you also think that we should resolve those other concerns too by the
time we actually remove support?

Thanks

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Jarek Potiuk <ja...@potiuk.com>]

All for it.

I think part of the k8s configuration being in the "core" is because
of its relation to K8s executor. We should make sure that Dask,
Celery, Kubernetes executors should come with their own "extras" (i.e.
need to install a provider) and  have an "own" configuration. I will
require some core behaviour change as we "hard-code' some of those
executor checks in core code.

I think, this is very much coupled with the discussion here:
https://lists.apache.org/thread/cycwt15bqr76vdol87477v808f1c2d1x
where the conclusion is that we should finally agree on the Executor
responsibilities and interface.

Maybe I am wrong but I think the separation should be solved together,
there is a good reason why some k8s settings are in core.

J.

On Sat, Apr 2, 2022 at 1:00 AM Daniel Standish
<da...@astronomer.io.invalid> wrote:
>
> Small clarification, KPO doesn't currently take into account the default namespace specified in kube settings.  AFAIK it considers the following:
>
> in_cluster
> cluster_context
> config_file
> enable_tcp_keepalive
> verify_ssl
>
> And so my proposal would be that, after a deprecation period, we only derive these settings from the hook + KPO params.  During deprecation period, we would emit a warning when the effective setting (i.e. the setting derived after applying all config sources in order of precedence) would be different after core airflow K8s config is excluded from KPO config.
>

Re: [DISCUSS] Deprecate core airflow k8s settings in KubernetesPodOperator

[Daniel Standish <da...@astronomer.io.INVALID>]

Small clarification, KPO doesn't currently take into account the default
namespace specified in kube settings.  AFAIK it considers the following:

   - in_cluster
   - cluster_context
   - config_file
   - enable_tcp_keepalive
   - verify_ssl

And so my proposal would be that, after a deprecation period, we only
derive these settings from the hook + KPO params.  During
deprecation period, we would emit a warning when the effective setting
(i.e. the setting derived after applying all config sources in order of
precedence) would be different after core airflow K8s config is excluded
from KPO config.