Skip to content

Comments

Rename 'resources' arg in Kub op to container_resources#24673

Merged
uranusjr merged 1 commit intoapache:mainfrom
astronomer:rename-k8s-resources
Jun 28, 2022
Merged

Rename 'resources' arg in Kub op to container_resources#24673
uranusjr merged 1 commit intoapache:mainfrom
astronomer:rename-k8s-resources

Conversation

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Jun 27, 2022

Using 'resources' to mean a different thing is incompatible with the base operator. It breaks Liskov substitution and results in incorrect task mapping behavior. Renaming the argument fixes everything.

Fix #23783.

@uranusjr uranusjr requested a review from jedcunningham as a code owner June 27, 2022 04:21
@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers labels Jun 27, 2022
eladkal
eladkal approved these changes Jun 27, 2022
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think resource parameter in BaseOperator is not really used anywhere?
I see also ash pointed it out before #16563 (comment)

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jun 27, 2022
@github-actions
Copy link

github-actions bot commented Jun 27, 2022

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr
Copy link
Member Author

uranusjr commented Jun 27, 2022

Yeah, right now resources is only used by CgroupTaskRunner. Maybe someone could implement some kind of interface to use resources with Kubernetes, but directly passing in a V1ResourceRequirements is not the way to do it.

@uranusjr uranusjr force-pushed the rename-k8s-resources branch from 6018d81 to 6c1cf24 Compare June 27, 2022 12:07
@jedcunningham jedcunningham requested a review from dstandish June 27, 2022 18:35
@dstandish
Copy link
Contributor

dstandish commented Jun 27, 2022

wow i had no clue we had this thing

eladkal
eladkal reviewed Jun 27, 2022
View reviewed changes
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py Outdated
Copy link
Contributor

@eladkal eladkal Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just something to think about
Do we want k8s_ or kubernetes_ ?

Copy link

@savingoyal savingoyal Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be a breaking change with a significant impact on user DAGs? Curious, what is the backward compatibility philosophy for such changes?

Copy link
Contributor

@dstandish dstandish Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or what about pod_resources -- this seems better

Copy link
Contributor

@dstandish dstandish Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option would be to accept task_resources and forward this to BaseOperator resources and keep resources to mean pod resources but probably no one will like this idea ;)

Copy link
Member

@jedcunningham jedcunningham Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or what about pod_resources -- this seems better

container_resources would be better (it is set on the container after all)

Copy link
Contributor

@dstandish dstandish Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah interesting yeah sounds good

Copy link
Member Author

@uranusjr uranusjr Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s go with container_resources then.

@savingoyal Passing resources will continue to work, emitting a deprecation warning. You must use the new argument for task mapping, but that doesn’t work right now anyway so nothing is broken.

Copy link
Contributor

@eladkal eladkal Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

container_resources sounds good

@uranusjr
Rename 'resources' arg in Kub op to k8s_resources
2cc2d57 
Using 'resources' to mean a different thing is incompatible with the
base operator. It breaks Liskov substitution and results in incorrect
task mapping behavior. Renaming the argument fixes everything.
@uranusjr uranusjr force-pushed the rename-k8s-resources branch from 6c1cf24 to 2cc2d57 Compare June 28, 2022 04:31
@uranusjr uranusjr merged commit 45f4290 into apache:main Jun 28, 2022
@uranusjr uranusjr deleted the rename-k8s-resources branch June 28, 2022 06:45
@valayDave
Copy link

valayDave commented Jul 9, 2022

What version of the Kubernetes operator will incorporate this change? Any idea when this change will be incorporated into the public release?

@potiuk
Copy link
Member

potiuk commented Jul 10, 2022 •
edited
Loading

We release providers roughly monthly - the next wave is coming in the next few days + min 3 days for voting. But the actual release time depends very much on testing i tby the users.

I will male sure to add you to the least of people whowwen i release the providers @valayDave so that you can test it and confirm that it solves your problem.

Generally speaking the sooner people like you confirm that the relaese candidate is sound, the more likely it will be released faster

@valayDave
Copy link

valayDave commented Jul 26, 2022 •
edited
Loading

Hello! which release will this be a part of ?

@uranusjr
Copy link
Member Author

uranusjr commented Jul 27, 2022

This is already in the latest apache-airflow-providers-cncf-kubernetes release.

@uranusjr uranusjr changed the title Rename 'resources' arg in Kub op to k8s_resources Rename 'resources' arg in Kub op to container_resources Jul 27, 2022
@valayDave
Copy link

valayDave commented Jul 29, 2022 •
edited
Loading

This is not in the latest documentation hence I asked. I see docs only until 4.2.0 for the kuberentes provider.
^^^ Ignore this comment. I just saw it in the release notes. Thanks.

