[AIRFLOW-3203] Fix DockerOperator & some operator test

[XD-DENG]

Jira

• https://issues.apache.org/jira/browse/AIRFLOW-3203

Description

• For argument image, no need to explicitly add "latest" if tag is omitted.
  "latest" will be used by default if no tag provided. This is handled by docker package itself.

• Intermediate variable cpu_shares is not needed.

• Fix out-dated usage of cpu_shares and cpu_shares.
  Based on https://docker-py.readthedocs.io/en/stable/api.html#docker.api.container.ContainerApiMixin.create_host_config,
  They should be an arguments of self.cli.create_host_config() rather than APIClient.create_container().

• Change name of the corresponding test script, to ensure it can be discovered.

• Fix the test itself.

• This PR also fixed part of the test scripts of Operators. They were named incorrectly which result in test discovery failure.

[codecov-io]

Codecov Report

Merging #4049 into master will increase coverage by 1.77%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4049      +/-   ##
==========================================
+ Coverage   75.91%   77.69%   +1.77%     
==========================================
  Files         199      199              
  Lines       15948    15944       -4     
==========================================
+ Hits        12107    12387     +280     
+ Misses       3841     3557     -284

Impacted Files	Coverage Δ
airflow/operators/docker_operator.py	97.61% <100%> (+97.61%)	⬆️
airflow/jobs.py	82.48% <0%> (+0.35%)	⬆️
airflow/hooks/hive_hooks.py	73.42% <0%> (+0.52%)	⬆️
airflow/operators/hive_operator.py	86.53% <0%> (+5.76%)	⬆️
airflow/utils/file.py	84% <0%> (+8%)	⬆️
airflow/operators/python_operator.py	95.03% <0%> (+13.04%)	⬆️
airflow/operators/subdag_operator.py	90.32% <0%> (+19.35%)	⬆️
airflow/operators/latest_only_operator.py	90% <0%> (+65%)	⬆️
airflow/operators/s3_to_hive_operator.py	94.01% <0%> (+94.01%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 719e0b1...111a803. Read the comment docs.

[XD-DENG]

Hi @Fokko,

As we discussed in the email list yesterday, some scripts in /test should be renamed to ensure they can be discovered by the CI process.

This PR only addresses issues for DockerOperator though. There are some issues in both the DockerOperator itself & its test (including name of the test script).

For other operators whose test were missed due to file name, I suspect there are accumulated issues to fix as well (it may not be as easy as just prepending test_ to the test file name). I may take a look into them later when I get time.

Cheers

[XD-DENG]

Hi @Fokko ,

I have changed the name of some of the Operator test scripts (prepend with "test_"), after making sure they don't raise any exception and work as designed, including

• tests/operators/docker_operator.py (with some code change)
• tests/operators/hive_operator.py
• tests/operators/latest_only_operator.py
• tests/operators/python_operator.py
• tests/operators/s3_to_hive_operator.py
• tests/operators/slack_operator.py
• tests/operators/subdag_operator.py

There are another two,

• tests/operators/bash_operator.py
• tests/operators/operator.py
  needing more works. I may fix them later (if I get time & nobody else picks them up).

[XD-DENG]

In addition, I would suggest to include this commit into 1.10.1 which is intended to fix bugs. Re-enabling these tests should be useful to ensure less potential bugs.

[ashb]

Why were these remove? (I can't work out from the diff alone.

[XD-DENG]

Hi @ashb , this is because I found out that this will be handled by docker package itself.

Here this operator is using docker.APIClient.create_container(image=...) to create the container under the hood. If the image is just the name withOUT tag, say "fake_image", then the program will go search for "fake_image:latest" by default (of course it will search for the tag given by user if user has specified the tag).

You can try the code below to validate:

import docker
client = docker.APIClient(base_url='unix://var/run/docker.sock')
client.create_container(image="fake_image")

It will tell you something like "'fake_image:latest' can't be found" (I don't have Docker on the laptop I'm using at this moment so can't provide the exact exception). Please help double-validate.

Cheers

[XD-DENG]

Hi @ashb , I have checked on a machine with Docker installed.

Without Tag

import docker
client = docker.APIClient(base_url='unix://var/run/docker.sock')
client.create_container(image="fake_image")

Result:

ImageNotFound: 404 Client Error: Not Found ("No such image: fake_image:latest")

With Tag

import docker
client = docker.APIClient(base_url='unix://var/run/docker.sock')
client.create_container(image="fake_image:version_1")

Result:

ImageNotFound: 404 Client Error: Not Found ("No such image: fake_image:version_1")

Conclusion

So as I shared earlier: if tag is omitted for image, "latest" will be used by default; if tag is provided by user, then that tag will be used.

So here in DockerOperator, we don't need to explicitly check & add ":latest".

[feluelle]

Hey @XD-DENG were you really certain about that? Because I noticed something weird when I use for example just r-base as an image name.

The DockerOperator task pulled every image.

r-base                                                     latest              62c848eeb175        2 weeks ago         649MB
r-base                                                     3.5.3               62c848eeb175        2 weeks ago         649MB
r-base                                                     3.5.2               880eb7da671b        5 weeks ago         655MB
r-base                                                     3.5.1               bd9edc1a85ed        4 months ago        712MB
r-base                                                     3.5.0               190658892827        9 months ago        692MB
r-base                                                     3.4.4               d1325eaa28ad        11 months ago       667MB
r-base                                                     3.4.3               d1e1c25485af        13 months ago       670MB
r-base                                                     3.4.2               02d3b7e00020        17 months ago       651MB
r-base                                                     3.4.1               0ab131e275e4        19 months ago       1.18GB
r-base                                                     3.4.0               5a6c58403310        22 months ago       656MB
r-base                                                     3.3.3               88436550cddc        2 years ago         635MB
r-base                                                     3.3.2               38270dc4745b        2 years ago         635MB
r-base                                                     3.3.1               7ba1baf9d8bb        2 years ago         657MB
r-base                                                     3.3.0               8f62a54a58ab        2 years ago         970MB
r-base                                                     3.2.5               ee4ea743f431        2 years ago         1.05GB
r-base                                                     3.2.4               9d9c50e41475        3 years ago         1.07GB
r-base                                                     3.2.3               1844ae3ec8d8        3 years ago         1.03GB
r-base                                                     3.2.2               7201dfdf7e21        3 years ago         1.01GB
r-base                                                     3.2.1               62e43f6f9a91        3 years ago         563MB
r-base                                                     3.2.0               9adf6ba7ea84        3 years ago         552MB
r-base                                                     3.1.3               8b37253aa5ea        4 years ago         517MB
r-base                                                     3.1.2               803d5e0e0278        4 years ago         483MB

pip list shows me docker 3.7.2 in airflow 1.10.2

[XD-DENG]

@feluelle let me have a quick check and get back here.

[XD-DENG]

@feluelle may you share your log? You may have logs like Pulling image (xxx) from xxx. Thanks

[XD-DENG]

Hi @feluelle the change was made based on the facts below:

1. For pull: https://docker-py.readthedocs.io/en/stable/api.html#docker.api.image.ImageApiMixin.pull In the example, you can find that the latest tag is used if no tag is provided together with the repo.

2. For create_container: you can try to code below

import docker
client = docker.APIClient(base_url='unix://var/run/docker.sock')
client.create_container(image="fake_image")

It will give you error "ImageNotFound: 404 Client Error: Not Found ("No such image: fake_image:latest")"

[feluelle]

Sure. https://pastebin.com/raw/NUmS0tYh

[feluelle]

(That's not all) I think it is for like 3 images

[XD-DENG]

@ashb @Fokko , may you take another look?

Cheers!

[XD-DENG]

Hi @Fokko , a gentle ping. Cheers!

[Fokko]

Thanks for picking this up and sorting out the Docker stuff @XD-DENG. Highly appreciated.

I'm fine with moving this into 1.10.1. I don't see any API's being changed. What do you think @ashb

[Fokko]

@ashb Cherry-picked it onto the 1.10.1 branch

[XD-DENG]

Thanks @Fokko