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 2021/08/06 16:56:46 UTC
[GitHub] [airflow] enima2684 opened a new pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
enima2684 opened a new pull request #17474:
URL: https://github.com/apache/airflow/pull/17474
This PR adds the following functionality to the DockerSwarmOperator :
- support for setting secrets and configs exposed to the started service
- support for specifying networks that can be joined by the containers of the service
- support for specifying the number of replicas for the swarm service
This features are essential in order to run the DockerSwarmOperator on a production environment with shared secrets, configs and networks.
The issue #7955 also mentions the need for this features.
P.S: this is my first contribution to Airflow. Tried to follow the contribution guidelines. Forgive any missed step and looking forward for your feedback !
<!--
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/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] enima2684 commented on pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-895847448
Fixed the imports as asked :)
--
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] uranusjr commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r684986073
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -121,12 +153,16 @@ def _run_service(self) -> None:
env=self.environment,
user=self.user,
tty=self.tty,
+ configs=self.configs,
+ secrets=self.secrets,
),
restart_policy=types.RestartPolicy(condition='none'),
resources=types.Resources(mem_limit=self.mem_limit),
+ networks=self.networks,
),
name=f'airflow-{get_random_string()}',
labels={'name': f'airflow__{self.dag_id}__{self.task_id}'},
+ mode=types.ServiceMode(mode=self.mode, replicas=self.replicas),
Review comment:
I meant why not just
```python
def __init__(
self,
...,
mode: ServiceMode = types.ServiceMode(mode="replicated", replicas=1),
...,
) -> None:
```
instead.
--
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] enima2684 commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r685174687
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -121,12 +153,16 @@ def _run_service(self) -> None:
env=self.environment,
user=self.user,
tty=self.tty,
+ configs=self.configs,
+ secrets=self.secrets,
),
restart_policy=types.RestartPolicy(condition='none'),
resources=types.Resources(mem_limit=self.mem_limit),
+ networks=self.networks,
),
name=f'airflow-{get_random_string()}',
labels={'name': f'airflow__{self.dag_id}__{self.task_id}'},
+ mode=types.ServiceMode(mode=self.mode, replicas=self.replicas),
Review comment:
👍 You got a point. Will modify it then
--
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] enima2684 commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r684980438
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -121,12 +153,16 @@ def _run_service(self) -> None:
env=self.environment,
user=self.user,
tty=self.tty,
+ configs=self.configs,
+ secrets=self.secrets,
),
restart_policy=types.RestartPolicy(condition='none'),
resources=types.Resources(mem_limit=self.mem_limit),
+ networks=self.networks,
),
name=f'airflow-{get_random_string()}',
labels={'name': f'airflow__{self.dag_id}__{self.task_id}'},
+ mode=types.ServiceMode(mode=self.mode, replicas=self.replicas),
Review comment:
Thanks @uranusjr for your review.
I did not get your remark. Can you please elaborate ?
In all cases, I followed the signature of the function `create_service` as defined on the docker sdk for python :
https://docker-py.readthedocs.io/en/stable/api.html#module-docker.api.service
--
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] akki commented on pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
akki commented on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-903072838
Great. Congrats @enima2684 & welcome to the committers clan! 😉
--
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] uranusjr commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r684965907
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -121,12 +153,16 @@ def _run_service(self) -> None:
env=self.environment,
user=self.user,
tty=self.tty,
+ configs=self.configs,
+ secrets=self.secrets,
),
restart_policy=types.RestartPolicy(condition='none'),
resources=types.Resources(mem_limit=self.mem_limit),
+ networks=self.networks,
),
name=f'airflow-{get_random_string()}',
labels={'name': f'airflow__{self.dag_id}__{self.task_id}'},
+ mode=types.ServiceMode(mode=self.mode, replicas=self.replicas),
Review comment:
Any reason why this isn’t set by passing in a `ServiceMode` instance but two separate flags?
--
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] akki commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
akki commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r689036685
##########
File path: tests/providers/docker/operators/test_docker_swarm.py
##########
@@ -66,22 +65,31 @@ def _client_service_logs_effect():
mem_limit='128m',
user='unittest',
task_id='unittest',
- mounts=[Mount(source='/host/path', target='/container/path', type='bind')],
+ mounts=[types.Mount(source='/host/path', target='/container/path', type='bind')],
auto_remove=True,
tty=True,
+ configs=[types.ConfigReference(config_id="dummy_cfg_id", config_name="dummy_cfg_name")],
+ secrets=[types.SecretReference(secret_id="dummy_secret_id", secret_name="dummy_secret_name")],
+ mode=types.ServiceMode(mode="replicated", replicas=3),
+ networks=["dummy_network"],
)
operator.execute(None)
types_mock.TaskTemplate.assert_called_once_with(
- container_spec=mock_obj, restart_policy=mock_obj, resources=mock_obj
+ container_spec=mock_obj,
+ restart_policy=mock_obj,
+ resources=mock_obj,
+ networks=["dummy_network"],
)
types_mock.ContainerSpec.assert_called_once_with(
image='ubuntu:latest',
command='env',
user='unittest',
- mounts=[Mount(source='/host/path', target='/container/path', type='bind')],
+ mounts=[types.Mount(source='/host/path', target='/container/path', type='bind')],
tty=True,
env={'UNIT': 'TEST', 'AIRFLOW_TMP_DIR': '/tmp/airflow'},
+ configs=[types.ConfigReference(config_id="dummy_cfg_id", config_name="dummy_cfg_name")],
+ secrets=[types.SecretReference(secret_id="dummy_secret_id", secret_name="dummy_secret_name")],
Review comment:
Can we also add an assertion for the `mode` config?
Something like `assert cskwargs['mode'] == types.ServiceMode(mode="replicated", replicas=3)` below the assertions for `name` & `labels`.
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -93,13 +93,40 @@ class DockerSwarmOperator(DockerOperator):
Supported only if the Docker engine is using json-file or journald logging drivers.
The `tty` parameter should be set to use this with Python applications.
:type enable_logging: bool
+ :param configs: List of docker configs to be exposed to the containers of the swarm service.
+ The configs are ConfigReference objects as per the docker api
+ [https://docker-py.readthedocs.io/en/stable/services.html#docker.models.services.ServiceCollection.create]_
+ :type configs: List[docker.types.ConfigReference]
+ :param secrets: List of docker secrets to be exposed to the containers of the swarm service.
+ The secrets are SecretReference objects as per the docker create_service api.
+ [https://docker-py.readthedocs.io/en/stable/services.html#docker.models.services.ServiceCollection.create]_
+ :type secrets: List[docker.types.SecretReference]
+ :param mode: Indicate whether a service should be deployed as a replicated or global service,
+ and associated parameters
+ :type mode: docker.types.ServiceMode
+ :param networks: List of network names or IDs or NetworkAttachmentConfig to attach the service to.
+ :type networks: List[Union[str, NetworkAttachmentConfig]]
"""
- def __init__(self, *, image: str, enable_logging: bool = True, **kwargs) -> None:
+ def __init__(
+ self,
+ *,
+ image: str,
+ enable_logging: bool = True,
+ configs: Optional[List[types.ConfigReference]] = None,
Review comment:
So users would need to import `docker` in their code to use this feature, right?
I was thinking it might be worth adding the usage example in [example_docker_swarm.py](https://github.com/apache/airflow/blob/b10ed95a2aded01eb5580120ab2abbde1bac633b/airflow/providers/docker/example_dags/example_docker_swarm.py) to make it easy for (new) users to adopt this feature.
--
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] enima2684 commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r684992326
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -121,12 +153,16 @@ def _run_service(self) -> None:
env=self.environment,
user=self.user,
tty=self.tty,
+ configs=self.configs,
+ secrets=self.secrets,
),
restart_policy=types.RestartPolicy(condition='none'),
resources=types.Resources(mem_limit=self.mem_limit),
+ networks=self.networks,
),
name=f'airflow-{get_random_string()}',
labels={'name': f'airflow__{self.dag_id}__{self.task_id}'},
+ mode=types.ServiceMode(mode=self.mode, replicas=self.replicas),
Review comment:
Yes possible.
I just wanted to avoid as much as possible exposing to airflow users docker objects.
--
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] boring-cyborg[bot] commented on pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-901818119
Awesome work, congrats on your first merged pull request!
--
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] boring-cyborg[bot] commented on pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-894389306
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] enima2684 commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r689570502
##########
File path: tests/providers/docker/operators/test_docker_swarm.py
##########
@@ -66,22 +65,31 @@ def _client_service_logs_effect():
mem_limit='128m',
user='unittest',
task_id='unittest',
- mounts=[Mount(source='/host/path', target='/container/path', type='bind')],
+ mounts=[types.Mount(source='/host/path', target='/container/path', type='bind')],
auto_remove=True,
tty=True,
+ configs=[types.ConfigReference(config_id="dummy_cfg_id", config_name="dummy_cfg_name")],
+ secrets=[types.SecretReference(secret_id="dummy_secret_id", secret_name="dummy_secret_name")],
+ mode=types.ServiceMode(mode="replicated", replicas=3),
+ networks=["dummy_network"],
)
operator.execute(None)
types_mock.TaskTemplate.assert_called_once_with(
- container_spec=mock_obj, restart_policy=mock_obj, resources=mock_obj
+ container_spec=mock_obj,
+ restart_policy=mock_obj,
+ resources=mock_obj,
+ networks=["dummy_network"],
)
types_mock.ContainerSpec.assert_called_once_with(
image='ubuntu:latest',
command='env',
user='unittest',
- mounts=[Mount(source='/host/path', target='/container/path', type='bind')],
+ mounts=[types.Mount(source='/host/path', target='/container/path', type='bind')],
tty=True,
env={'UNIT': 'TEST', 'AIRFLOW_TMP_DIR': '/tmp/airflow'},
+ configs=[types.ConfigReference(config_id="dummy_cfg_id", config_name="dummy_cfg_name")],
+ secrets=[types.SecretReference(secret_id="dummy_secret_id", secret_name="dummy_secret_name")],
Review comment:
Thanks for that ! I struggled initially to do it but thanks to your comment, I was able to do it finally :)
It is done !
--
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 #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-895850274
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.
--
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] uranusjr commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r684965401
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -93,13 +94,44 @@ class DockerSwarmOperator(DockerOperator):
Supported only if the Docker engine is using json-file or journald logging drivers.
The `tty` parameter should be set to use this with Python applications.
:type enable_logging: bool
+ :param configs: List of docker configs to be exposed to the containers of the swarm service.
+ The configs are ConfigReference objects as per the docker api
+ [https://docker-py.readthedocs.io/en/stable/services.html#docker.models.services.ServiceCollection.create]_
+ :type configs: list[ConfigReference]
+ :param secrets: List of docker secrets to be exposed to the containers of the swarm service.
+ The secrets are SecretReference objects as per the docker create_service api.
+ [https://docker-py.readthedocs.io/en/stable/services.html#docker.models.services.ServiceCollection.create]_
+ :type secrets: list[SecretReference]
+ :param mode: Indicate whether a service should be deployed as a replicated or global service.
+ Can be either `replicated` or `global`
+ :type mode: str
+ :param replicas: Number of replicas. For replicated services only.
+ :type replicas: int
+ :param networks: List of network names or IDs or NetworkAttachmentConfig to attach the service to.
+ :type networks: List[Union[str, NetworkAttachmentConfig]]
"""
- def __init__(self, *, image: str, enable_logging: bool = True, **kwargs) -> None:
+ def __init__(
+ self,
+ *,
+ image: str,
+ enable_logging: bool = True,
+ configs: List[ConfigReference] = None,
+ secrets: List[SecretReference] = None,
+ mode: str = "replicated",
+ replicas: int = 1,
+ networks: List[Union[str, NetworkAttachmentConfig]] = None,
Review comment:
```suggestion
configs: Optional[List[ConfigReference]] = None,
secrets: Optional[List[SecretReference]] = None,
mode: str = "replicated",
replicas: int = 1,
networks: Optional[List[Union[str, NetworkAttachmentConfig]]] = None,
```
--
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] ashb merged pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #17474:
URL: https://github.com/apache/airflow/pull/17474
--
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] uranusjr edited a comment on pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-895841252
We now have
* `from docker import types`
* `from docker.types import ...`
* `from docker.types.services import ...`
Around the same proximity. They should be somewhat unified. I think `from docker import types` can be used to access everything?
--
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] uranusjr commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r685170203
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -121,12 +153,16 @@ def _run_service(self) -> None:
env=self.environment,
user=self.user,
tty=self.tty,
+ configs=self.configs,
+ secrets=self.secrets,
),
restart_policy=types.RestartPolicy(condition='none'),
resources=types.Resources(mem_limit=self.mem_limit),
+ networks=self.networks,
),
name=f'airflow-{get_random_string()}',
labels={'name': f'airflow__{self.dag_id}__{self.task_id}'},
+ mode=types.ServiceMode(mode=self.mode, replicas=self.replicas),
Review comment:
Yeah that’s a good policy, and I would have recommended the same normally. But we are already exposing `ConfigReference` and `SecretReference` and tie-coupling to Docker.py, so it’s not very meaningful to not expose it anymore. If we want to hide it, we need to wrap *all* the configurations.
--
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] enima2684 commented on pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-901202119
Hi @akki,
Thanks for your review and comments :)
I think we have covered them all and needed fixes are done.
Can you please approve the PR if all ok for you ?
Many thanks
--
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] enima2684 commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r689567708
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -93,13 +93,40 @@ class DockerSwarmOperator(DockerOperator):
Supported only if the Docker engine is using json-file or journald logging drivers.
The `tty` parameter should be set to use this with Python applications.
:type enable_logging: bool
+ :param configs: List of docker configs to be exposed to the containers of the swarm service.
+ The configs are ConfigReference objects as per the docker api
+ [https://docker-py.readthedocs.io/en/stable/services.html#docker.models.services.ServiceCollection.create]_
+ :type configs: List[docker.types.ConfigReference]
+ :param secrets: List of docker secrets to be exposed to the containers of the swarm service.
+ The secrets are SecretReference objects as per the docker create_service api.
+ [https://docker-py.readthedocs.io/en/stable/services.html#docker.models.services.ServiceCollection.create]_
+ :type secrets: List[docker.types.SecretReference]
+ :param mode: Indicate whether a service should be deployed as a replicated or global service,
+ and associated parameters
+ :type mode: docker.types.ServiceMode
+ :param networks: List of network names or IDs or NetworkAttachmentConfig to attach the service to.
+ :type networks: List[Union[str, NetworkAttachmentConfig]]
"""
- def __init__(self, *, image: str, enable_logging: bool = True, **kwargs) -> None:
+ def __init__(
+ self,
+ *,
+ image: str,
+ enable_logging: bool = True,
+ configs: Optional[List[types.ConfigReference]] = None,
Review comment:
Hi @akki, thanks a lot for your review :)
I tought of doing it but struggled on this because the secrets, configs or networks should be created ahead on the swarm cluster.
Adding them on the example will result on having to preconfigure the cluster with secrets, configs and networks before being able to run it.
In order to keep the examples running standalone, I prefered not to add it.
Another option would be to create a config, secret and network with the docker sdk on the example. But this does not have much to do with airflow.
What do you think ?
--
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] uranusjr commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r685170203
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -121,12 +153,16 @@ def _run_service(self) -> None:
env=self.environment,
user=self.user,
tty=self.tty,
+ configs=self.configs,
+ secrets=self.secrets,
),
restart_policy=types.RestartPolicy(condition='none'),
resources=types.Resources(mem_limit=self.mem_limit),
+ networks=self.networks,
),
name=f'airflow-{get_random_string()}',
labels={'name': f'airflow__{self.dag_id}__{self.task_id}'},
+ mode=types.ServiceMode(mode=self.mode, replicas=self.replicas),
Review comment:
Yeah that’s a good policy, and I would have recommended the same normally. But we are already exposing `ConfigReference` and `SecretReference` and tight-coupling to Docker.py, so it’s not very meaningful to not expose it anymore. If we want to hide it, we need to wrap *all* the configurations.
--
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] enima2684 commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r685786839
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -121,12 +153,16 @@ def _run_service(self) -> None:
env=self.environment,
user=self.user,
tty=self.tty,
+ configs=self.configs,
+ secrets=self.secrets,
),
restart_policy=types.RestartPolicy(condition='none'),
resources=types.Resources(mem_limit=self.mem_limit),
+ networks=self.networks,
),
name=f'airflow-{get_random_string()}',
labels={'name': f'airflow__{self.dag_id}__{self.task_id}'},
+ mode=types.ServiceMode(mode=self.mode, replicas=self.replicas),
Review comment:
@uranusjr Changes done as requested. Can you please review / approve ?
--
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] enima2684 commented on pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
enima2684 commented on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-901202119
Hi @akki,
Thanks for your review and comments :)
I think we have covered them all and needed fixes are done.
Can you please approve the PR if all ok for you ?
Many thanks
--
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] akki commented on a change in pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
akki commented on a change in pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#discussion_r689656767
##########
File path: airflow/providers/docker/operators/docker_swarm.py
##########
@@ -93,13 +93,40 @@ class DockerSwarmOperator(DockerOperator):
Supported only if the Docker engine is using json-file or journald logging drivers.
The `tty` parameter should be set to use this with Python applications.
:type enable_logging: bool
+ :param configs: List of docker configs to be exposed to the containers of the swarm service.
+ The configs are ConfigReference objects as per the docker api
+ [https://docker-py.readthedocs.io/en/stable/services.html#docker.models.services.ServiceCollection.create]_
+ :type configs: List[docker.types.ConfigReference]
+ :param secrets: List of docker secrets to be exposed to the containers of the swarm service.
+ The secrets are SecretReference objects as per the docker create_service api.
+ [https://docker-py.readthedocs.io/en/stable/services.html#docker.models.services.ServiceCollection.create]_
+ :type secrets: List[docker.types.SecretReference]
+ :param mode: Indicate whether a service should be deployed as a replicated or global service,
+ and associated parameters
+ :type mode: docker.types.ServiceMode
+ :param networks: List of network names or IDs or NetworkAttachmentConfig to attach the service to.
+ :type networks: List[Union[str, NetworkAttachmentConfig]]
"""
- def __init__(self, *, image: str, enable_logging: bool = True, **kwargs) -> None:
+ def __init__(
+ self,
+ *,
+ image: str,
+ enable_logging: bool = True,
+ configs: Optional[List[types.ConfigReference]] = None,
Review comment:
Good point. Maybe better to keep it simple & leave it as is then.
I guess examples are the most useful to beginners, so adding too much complexity (like Docker sdk code) might make it confusing for them.
--
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] uranusjr commented on pull request #17474: Add support for configs, secrets, networks and replicas for DockerSwarmOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17474:
URL: https://github.com/apache/airflow/pull/17474#issuecomment-895841252
We now have
* `from docker import types`
* `from docker.types import ...`
* `from docker.types.services import ...`
Around the same proximation. They should be somewhat unified. I think `from docker import types` can be used to access everything?
--
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