pass networks to container clone #14059

cdoern · 2022-04-29T02:40:45Z

since the network config is a string map, json.unmarshal does not recognize
the config and spec as the same entity, need to map this option manually

resolves #13713

Signed-off-by: cdoern [email protected]

Luap99

This is not the problem. The json marshal/unmarshal will handle it just fine.

Podman stores the network information inside an extra db bucket and not the container config. I guess container clone must read from the there and not the settings in the container config.

cdoern · 2022-04-29T13:21:11Z

ah I see @Luap99 I will correct my comment but this fixes the issue, right?

[charliedoern@charliesthinkpad podman]$ bin/podman run --network=tl --name test alpine ip a
Resolved "alpine" as an alias (/etc/containers/registries.conf.d/000-shortnames.conf)
Trying to pull docker.io/library/alpine:latest...
Getting image source signatures
Copying blob df9b9388f04a done  
Copying config 0ac33e5f5a done  
Writing manifest to image destination
Storing signatures
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
2: eth0@if4: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether 9e:4f:37:68:91:da brd ff:ff:ff:ff:ff:ff
    inet 10.89.0.2/24 brd 10.89.0.255 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::9c4f:37ff:fe68:91da/64 scope link tentative 
       valid_lft forever preferred_lft forever
[charliedoern@charliesthinkpad podman]$ bin/podman container clone test
7ac29969bb80d3a42d7b4f5156f05f79aa16de818c7bbe23cdf1c576ecf067fc
[charliedoern@charliesthinkpad podman]$ bin/podman start --attach test-clone
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
2: eth0@if4: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether c2:10:a1:d3:db:32 brd ff:ff:ff:ff:ff:ff
    inet 10.89.0.3/24 brd 10.89.0.255 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::c010:a1ff:fed3:db32/64 scope link tentative 
       valid_lft forever preferred_lft forever

Luap99 · 2022-04-29T13:29:00Z

Try with network connect/disconnect. Networks must always be read from the db, see c.networks().

cdoern · 2022-04-29T13:34:04Z

Try with network connect/disconnect. Networks must always be read from the db, see c.networks().

yes it works with connect/disconnect.


[charliedoern@charliesthinkpad podman]$ podman network connect tl test-clone
Error: container 7ac29969bb80d3a42d7b4f5156f05f79aa16de818c7bbe23cdf1c576ecf067fc is already connected to network "tl": network already exists
[charliedoern@charliesthinkpad podman]$ podman network disconnect tl test-clone
[charliedoern@charliesthinkpad podman]$ podman network connect tl test-clone
[charliedoern@charliesthinkpad podman]$ podman start --attach test-clone
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
2: eth0@if4: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether aa:97:42:87:71:f9 brd ff:ff:ff:ff:ff:ff
    inet 10.89.0.5/24 brd 10.89.0.255 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::a897:42ff:fe87:71f9/64 scope link tentative 
       valid_lft forever preferred_lft forever

Luap99 · 2022-04-29T13:35:27Z

Also if you connect a second network?

Luap99 · 2022-04-29T13:36:07Z

You have to connect before you clone

cdoern · 2022-04-29T13:38:11Z

@Luap99 I will add some tests for these cases

Luap99 · 2022-04-29T13:42:07Z

I just tried on main branch and it looks like it already works without this change. I assume something else merged in the meantime which fixed this problem

cdoern · 2022-04-29T13:44:36Z

@Luap99 I dont see how that could be possible, specg.Networks in ConfigToSpec was always empty! let me check one more time.

Luap99 · 2022-04-29T13:47:58Z

nvm I tested the wrong container, but it does not work with this patch either.
config.Networks are always nil because we unset it since it has to be read from the db directly.

cdoern · 2022-04-29T13:50:42Z

I will keep working on this and add some more tests. This breaks running a cloned ctr in a pod too which makes me think this solution works and we were just never passing the network options. I will look at the original --network path and see what I am missing

Luap99 · 2022-04-29T14:09:19Z

Ok I see the problem I thought I unset the networks but the logic does this to late after the config is written:

podman/libpod/boltdb_state_internal.go

Lines 584 to 586 in 73836e0

    
           // Set the original value to nil. We can safe some space by not storing it in the config 
        
           // since we store it in a different mutable bucket anyway. 
        
           ctr.config.Networks = nil

So you end up with networks in the config, however no caller should ever use this, always use c.networks()

Your patch is correct because Networks from the config has a different json tag (newNetworks) due backport compat.

So the only thing missing is do get the updated network list from the db. I guess it would make sense to attach this in c.Config() since outside caller should not worry about this detail IMO.

