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

feat: support container network mode #1429

Merged
merged 10 commits into from
Aug 8, 2023

Conversation

schizo99
Copy link
Contributor

@schizo99 schizo99 commented Oct 17, 2022

This PR solves the issue when using network_mode: service:xxxxx in a service.
It closes #1286

For this to work not only does the label com.centurylinklabs.watchtower.depends-on be set to the parent container. It also require a new label com.centurylinklabs.watchtower.network-service to be set.

It solves the issue by updating the HostConfig with the ID of the new parent container.

Update: Since NetworkMode: "container:*" would mean that the container would not work without being re-created if the network supplying container was updated, this should no longer be necessary. Instead it is now treated as an implicit link.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congratulations on opening your first pull request! We'll get back to you as soon as possible. In the meantime, please make sure you've updated the documentation to reflect your changes and have added test automation as needed. Thanks! 🙏🏼

@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Patch coverage: 83.33% and project coverage change: +0.21% 🎉

Comparison is base (dfe4346) 67.47% compared to head (fdd0406) 67.69%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1429      +/-   ##
==========================================
+ Coverage   67.47%   67.69%   +0.21%     
==========================================
  Files          26       26              
  Lines        2346     2377      +31     
==========================================
+ Hits         1583     1609      +26     
- Misses        666      670       +4     
- Partials       97       98       +1     
Files Changed Coverage Δ
pkg/container/container.go 52.55% <25.00%> (-0.58%) ⬇️
pkg/container/client.go 49.47% <100.00%> (+3.27%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@schizo99
Copy link
Contributor Author

I think I have found a better way to solve this.

Will update PR later on.

@schizo99
Copy link
Contributor Author

Maybe did a mistake by force pushing?

In any case I think this new approach is nicer and does not require any additional labels.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

This seems like it should work. The only thing I am not thrilled about is that the API for getting the container info also alters the returned info. The way it's used right now, this shouldn't be an issue, but it's hidden/unexpected, which could lead to bugs further on.

Of course, the "right" way would be to do this when preparing the new configuration before starting the container. Except that at that time, the old container should already have been removed.

pkg/container/client.go Outdated Show resolved Hide resolved
pkg/container/client.go Outdated Show resolved Hide resolved
@schizo99
Copy link
Contributor Author

You're right. I'm not 100% sold on this but it's the most efficient way.
It might, as you say, cause issues should the Docker API change from allowing name or id in the network_mode.

The other option would be my first approach which would require an additional label to be added besides the depends_on label.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

It still needs tests, but otherwise I am happy with this. Changing the container:ContainerID to container:ContainerName is in a way more correct, even if it's not what the raw inspect contained.

@schizo99
Copy link
Contributor Author

Was intending to add tests.

However, as soon as I update the container_watchtower.json and change network_mode to container:xxxxx the server throws this error.

Go test thrown errors
(cloud) {15:37}~/fab/watchtower/pkg/container:networkmode-service ✗ ➭ go test -v
=== RUN   TestContainer
Running Suite: Container Suite
==============================
Random Seed: 1666618638
Will run 44 of 44 specs

•••••WARN[0000] Failed to retrieve container image info: request returned Internal Server Error for API route and version http://127.0.0.1:33371/v1.41/images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json, check if the server supports the requested API version 

------------------------------
• Failure [0.003 seconds]
the client
/home/aahp/fab/watchtower/pkg/container/client_test.go:23
  when listing containers
  /home/aahp/fab/watchtower/pkg/container/client_test.go:72
    when no filter is provided
    /home/aahp/fab/watchtower/pkg/container/client_test.go:73
      should return all available containers [It]
      /home/aahp/fab/watchtower/pkg/container/client_test.go:74

      Path mismatch
      Expected
          <string>: /v1.41/containers/xxxxxxxxxxxxxxxxxxxx/json
      to have suffix
          <string>: /images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json

      /home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51
------------------------------
WARN[0000] Failed to retrieve container image info: request returned Internal Server Error for API route and version http://127.0.0.1:38345/v1.41/images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json, check if the server supports the requested API version 
• Failure [0.001 seconds]
the client
/home/aahp/fab/watchtower/pkg/container/client_test.go:23
  when listing containers
  /home/aahp/fab/watchtower/pkg/container/client_test.go:72
    when a filter matching nothing
    /home/aahp/fab/watchtower/pkg/container/client_test.go:86
      should return an empty array [It]
      /home/aahp/fab/watchtower/pkg/container/client_test.go:87

      Path mismatch
      Expected
          <string>: /v1.41/containers/xxxxxxxxxxxxxxxxxxxx/json
      to have suffix
          <string>: /images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json

      /home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51
------------------------------
WARN[0000] Failed to retrieve container image info: request returned Internal Server Error for API route and version http://127.0.0.1:35389/v1.41/images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json, check if the server supports the requested API version 
• Failure [0.002 seconds]
the client
/home/aahp/fab/watchtower/pkg/container/client_test.go:23
  when listing containers
  /home/aahp/fab/watchtower/pkg/container/client_test.go:72
    when a watchtower filter is provided
    /home/aahp/fab/watchtower/pkg/container/client_test.go:100
      should return only the watchtower container [It]
      /home/aahp/fab/watchtower/pkg/container/client_test.go:101

      Path mismatch
      Expected
          <string>: /v1.41/containers/xxxxxxxxxxxxxxxxxxxx/json
      to have suffix
          <string>: /images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json

      /home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51
------------------------------
WARN[0000] Failed to retrieve container image info: request returned Internal Server Error for API route and version http://127.0.0.1:42385/v1.41/images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json, check if the server supports the requested API version 
• Failure [0.002 seconds]
the client
/home/aahp/fab/watchtower/pkg/container/client_test.go:23
  when listing containers
  /home/aahp/fab/watchtower/pkg/container/client_test.go:72
    when include stopped is enabled
    /home/aahp/fab/watchtower/pkg/container/client_test.go:113
      should return both stopped and running containers [It]
      /home/aahp/fab/watchtower/pkg/container/client_test.go:114

      Path mismatch
      Expected
          <string>: /v1.41/containers/xxxxxxxxxxxxxxxxxxxx/json
      to have suffix
          <string>: /images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json

      /home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51
------------------------------
WARN[0000] Failed to retrieve container image info: request returned Internal Server Error for API route and version http://127.0.0.1:45507/v1.41/images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json, check if the server supports the requested API version 
• Failure [0.001 seconds]
the client
/home/aahp/fab/watchtower/pkg/container/client_test.go:23
  when listing containers
  /home/aahp/fab/watchtower/pkg/container/client_test.go:72
    when include restarting is enabled
    /home/aahp/fab/watchtower/pkg/container/client_test.go:126
      should return both restarting and running containers [It]
      /home/aahp/fab/watchtower/pkg/container/client_test.go:127

      Path mismatch
      Expected
          <string>: /v1.41/containers/xxxxxxxxxxxxxxxxxxxx/json
      to have suffix
          <string>: /images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json

      /home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51
------------------------------
WARN[0000] Failed to retrieve container image info: request returned Internal Server Error for API route and version http://127.0.0.1:39927/v1.41/images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json, check if the server supports the requested API version 
• Failure [0.001 seconds]
the client
/home/aahp/fab/watchtower/pkg/container/client_test.go:23
  when listing containers
  /home/aahp/fab/watchtower/pkg/container/client_test.go:72
    when include restarting is disabled
    /home/aahp/fab/watchtower/pkg/container/client_test.go:139
      should not return restarting containers [It]
      /home/aahp/fab/watchtower/pkg/container/client_test.go:140

      Path mismatch
      Expected
          <string>: /v1.41/containers/xxxxxxxxxxxxxxxxxxxx/json
      to have suffix
          <string>: /images/sha256:4dbc5f9c07028a985e14d1393e849ea07f68804c4293050d5a641b138db72daa/json

      /home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51
------------------------------
• Failure [1.002 seconds]
the client
/home/aahp/fab/watchtower/pkg/container/client_test.go:23
  ExecuteCommand
  /home/aahp/fab/watchtower/pkg/container/client_test.go:153
    when logging
    /home/aahp/fab/watchtower/pkg/container/client_test.go:154
      should include container id field [It]
      /home/aahp/fab/watchtower/pkg/container/client_test.go:155

      Timed out after 1.001s.
      Got stuck at:
          ERRO[0000] Failed to extract command exec logs: cannot connect to the Docker daemon. Is 'docker daemon' running on this host?: dial http: unknown network http  containerID=ex-cont-id
          
      Waiting for:
          containerID="?ex-cont-id"?

      /home/aahp/fab/watchtower/pkg/container/client_test.go:218
------------------------------
••••••••••••••••••••••••••••••••

Summarizing 7 Failures:

[Fail] the client when listing containers when no filter is provided [It] should return all available containers 
/home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51

[Fail] the client when listing containers when a filter matching nothing [It] should return an empty array 
/home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51

[Fail] the client when listing containers when a watchtower filter is provided [It] should return only the watchtower container 
/home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51

[Fail] the client when listing containers when include stopped is enabled [It] should return both stopped and running containers 
/home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51

[Fail] the client when listing containers when include restarting is enabled [It] should return both restarting and running containers 
/home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51

[Fail] the client when listing containers when include restarting is disabled [It] should not return restarting containers 
/home/aahp/go/pkg/mod/github.com/onsi/gomega@v1.21.1/ghttp/handlers.go:51

[Fail] the client ExecuteCommand when logging [It] should include container id field 
/home/aahp/fab/watchtower/pkg/container/client_test.go:218

Ran 44 of 44 Specs in 1.013 seconds
FAIL! -- 37 Passed | 7 Failed | 0 Pending | 0 Skipped
--- FAIL: TestContainer (1.02s)
FAIL
exit status 1
FAIL	github.com/containrrr/watchtower/pkg/container	1.028s

And I don't have the slightest as to why this is not working even if the container is pointing to itself.

@hicksoft
Copy link

Any updates on this PR? Updating containers with service network mode is painful right now. Looking forward to seeing this get merged.

@piksel
Copy link
Member

piksel commented Mar 16, 2023

@s-wasser it still needs tests, and I don't have enough knowledge of how docker network configuration changes the container info to fix it easily. I can take a look next time I get some free time though...

@shivindera
Copy link

Any updates on this getting merged?

@piksel
Copy link
Member

piksel commented Apr 23, 2023

@shivindera still needs tests. I have no idea if this actually works or not. The tests need to be there for us to be able to maintain this project. They tell us what some code is expected to do and how we can incorporate other changes without breaking this one's. I don't personally use this functionality myself, so it both requires extra time on reading up on and understanding the use cases, but it also means it would be guesses from my part in the end.

@heisenberg2980
Copy link

@schizo99 any chance of getting this PR tested and merged? it seems you guys were really close last year, what is missing here to fix the issue?

@schizo99
Copy link
Contributor Author

@heisenberg2980 What's missing are tests for the new code. But I'm getting errors when implementing the tests.
See my previous comment

@heisenberg2980
Copy link

I would love to help but I have never implemented this type of tests myself. @piksel can you help with the errors @schizo99 is getting when trying to implement the test?

@piksel
Copy link
Member

piksel commented Jul 24, 2023

@schizo99 if you can push the tests with the errors, I can probably fix them!

Also, I got a VPN subscription and created some integration test cases that use gluetun as the container network. It still needs credentials of course, but if you have a service provider you should be able to test watchtower with it. I can add it to the tools in this PR.

@schizo99
Copy link
Contributor Author

schizo99 commented Jul 24, 2023

@piksel I never got so far as to create any test cases for these changes since it started failing as soon as I modified the NetworkMode for the container in container_watchtower.json.

So all you need to do for the tests to start failing is to update the file and set NetworkMode as this "NetworkMode": "container:watchtower-running"

Might very well be me not understanding the mock framework.

@piksel
Copy link
Member

piksel commented Jul 25, 2023

To make the tests easier to write, I did some refactoring on the docker API server mock.
I ended up just writing a single test, and the only thing that is left un-tested is when the referenced container cannot be found.
I did at least add the integration test and the corresponding docker-compose.yml and could confirm that it was working with this fix.
Adding an additional test for when the container isn't found should be fairly trivial, but I don't have any more time today.

@heisenberg2980
Copy link

Thank you very much @piksel, we are really close to have this PR merged and the issue fixed

@heisenberg2980
Copy link

@piksel will you be able to add the additional test to get this PR merged?

@piksel piksel changed the title Networkmode service feat: support container network mode Aug 8, 2023
@piksel piksel merged commit dca45f5 into containrrr:main Aug 8, 2023
11 checks passed
@heisenberg2980
Copy link

heisenberg2980 commented Aug 9, 2023

@piksel do you know when a new version of watchtower containing this change will be released so we can test it?

@exil0867
Copy link

exil0867 commented Aug 9, 2023

@piksel do you know when a new version of watchtower containing this change will be released so we can test it?

Use latest-dev image tag.

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

Successfully merging this pull request may close these issues.

Watchtower Fails Recreating Containers that Share a Network Stack
7 participants