Skip to content
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

make docker operators always use DockerHook for API calls #28363

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

Taragolis
Copy link
Contributor

Right now DockerHook only partially uses in DockerOperator and DockerSwarmOperator.

This PR refactor some parts

  1. Drop raising errors for most of the missing arguments during constructing hook. In Docker SDK all of them optional.
  2. Move out all expensive call from __init__, e.g. obtain connection.
  3. docker_conn_id now optional. If user not required login to docker Registry this field might be set to None.
    Previously this functional sits in DockerOperator
  4. Cache connection to docker server/host.
  5. Move out constructing TLSConfig from DockerOperator to DockerHook

This kind of integration will allow in the future move all complex logic from Operators to Hook and make them reusable.

Also this PR pre-requirements for #26162

@Taragolis Taragolis changed the title make docker operators use DockerHook for API calls make docker operators always use DockerHook for API calls Dec 14, 2022
@Taragolis
Copy link
Contributor Author

BTW, may be someone know why docker-provider has min airflow version 2.4?

@Taragolis Taragolis force-pushed the docker-hook-refactor branch 2 times, most recently from b3408be to 5d1401f Compare December 27, 2022 10:24
@eladkal
Copy link
Contributor

eladkal commented Dec 27, 2022

BTW, may be someone know why docker-provider has min airflow version 2.4?

Because of ExternalPythonOperator #25780

@Taragolis
Copy link
Contributor Author

BTW, may be someone know why docker-provider has min airflow version 2.4?

Because of ExternalPythonOperator #25780

I can't find any information why it should affect docker provider but maybe I blind

Also we have this lines, which make sense if we support 2.3+ and look redundant if 2.4+

try:
from airflow.utils.decorators import remove_task_decorator
# This can be removed after we move to Airflow 2.4+
except ImportError:
from airflow.utils.python_virtualenv import remove_task_decorator

@potiuk
Copy link
Member

potiuk commented Dec 30, 2022

I can't find any information why it should affect docker provider but maybe I blind

It looks like it was a mistake. We should get it down to 2.3.0 in the next release:

We already have a code added in the same PR that handles pre-2.4 case - https://github.com/apache/airflow/pull/25780/files#diff-1042a900e2f4c7b044453f7229033b8787d35abe66b14d0e9392fee8b1a714ffR30

So >=2.3.0 should be fine.

@potiuk
Copy link
Member

potiuk commented Dec 30, 2022

I think it was just failing with 2.3 and I first bumped it to 2.4 and then added the if and forgot to move it down to 2.3.0

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Very nice refactor.

However I have one request - could we also handle tls parameter In Hook possible to be passed as string for backwards compatibility? Seems that it should be very easy to implement and not that unreasonable even (Hook will be then a bit more flexible on what cold be passed to tls. And we will avoid having to bump major version of the provider? Seems like all other changes are backwards compatible.

@Taragolis
Copy link
Contributor Author

However I have one request - could we also handle tls parameter In Hook possible to be passed as string for backwards compatibility?

@potiuk, I will have a look again (today last day of my holidays) however based on docker-py code pass tls parameter as string no make sense at least in docker>=5.0.3 (our requirements). It will interpreters as bool(tls) if it not an instance of TLSConfig

Internally we do not check type of this argument (if not count mypy), so if user pass string and this somehow work it will work.

@potiuk
Copy link
Member

potiuk commented Jan 3, 2023

tls parameter as string no make sense at least in docker>=5.0.3 (our requirements)

Ah. Then it makes perfect sense - it's really a bugfix not a backwards-compatibility probiem then.

@potiuk potiuk merged commit 57a889d into apache:main Jan 3, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 3, 2023
Those tests are running various tests with Providers Manager and
could catch errors introduced by provider-only changes so they
should be added to "Always" tests to be "always" run in our CI.

Skipping this test was the main reason why apache#28700 change was needed after
the apache#28363 was merged.
jedcunningham pushed a commit that referenced this pull request Jan 3, 2023
Those tests are running various tests with Providers Manager and
could catch errors introduced by provider-only changes so they
should be added to "Always" tests to be "always" run in our CI.

Skipping this test was the main reason why #28700 change was needed after
the #28363 was merged.
@Taragolis
Copy link
Contributor Author

Ah. Then it makes perfect sense - it's really a bugfix not a backwards-compatibility probiem then.

maybe somewhere in the past tls_config could be a string (I didn't check) but the main issue that Hook previously do not use in DockerOperator and others and even when it use then tls_config constructed from multiple separate parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants