Skip to content

Don't return err on disconnectNetwork in docker executor

Lyubomir Raykov requested to merge stop-disconnect-network-err into master

What does this MR do?

Remove the returning of err in disconnectNetwork. It wasn't used in the callers.

Why was this MR needed?

The convention for executor Cleanup methods is not to return an error, but just to log. This happens already in this method so there's no point in returning an error, given it's not handled in the callers. It is used either before docker kill or docker rm -f. They both disconnect the container network so it's not an issue even if this explicit call fails (more details below).

Are there points in the code the reviewer needs to double check?

The assumption I'm going with is that the explicit network disconnect is just nice-to-have and not a necessary operation to clean up resources. It was done for #1642 (closed) which was caused by a couple of now-resolved Docker issues - https://github.com/moby/moby/issues/22772 and https://github.com/moby/moby/issues/20398

Small validation that docker rm and docker kill disconnect network:

# docker kill
~  >> docker run --network="test-network" -d alpine:latest tail -f
8a8e3b8692a83f967ac87e62b0723e56423033aff127cedbb7c5d352fb61b2c2
 ~  >> t0=$(date "+%Y-%m-%dT%H:%M:%S")
 ~  >> docker kill 8a8e3b8692a83f96
8a8e3b8692a83f96
 ~  >> t1=$(date "+%Y-%m-%dT%H:%M:%S")
 ~  >> docker events --since $t0 --until $t1
2020-03-10T11:17:59.749897900+02:00 container kill 8a8e3b8692a83f967ac87e62b0723e56423033aff127cedbb7c5d352fb61b2c2 (image=alpine:latest, name=musing_lederberg, signal=9)
2020-03-10T11:17:59.825943900+02:00 container die 8a8e3b8692a83f967ac87e62b0723e56423033aff127cedbb7c5d352fb61b2c2 (exitCode=137, image=alpine:latest, name=musing_lederberg)
2020-03-10T11:17:59.897851500+02:00 network disconnect b702da57a337579c43cf1b87ffd3dbf7b1d744c206ed7951a7d78429af3de889 (container=8a8e3b8692a83f967ac87e62b0723e56423033aff127cedbb7c5d352fb61b2c2, name=test-network, type=bridge)

and

# docker rm
~  >> docker run --network="test-network" -d alpine:latest tail -f
dbafeaf197419e2a378e6c9a20368d95746157afd1f5f111d18ac132e53262cc
 ~  >> t0=$(date "+%Y-%m-%dT%H:%M:%S")
 ~  >> docker container rm -f dbafeaf1974
dbafeaf1974
 ~  >> t1=$(date "+%Y-%m-%dT%H:%M:%S")
 ~  >> docker events --since $t0 --until $t1
2020-03-10T11:19:00.909927800+02:00 container kill dbafeaf197419e2a378e6c9a20368d95746157afd1f5f111d18ac132e53262cc (image=alpine:latest, name=vigorous_thompson, signal=9)
2020-03-10T11:19:00.983076700+02:00 container die dbafeaf197419e2a378e6c9a20368d95746157afd1f5f111d18ac132e53262cc (exitCode=137, image=alpine:latest, name=vigorous_thompson)
2020-03-10T11:19:01.049962800+02:00 network disconnect b702da57a337579c43cf1b87ffd3dbf7b1d744c206ed7951a7d78429af3de889 (container=dbafeaf197419e2a378e6c9a20368d95746157afd1f5f111d18ac132e53262cc, name=test-network, type=bridge)
2020-03-10T11:19:01.076226700+02:00 container destroy dbafeaf197419e2a378e6c9a20368d95746157afd1f5f111d18ac132e53262cc (image=alpine:latest, name=vigorous_thompson)

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Added tests for this feature/bug
  • In case of conflicts with master - branch was rebased

What are the relevant issue numbers?

Fixes #8322 (closed)

Merge request reports