cdoern · 2022-04-29T15:21:50Z

@Luap99 can I just get the whole config again from the db in Config() and deep copy that?

Luap99 · 2022-04-29T15:25:35Z

I was thinking something like this:

diff --git a/libpod/container.go b/libpod/container.go
index 3e7ab7b0a..7afa78dc8 100644
--- a/libpod/container.go
+++ b/libpod/container.go
@@ -288,6 +288,12 @@ func (c *Container) Config() *ContainerConfig {
                return nil
        }
 
+       networks, err := c.networks()
+       if err != nil {
+               return nil
+       }
+       returnConfig.Networks = networks
+
        return returnConfig
 }

pkg/domain/infra/abi/containers.go

umohnani8 · 2022-05-02T13:40:24Z

LGTM

rhatdan · 2022-05-02T14:08:04Z

@Luap99 PTAL

test/e2e/container_clone_test.go

pkg/specgen/generate/container_create.go

libpod/container.go

since the network config is a string map, json.unmarshal does not recognize the config and spec as the same entity, need to map this option manually resolves containers#13713 Signed-off-by: cdoern <[email protected]>

Luap99

LGTM, one nit but not worth to repush

Luap99 · 2022-05-05T09:49:16Z

test/e2e/container_clone_test.go

+		_, ok := inspect.InspectContainerToJSON()[0].NetworkSettings.Networks["testing123"]
+		Expect(ok).To(BeTrue())


nit please use HaveKeyValue() for such tests.

If this check fails you currently would get a very unhelpful expected false to be true message.

Luap99 · 2022-05-05T09:49:55Z

/approve
/lgtm

openshift-ci · 2022-05-05T09:50:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 reviewed Apr 29, 2022

View reviewed changes

cdoern force-pushed the clone branch 3 times, most recently from 01b0f83 to a36a4ac Compare April 29, 2022 22:22

Luap99 reviewed Apr 30, 2022

View reviewed changes

pkg/domain/infra/abi/containers.go Outdated Show resolved Hide resolved

pkg/domain/infra/abi/containers.go Outdated Show resolved Hide resolved

cdoern force-pushed the clone branch from a36a4ac to 0a1cc52 Compare May 2, 2022 00:21

Luap99 reviewed May 2, 2022

View reviewed changes

test/e2e/container_clone_test.go Outdated Show resolved Hide resolved

pkg/specgen/generate/container_create.go Show resolved Hide resolved

libpod/container.go Show resolved Hide resolved

pass networks to container clone

1585b17

since the network config is a string map, json.unmarshal does not recognize the config and spec as the same entity, need to map this option manually resolves containers#13713 Signed-off-by: cdoern <[email protected]>

cdoern force-pushed the clone branch from 0a1cc52 to 1585b17 Compare May 4, 2022 03:04

Luap99 reviewed May 5, 2022

View reviewed changes

openshift-ci bot assigned Luap99 May 5, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2022

openshift-merge-robot merged commit 7af4612 into containers:main May 5, 2022

Luap99 mentioned this pull request May 6, 2022

libpod: add c.ConfigWithNetworks() #14136

Merged

This was referenced May 9, 2022

Aardvark Test 5: Two containers on their own subnets, one container on both #14171

Closed

CI flake: secret file is not leaked into image #13417

Closed

edsantiago mentioned this pull request Jun 2, 2022

Timeout in podman-save multi-image #14468

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pass networks to container clone #14059

pass networks to container clone #14059

cdoern commented Apr 29, 2022

Luap99 left a comment

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022 •

edited

Loading

Luap99 commented Apr 29, 2022

umohnani8 commented May 2, 2022

rhatdan commented May 2, 2022

Luap99 left a comment

Luap99 May 5, 2022

Luap99 commented May 5, 2022

openshift-ci bot commented May 5, 2022

		_, ok := inspect.InspectContainerToJSON()[0].NetworkSettings.Networks["testing123"]
		Expect(ok).To(BeTrue())

pass networks to container clone #14059

pass networks to container clone #14059

Conversation

cdoern commented Apr 29, 2022

Luap99 left a comment

Choose a reason for hiding this comment

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022

Luap99 commented Apr 29, 2022

cdoern commented Apr 29, 2022 • edited Loading

Luap99 commented Apr 29, 2022

umohnani8 commented May 2, 2022

rhatdan commented May 2, 2022

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 May 5, 2022

Choose a reason for hiding this comment

Luap99 commented May 5, 2022

openshift-ci bot commented May 5, 2022

cdoern commented Apr 29, 2022 •

edited

Loading