valayDave added a commit to valayDave/metaflow that referenced this pull request Jul 29, 2022
@valayDave
Uncommented code for foreach support with k8s
a7e1ecd 
KubernetesPodOperator version 4.2.0 renamed `resources` to
`container_resources`
- Check : (apache/airflow#24673) /
- (apache/airflow@45f4290)

This was done because `KubernetesPodOperator` didn't play nice with dynamic task mapping and they had to deprecate the `resources` argument. Hence the below codepath checks for the version of `KubernetesPodOperator`
and then sets the argument. If the version < 4.2.0 then we set the argument as `resources`.
If it is > 4.2.0 then we set the argument as `container_resources`
The `resources` argument of KuberentesPodOperator is going to be deprecated soon in the future.
So we will only use it for `KuberentesPodOperator` version < 4.2.0
The `resources` argument will also not work for foreach's.
valayDave added a commit to valayDave/metaflow that referenced this pull request Jul 29, 2022
@valayDave
Airflow on Kubernetes minus Foreachs.
ed30060 
- Support for all metaflow construct without foreach and sensors

Squashed commit of the following:

commit ef8b1e3
Author: Valay Dave <[email protected]>
Date:   Fri Jul 29 01:06:26 2022 +0000

    Removed sernsors and banned foreach's

commit 8d517c4
Author: Valay Dave <[email protected]>
Date:   Fri Jul 29 00:59:01 2022 +0000

    commiting k8s related file from master.

commit a7e1ecd
Author: Valay Dave <[email protected]>
Date:   Fri Jul 29 00:54:45 2022 +0000

    Uncommented code for foreach support with k8s

    KubernetesPodOperator version 4.2.0 renamed `resources` to
    `container_resources`
    - Check : (apache/airflow#24673) /
    - (apache/airflow@45f4290)

    This was done because `KubernetesPodOperator` didn't play nice with dynamic task mapping and they had to deprecate the `resources` argument. Hence the below codepath checks for the version of `KubernetesPodOperator`
    and then sets the argument. If the version < 4.2.0 then we set the argument as `resources`.
    If it is > 4.2.0 then we set the argument as `container_resources`
    The `resources` argument of KuberentesPodOperator is going to be deprecated soon in the future.
    So we will only use it for `KuberentesPodOperator` version < 4.2.0
    The `resources` argument will also not work for foreach's.

commit 2719f5d
Author: Valay Dave <[email protected]>
Date:   Mon Jul 18 18:31:58 2022 +0000

    nit fixes :
    - fixing comments.
    - refactor some variable/function names.

commit 2079293
Author: Valay Dave <[email protected]>
Date:   Mon Jul 18 18:14:53 2022 +0000

    change `token` to `production_token`

commit 14aad5f
Author: Valay Dave <[email protected]>
Date:   Mon Jul 18 18:11:56 2022 +0000

    Refactored import Airflow Sensors.

commit b1472d5
Author: Valay Dave <[email protected]>
Date:   Mon Jul 18 18:08:41 2022 +0000

    new comment on `startup_timeout_seconds` env var.

commit 6d81b75
Author: Valay Dave <[email protected]>
Date:   Mon Jul 18 18:06:09 2022 +0000

    Removing traces of `@airflow_schedule_interval`

commit 0673db7
Author: Valay Dave <[email protected]>
Date:   Thu Jul 14 12:43:08 2022 -0700

    Foreach polish (#62)

    * Removing unused imports
    * Added validation logic for airflow version numbers with foreaches
    * Removed `airflow_schedule_interval` decorator.

    * Added production/deployment token related changes
    - Uses s3 as a backend to store the production token
    - Token used for avoiding nameclashes
    - token stored via `FlowDatastore`

    * Graph type validation for airflow foreachs
    - Airflow foreachs only support single node fanout.
    - validation invalidates graphs with nested foreachs

    * Added configuration about startup_timeout.

    * Added final todo on `resources` argument of k8sOp
    - added a commented code block
    - it needs to be uncommented when airflow releasese the patch for the op
    - Code seems feature complete keeping aside airflow patch

commit 4b2dd12
Author: Valay Dave <[email protected]>
Date:   Thu Jul 7 19:33:07 2022 +0000

    Removed retries from user-defaults.

commit 0e87a97
Author: Valay Dave <[email protected]>
Date:   Wed Jul 6 16:29:33 2022 +0000

    updated pod startup time

commit fce2bd2
Author: Valay Dave <[email protected]>
Date:   Wed Jun 29 18:44:11 2022 +0000

    Adding default 1 retry for any airflow worker.

commit 5ef6bbc
Author: Valay Dave <[email protected]>
Date:   Mon Jun 27 01:22:42 2022 +0000

    Airflow Foreach Integration
    - Simple one node foreach-join support as gaurenteed by airflow
    - Fixed env variable setting issue
    - introduced MetaflowKuberentesOperator
    - Created a new operator to allow smootness in plumbing xcom values
    - Some todos

commit d319fa9
Author: Valay Dave <[email protected]>
Date:   Fri Jun 24 21:12:09 2022 +0000

    simplifying run-id macro.

commit 0ffc813
Author: Valay Dave <[email protected]>
Date:   Fri Jun 24 11:51:42 2022 -0700

    Refactored parameter macro settings. (#60)

commit a3a4950
Author: Valay Dave <[email protected]>
Date:   Fri Jun 24 02:05:57 2022 +0000

    added comment on need for `start_date`

commit a3147be
Author: Valay Dave <[email protected]>
Date:   Tue Jun 21 06:03:56 2022 +0000

    Refactored an `id_creator` method.

commit 04d7f20
Author: Valay Dave <[email protected]>
Date:   Tue Jun 21 05:52:05 2022 +0000

    refactor :
    -`RUN_ID_LEN` to `RUN_HASH_ID_LEN`
    - `TASK_ID_LEN` to `TASK_ID_HASH_LEN`

commit cde4605
Author: Valay Dave <[email protected]>
Date:   Tue Jun 21 05:48:55 2022 +0000

    refactored an error string

commit 1145818
Author: Valay Dave <[email protected]>
Date:   Mon Jun 20 22:42:36 2022 -0700

    addressing  savins comments. (#59)

    - Added many adhoc changes based for some comments.
    - Integrated secrets and `KUBERNETES_SECRETS`
    - cleaned up parameter setting
    - cleaned up setting of scheduling interval
    - renamed `AIRFLOW_TASK_ID_TEMPLATE_VALUE` to `AIRFLOW_TASK_ID`
    - renamed `AirflowSensorDecorator.compile` to `AirflowSensorDecorator.validate`
    - Checking if dagfile and flow file are same.
    - fixing variable names.
    - checking out `kubernetes_decorator.py` from master (6441ed5)
    - bug fixing secret setting in airflow.
    - simplified parameter type parsing logic
    - refactoring airflow argument parsing code.

commit 83b20a7
Author: Valay Dave <[email protected]>
Date:   Mon Jun 13 14:02:57 2022 -0700

    Addressing Final comments.  (#57)

    - Added dag-run timeout.
    - airflow related scheduling checks in decorator.
    - Auto naming sensors if no name is provided
    - Annotations to k8s operators
    - fix: argument serialization for `DAG` arguments (method names refactored like `to_dict` became `serialize`)
    - annotation bug fix
    - setting`workflow-timeout` for only scheduled dags

commit 4931f9c
Author: Valay Dave <[email protected]>
Date:   Mon Jun 6 04:50:49 2022 +0000

    k8s bug fix

commit 200ae8e
Author: Valay Dave <[email protected]>
Date:   Mon Jun 6 04:39:50 2022 +0000

    removed un-used function

commit 70e285e
Author: Valay Dave <[email protected]>
Date:   Mon Jun 6 04:38:37 2022 +0000

    Removed unused `sanitize_label` function

commit 84fc622
Author: Valay Dave <[email protected]>
Date:   Mon Jun 6 04:37:34 2022 +0000

    GPU support added + container naming same as argo

commit c92280d
Author: Valay Dave <[email protected]>
Date:   Mon Jun 6 04:25:17 2022 +0000

    Refactored sensors to different files + bug fix
    - bug caused due `util.compress_list`.
    - The function doesn't play nice with strings with variety of characters.
    - Ensured that exceptions are handled appropriately.
    - Made new file for each sensor under `airflow.sensors` module.

commit b72a1dc
Author: Valay Dave <[email protected]>
Date:   Sat Jun 4 01:41:49 2022 +0000

    ran black.

commit 558c82f
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 18:32:48 2022 -0700

    Moving information from airflow_utils to compiler (#56)

    - commenting todos to organize unfinished changes.
    - some environment variables set via`V1EnvVar`
        - `client.V1ObjectFieldSelector` mapped env vars were not working in json form
        - Moving k8s operator import into its own function.
        - env vars moved.

commit 9bb5f63
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 18:06:03 2022 +0000

    added mising Run-id prefixes to variables.
    - merged `hash` and `dash_connect` filters.

commit 37b5e6a
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 18:00:22 2022 +0000

    nit fix : variable name change.

commit 660756f
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 17:58:34 2022 +0000

    nit fixes to dag.py's templating variables.

commit 1202f5b
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 17:56:53 2022 +0000

    Fixed defaults passing
    - Addressed comments for airflow.py

commit b9387dd
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 17:52:24 2022 +0000

    Following Changes:
    - Refactors setting scheduling interval
    - refactor dag file creating function
    - refactored is_active to is_paused_upon_creation
    - removed catchup

commit 054e3f3
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 17:33:25 2022 +0000

    Multiple Changes based on comments:
    1. refactored `create_k8s_args` into _to_job
    2. Addressed comments for snake casing
    3. refactored `attrs` for simplicity.
    4. refactored `metaflow_parameters` to `parameters`.
    5. Refactored setting of `input_paths`

commit d481b2f
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 16:42:24 2022 +0000

    Removed Sensor metadata extraction.

commit d8e6ec0
Author: Valay Dave <[email protected]>
Date:   Fri Jun 3 16:30:34 2022 +0000

    porting savin's comments
    - next changes : addressing comments.

commit 3f2353a
Merge: d370ffb c1ff469
Author: Valay Dave <[email protected]>
Date:   Thu Jul 28 23:52:16 2022 +0000

    Merge branch 'master' into airflow

commit d370ffb
Merge: a82f144 e4eb751
Author: Valay Dave <[email protected]>
Date:   Thu Jul 14 19:38:48 2022 +0000

    Merge branch 'master' into airflow

commit a82f144
Merge: bdb1f0d 6f097e3
Author: Valay Dave <[email protected]>
Date:   Wed Jul 13 00:35:49 2022 +0000

    Merge branch 'master' into airflow

commit bdb1f0d
Merge: 8511215 f9a4968
Author: Valay Dave <[email protected]>
Date:   Wed Jun 29 18:44:51 2022 +0000

    Merge branch 'master' into airflow

commit 8511215
Author: Valay Dave <[email protected]>
Date:   Tue Jun 21 02:53:11 2022 +0000

    Bug fix from master merge.

commit 90c06f1
Merge: 0fb73af 6441ed5
Author: Valay Dave <[email protected]>
Date:   Mon Jun 20 21:20:20 2022 +0000

    Merge branch 'master' into airflow

commit 0fb73af
Author: Valay Dave <[email protected]>
Date:   Sat Jun 4 00:53:10 2022 +0000

    squashing bugs after changes from master.

commit 09c6ba7
Merge: 7bdf662 ffff49b
Author: Valay Dave <[email protected]>
Date:   Sat Jun 4 00:20:38 2022 +0000

    Merge branch 'master' into af-mmr

commit 7bdf662
Author: Valay Dave <[email protected]>
Date:   Mon May 16 17:42:38 2022 -0700

    Airflow sensor api (#3)

    * Fixed run-id setting
    - Change gaurentees that multiple dags triggered at same moment have unique run-id

    * added allow multiple in `Decorator` class

    * Airflow sensor integration.
     >> support added for :
    - ExternalTaskSensor
    - S3KeySensor
    - SqlSensor
    >> sensors allow multiple decorators
    >> sensors accept those arguments which are supported by airflow

    * Added `@airflow_schedule_interval` decorator
    * Fixing bug run-id related in env variable setting.

commit 2604a29
Author: Valay Dave <[email protected]>
Date:   Thu Apr 21 18:26:59 2022 +0000

    Addressed comments.

commit 584e88b
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 03:33:55 2022 +0000

    fixed printing bug

commit 169ac15
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 03:30:59 2022 +0000

    Option help bug fix.

commit 6f8489b
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 03:25:54 2022 +0000

    variable renamemetaflow_specific_args

commit 0c779ab
Merge: d299b13 5a61508
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 03:23:10 2022 +0000

    Merge branch 'airflow-tests' into airflow

commit 5a61508
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 03:22:54 2022 +0000

    Removing un-used code / resolved-todos.

commit d030830
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 03:06:03 2022 +0000

    ran black,

commit 2d1fc06
Merge: f2cb319 7921d13
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 03:04:19 2022 +0000

    Merge branch 'master' into airflow-tests

commit d299b13
Merge: f2cb319 7921d13
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 03:02:37 2022 +0000

    Merge branch 'master' into airflow

commit f2cb319
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 02:54:03 2022 +0000

    reverting change.

commit 05b9db9
Author: Valay Dave <[email protected]>
Date:   Wed Apr 20 02:47:41 2022 +0000

    3 changes:
    - Removing s3 dep
    - remove uesless import
    - added `deployed_on` in dag file template

commit c6afba9
Author: Valay Dave <[email protected]>
Date:   Fri Apr 15 22:50:52 2022 +0000

    Fixed passing secrets with kubernetes.

commit c3ce7e9
Author: Valay Dave <[email protected]>
Date:   Fri Apr 15 22:04:22 2022 +0000

    Refactored code .
    - removed compute/k8s.py
    - Moved k8s code to airflow_compiler.py
    - ran isort to airflow_compiler.py

commit d1c343d
Author: Valay Dave <[email protected]>
Date:   Fri Apr 15 18:02:25 2022 +0000

    Added validations about:
    - un-supported decorators
    - foreach
    Changed where validations