From befdb41995ca39a94203413ee2ab5f516e5cf56b Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 20 Sep 2023 11:01:09 +0200 Subject: [PATCH 1/2] libpod: remove unused ContainerState() fucntion First that function claims to deep copy but then actually return the original state so it does not work correctly, given that there are no users just remove it instead of fixing it. Signed-off-by: Paul Holzinger --- libpod/container.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/libpod/container.go b/libpod/container.go index d1f0c983c3..966b9e2cc7 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -1220,23 +1220,6 @@ func (c *Container) HostNetwork() bool { return true } -// ContainerState returns containerstate struct -func (c *Container) ContainerState() (*ContainerState, error) { - if !c.batched { - c.lock.Lock() - defer c.lock.Unlock() - - if err := c.syncContainer(); err != nil { - return nil, err - } - } - returnConfig := new(ContainerState) - if err := JSONDeepCopy(c.state, returnConfig); err != nil { - return nil, fmt.Errorf("copying container %s state: %w", c.ID(), err) - } - return c.state, nil -} - // HasHealthCheck returns bool as to whether there is a health check // defined for the container func (c *Container) HasHealthCheck() bool { From 8e5adde0b3f60cec75904458d36ed79be83aea38 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 20 Sep 2023 11:37:04 +0200 Subject: [PATCH 2/2] compat API: speed up network list The network list compat API requires us to include all containers with their ip addresses for the selected networks. Because we have no network -> container mapping in the db we have to go through all containers every time. However the old code did it in the most ineffective way possible, it quered the containers from the db for each individual network. The of course is extremely expensive. Now the other expensive call is calling Inspect() on the container each time. Inspect does for more than we need. To fix this we fist query containers only once for the API call, then replace the inspect call with directly accessing the network status. This will speed things up a lot! The reported scenario includes 100 containers and 25 networks, previously it took 1.5s for the API call not it takes 24ms, that is a more than a 62x improvement. (tested with curl) [NO NEW TESTS NEEDED] We have no timing tests. Fixes #20035 Signed-off-by: Paul Holzinger --- libpod/container.go | 15 +++++ pkg/api/handlers/compat/networks.go | 90 +++++++++++++++++++---------- 2 files changed, 74 insertions(+), 31 deletions(-) diff --git a/libpod/container.go b/libpod/container.go index 966b9e2cc7..f13ea55b4a 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -1339,6 +1339,21 @@ func (d ContainerNetworkDescriptions) getInterfaceByName(networkName string) (st return fmt.Sprintf("eth%d", val), exists } +// GetNetworkStatus returns the current network status for this container. +// This returns a map without deep copying which means this should only ever +// be used as read only access, do not modify this status. +func (c *Container) GetNetworkStatus() (map[string]types.StatusBlock, error) { + if !c.batched { + c.lock.Lock() + defer c.lock.Unlock() + + if err := c.syncContainer(); err != nil { + return nil, err + } + } + return c.getNetworkStatus(), nil +} + // getNetworkStatus get the current network status from the state. If the container // still uses the old network status it is converted to the new format. This function // should be used instead of reading c.state.NetworkStatus directly. diff --git a/pkg/api/handlers/compat/networks.go b/pkg/api/handlers/compat/networks.go index 67d15ad204..22451adf3a 100644 --- a/pkg/api/handlers/compat/networks.go +++ b/pkg/api/handlers/compat/networks.go @@ -22,6 +22,36 @@ import ( "github.com/sirupsen/logrus" ) +type containerNetStatus struct { + name string + id string + status map[string]nettypes.StatusBlock +} + +func getContainerNetStatuses(rt *libpod.Runtime) ([]containerNetStatus, error) { + cons, err := rt.GetAllContainers() + if err != nil { + return nil, err + } + statuses := make([]containerNetStatus, 0, len(cons)) + for _, con := range cons { + status, err := con.GetNetworkStatus() + if err != nil { + if errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved) { + continue + } + return nil, err + } + + statuses = append(statuses, containerNetStatus{ + id: con.ID(), + name: con.Name(), + status: status, + }) + } + return statuses, nil +} + func normalizeNetworkName(rt *libpod.Runtime, name string) (string, bool) { if name == nettypes.BridgeNetworkDriver { return rt.Network().DefaultNetworkName(), true @@ -56,45 +86,42 @@ func InspectNetwork(w http.ResponseWriter, r *http.Request) { utils.NetworkNotFound(w, name, err) return } - report, err := convertLibpodNetworktoDockerNetwork(runtime, &net, changed) + statuses, err := getContainerNetStatuses(runtime) if err != nil { utils.InternalServerError(w, err) return } + report := convertLibpodNetworktoDockerNetwork(runtime, statuses, &net, changed) utils.WriteResponse(w, http.StatusOK, report) } -func convertLibpodNetworktoDockerNetwork(runtime *libpod.Runtime, network *nettypes.Network, changeDefaultName bool) (*types.NetworkResource, error) { - cons, err := runtime.GetAllContainers() - if err != nil { - return nil, err - } - containerEndpoints := make(map[string]types.EndpointResource, len(cons)) - for _, con := range cons { - data, err := con.Inspect(false) - if err != nil { - if errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved) { - continue - } - return nil, err - } - if netData, ok := data.NetworkSettings.Networks[network.Name]; ok { +func convertLibpodNetworktoDockerNetwork(runtime *libpod.Runtime, statuses []containerNetStatus, network *nettypes.Network, changeDefaultName bool) *types.NetworkResource { + containerEndpoints := make(map[string]types.EndpointResource, len(statuses)) + for _, st := range statuses { + if netData, ok := st.status[network.Name]; ok { ipv4Address := "" - if netData.IPAddress != "" { - ipv4Address = fmt.Sprintf("%s/%d", netData.IPAddress, netData.IPPrefixLen) - } ipv6Address := "" - if netData.GlobalIPv6Address != "" { - ipv6Address = fmt.Sprintf("%s/%d", netData.GlobalIPv6Address, netData.GlobalIPv6PrefixLen) + macAddr := "" + for _, dev := range netData.Interfaces { + for _, subnet := range dev.Subnets { + // Note the docker API really wants the full CIDR subnet not just a single ip. + // https://github.com/containers/podman/pull/12328 + if netutil.IsIPv4(subnet.IPNet.IP) { + ipv4Address = subnet.IPNet.String() + } else { + ipv6Address = subnet.IPNet.String() + } + } + macAddr = dev.MacAddress.String() + break } containerEndpoint := types.EndpointResource{ - Name: con.Name(), - EndpointID: netData.EndpointID, - MacAddress: netData.MacAddress, + Name: st.name, + MacAddress: macAddr, IPv4Address: ipv4Address, IPv6Address: ipv6Address, } - containerEndpoints[con.ID()] = containerEndpoint + containerEndpoints[st.id] = containerEndpoint } } ipamConfigs := make([]dockerNetwork.IPAMConfig, 0, len(network.Subnets)) @@ -144,7 +171,7 @@ func convertLibpodNetworktoDockerNetwork(runtime *libpod.Runtime, network *netty Peers: nil, Services: nil, } - return &report, nil + return &report } func ListNetworks(w http.ResponseWriter, r *http.Request) { @@ -165,13 +192,14 @@ func ListNetworks(w http.ResponseWriter, r *http.Request) { utils.InternalServerError(w, err) return } + statuses, err := getContainerNetStatuses(runtime) + if err != nil { + utils.InternalServerError(w, err) + return + } reports := make([]*types.NetworkResource, 0, len(nets)) for _, net := range nets { - report, err := convertLibpodNetworktoDockerNetwork(runtime, &net, true) - if err != nil { - utils.InternalServerError(w, err) - return - } + report := convertLibpodNetworktoDockerNetwork(runtime, statuses, &net, true) reports = append(reports, report) } utils.WriteResponse(w, http.StatusOK, reports)