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.

Sign up for GitHub

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

Vendor c/common pasta branch for testing #21563

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Vendor c/common pasta branch for testing #21563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c1c4c0
Vendor latest c/common and fix tests
mheon Feb 8, 2024
03f6589
Fix events by fully adding the new PullError event
mheon Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ require (
github.com/checkpoint-restore/go-criu/v7 v7.0.0
github.com/containernetworking/plugins v1.4.0
github.com/containers/buildah v1.34.1-0.20240201124221-b850c711ff5c
github.com/containers/common v0.57.1-0.20240229151045-1c65e0de241a
github.com/containers/common v0.57.1-0.20240229165734-cec09922602e
github.com/containers/conmon v2.0.20+incompatible
github.com/containers/gvisor-tap-vsock v0.7.3
github.com/containers/image/v5 v5.29.3-0.20240227090231-5bef5e1e1506
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ github.com/containernetworking/plugins v1.4.0 h1:+w22VPYgk7nQHw7KT92lsRmuToHvb7w
github.com/containernetworking/plugins v1.4.0/go.mod h1:UYhcOyjefnrQvKvmmyEKsUA+M9Nfn7tqULPpH0Pkcj0=
github.com/containers/buildah v1.34.1-0.20240201124221-b850c711ff5c h1:r+1vFyTAoXptJrsPsnOMI3G0jm4+BCfXAcIyuA33lzo=
github.com/containers/buildah v1.34.1-0.20240201124221-b850c711ff5c/go.mod h1:Hw4qo2URFpWvZ2tjLstoQMpNC6+gR4PtxQefvV/UKaA=
github.com/containers/common v0.57.1-0.20240229151045-1c65e0de241a h1:N/AOv7bQBTD/+inld7Qpc2WzxtpbsPgPDB/gg7JGQKA=
github.com/containers/common v0.57.1-0.20240229151045-1c65e0de241a/go.mod h1:8irlyBcVooYx0F+YmoY7PQPAIgdJvCj17bvL7PqeaxI=
github.com/containers/common v0.57.1-0.20240229165734-cec09922602e h1:TPgCd6bWFyliJxCXEiCI1LnbB3kBUkpx1dw51ngDjWI=
github.com/containers/common v0.57.1-0.20240229165734-cec09922602e/go.mod h1:8irlyBcVooYx0F+YmoY7PQPAIgdJvCj17bvL7PqeaxI=
github.com/containers/conmon v2.0.20+incompatible h1:YbCVSFSCqFjjVwHTPINGdMX1F6JXHGTUje2ZYobNrkg=
github.com/containers/conmon v2.0.20+incompatible/go.mod h1:hgwZ2mtuDrppv78a/cOBNiCm6O0UMWGx1mu7P00nu5I=
github.com/containers/gvisor-tap-vsock v0.7.3 h1:yORnf15sP+sLFhxLNLgmB5/lOhldn9dRMHx/tmYtSOQ=
Expand Down
4 changes: 4 additions & 0 deletions libpod/events/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ type Event struct {
Type Type
// Health status of the current container
HealthStatus string `json:"health_status,omitempty"`
// Error code for certain events involving errors.
Error error `json:"error,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This cannot work with the file logger as you cannot unmarshal into an error interface so it must be changed to a string. And in general I am really not a fan of merging such changes (in an unrelated PR) without any tests nor chance for actual review.
I know we just wanted pasta to get in but that could have just gone it normal way without causing all the extra troubles by simply not merging unverified/untested changes in c/common without thinking how they effect podman.

Also the whole thing is not plumbed in at all, you never set this option from the libimage type so the error is lost and also not plugged into podman the cli nor API which means it will not show any error messages, users only see pull-error $IMAGE right now.

I am aware that your goal was to just get test passing but I write this because this "feature" if far from done and needs tests.


Details
}
Expand Down Expand Up @@ -170,6 +172,8 @@ const (
Prune Status = "prune"
// Pull ...
Pull Status = "pull"
// PullError is an error pulling an image
PullError Status = "pull-error"
// Push ...
Push Status = "push"
// Refresh indicates that the system refreshed the state after a
Expand Down
2 changes: 2 additions & 0 deletions libpod/events/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,8 @@ func StringToStatus(name string) (Status, error) {
return Prune, nil
case Pull.String():
return Pull, nil
case PullError.String():
return PullError, nil
case Push.String():
return Push, nil
case Refresh.String():
Expand Down
6 changes: 6 additions & 0 deletions libpod/events/journal_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ func (e EventJournalD) Write(ee Event) error {
case Image:
m["PODMAN_NAME"] = ee.Name
m["PODMAN_ID"] = ee.ID
if ee.Error != nil {
m["ERROR"] = ee.Error.Error()
}
case Container, Pod:
m["PODMAN_IMAGE"] = ee.Image
m["PODMAN_NAME"] = ee.Name
Expand Down Expand Up @@ -228,6 +231,9 @@ func newEventFromJournalEntry(entry *sdjournal.JournalEntry) (*Event, error) {
newEvent.Network = entry.Fields["PODMAN_NETWORK_NAME"]
case Image:
newEvent.ID = entry.Fields["PODMAN_ID"]
if _, ok := entry.Fields["ERROR"]; ok {
newEvent.Error = errors.New(entry.Fields["ERROR"])
}
}
return &newEvent, nil
}
Expand Down
60 changes: 25 additions & 35 deletions libpod/networking_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ func getContainerNetNS(ctr *Container) (string, *Container, error) {
func getContainerNetIO(ctr *Container) (map[string]define.ContainerNetworkStats, error) {
perNetworkStats := make(map[string]define.ContainerNetworkStats)

netNSPath, otherCtr, netPathErr := getContainerNetNS(ctr)
netNSPath, _, netPathErr := getContainerNetNS(ctr)
if netPathErr != nil {
return nil, netPathErr
}
Expand All @@ -201,49 +201,39 @@ func getContainerNetIO(ctr *Container) (map[string]define.ContainerNetworkStats,
return nil, nil
}

netMode := ctr.config.NetMode
netStatus := ctr.getNetworkStatus()
if otherCtr != nil {
netMode = otherCtr.config.NetMode
netStatus = otherCtr.getNetworkStatus()
}
if netMode.IsSlirp4netns() {
// create a fake status with correct interface name for the logic below
netStatus = map[string]types.StatusBlock{
"slirp4netns": {
Interfaces: map[string]types.NetInterface{"tap0": {}},
},
}
}
err := ns.WithNetNSPath(netNSPath, func(_ ns.NetNS) error {
for _, status := range netStatus {
for dev := range status.Interfaces {
link, err := netlink.LinkByName(dev)
if err != nil {
return err
}
stats := link.Attrs().Statistics
if stats != nil {
newStats := define.ContainerNetworkStats{
RxBytes: stats.RxBytes,
RxDropped: stats.RxDropped,
RxErrors: stats.RxErrors,
RxPackets: stats.RxPackets,
TxBytes: stats.TxBytes,
TxDropped: stats.TxDropped,
TxErrors: stats.TxErrors,
TxPackets: stats.TxPackets,
}
links, err := netlink.LinkList()
if err != nil {
return fmt.Errorf("retrieving all network interfaces: %w", err)
}
for _, link := range links {
attributes := link.Attrs()
if attributes.Flags&net.FlagLoopback != 0 {
continue
}

perNetworkStats[dev] = newStats
}
if attributes.Statistics != nil {
perNetworkStats[attributes.Name] = getNetStatsFromNetlinkStats(attributes.Statistics)
}
}
return nil
})
return perNetworkStats, err
}

func getNetStatsFromNetlinkStats(stats *netlink.LinkStatistics) define.ContainerNetworkStats {
return define.ContainerNetworkStats{
RxBytes: stats.RxBytes,
RxDropped: stats.RxDropped,
RxErrors: stats.RxErrors,
RxPackets: stats.RxPackets,
TxBytes: stats.TxBytes,
TxDropped: stats.TxDropped,
TxErrors: stats.TxErrors,
TxPackets: stats.TxPackets,
}
}

// joinedNetworkNSPath returns netns path and bool if netns was set
func (c *Container) joinedNetworkNSPath() (string, bool) {
for _, namespace := range c.config.Spec.Linux.Namespaces {
Expand Down
19 changes: 10 additions & 9 deletions libpod/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,15 +696,16 @@ func (r *Runtime) GetConfig() (*config.Config, error) {

// libimageEventsMap translates a libimage event type to a libpod event status.
var libimageEventsMap = map[libimage.EventType]events.Status{
libimage.EventTypeImagePull: events.Pull,
libimage.EventTypeImagePush: events.Push,
libimage.EventTypeImageRemove: events.Remove,
libimage.EventTypeImageLoad: events.LoadFromArchive,
libimage.EventTypeImageSave: events.Save,
libimage.EventTypeImageTag: events.Tag,
libimage.EventTypeImageUntag: events.Untag,
libimage.EventTypeImageMount: events.Mount,
libimage.EventTypeImageUnmount: events.Unmount,
libimage.EventTypeImagePull: events.Pull,
libimage.EventTypeImagePullError: events.PullError,
libimage.EventTypeImagePush: events.Push,
libimage.EventTypeImageRemove: events.Remove,
libimage.EventTypeImageLoad: events.LoadFromArchive,
libimage.EventTypeImageSave: events.Save,
libimage.EventTypeImageTag: events.Tag,
libimage.EventTypeImageUntag: events.Untag,
libimage.EventTypeImageMount: events.Mount,
libimage.EventTypeImageUnmount: events.Unmount,
}

// libimageEvents spawns a goroutine which will listen for events on
Expand Down
2 changes: 1 addition & 1 deletion test/apiv2/20-containers.at
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ t GET libpod/containers/json?all=true 200 \
.[0].IsInfra=false

# Test compat API for Network Settings (.Network is N/A when rootless)
network_expect="Networks.slirp4netns.NetworkID=slirp4netns"
network_expect="Networks.pasta.NetworkID=pasta"
if root; then
network_expect="Networks.podman.NetworkID=podman"
fi
Expand Down
17 changes: 0 additions & 17 deletions test/e2e/unshare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,6 @@ import (
. "github.com/onsi/gomega/gexec"
)

// podman unshare --rootless-netns leaks the process by design.
// Running a container will cause the cleanup to kick in when this container gets stopped.
func cleanupRootlessSlirp4netns(p *PodmanTestIntegration) {
session := p.Podman([]string{"run", "--network", "bridge", ALPINE, "true"})
session.WaitWithDefaultTimeout()
Expect(session).Should(ExitCleanly())
}

var _ = Describe("Podman unshare", func() {
BeforeEach(func() {
if _, err := os.Stat("/proc/self/uid_map"); err != nil {
Expand All @@ -37,15 +29,6 @@ var _ = Describe("Podman unshare", func() {
Expect(session.OutputToString()).ToNot(ContainSubstring(userNS))
})

It("podman unshare --rootless-netns", func() {
SkipIfRemote("podman-remote unshare is not supported")
defer cleanupRootlessSlirp4netns(podmanTest)
session := podmanTest.Podman([]string{"unshare", "--rootless-netns", "ip", "addr"})
session.WaitWithDefaultTimeout()
Expect(session).Should(ExitCleanly())
Expect(session.OutputToString()).To(ContainSubstring("tap0"))
})

It("podman unshare exit codes", func() {
SkipIfRemote("podman-remote unshare is not supported")
session := podmanTest.Podman([]string{"unshare", "false"})
Expand Down
7 changes: 5 additions & 2 deletions test/system/250-systemd.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

load helpers
load helpers.systemd
load helpers.network

SERVICE_NAME="podman_test_$(random_string)"

Expand Down Expand Up @@ -294,7 +295,7 @@ LISTEN_FDNAMES=listen_fdnames" | sort)
}

# https://github.com/containers/podman/issues/13153
@test "podman rootless-netns slirp4netns process should be in different cgroup" {
@test "podman rootless-netns pasta processes should be in different cgroup" {
edsantiago marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I looked and looked for some way to do this:

    run_podman info --format "what is the current rootless network provider"
    skip if it's anything other than pasta

You know, for futureproofing. I could not find any marker. Is there one?

Copy link
Member

Choose a reason for hiding this comment

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

Actually the test should happen for slirp and pasta. We do not have a marker it is just some containers.conf setting but looks we never actually explicitly test that here. I don't want to block this PR on this though. I can submit a PR tomorrow to address this and the other comments once this is merged.

is_rootless || skip "only meaningful for rootless"

cname=$(random_string)
Expand All @@ -314,9 +315,11 @@ LISTEN_FDNAMES=listen_fdnames" | sort)
# stop systemd container
service_cleanup

pasta_iface=$(default_ifname)

# now check that the rootless netns slirp4netns process is still alive and working
run_podman unshare --rootless-netns ip addr
is "$output" ".*tap0.*" "slirp4netns interface exists in the netns"
is "$output" ".*$pasta_iface.*" "pasta interface exists in the netns"
run_podman exec $cname2 nslookup google.com

run_podman rm -f -t0 $cname2
Expand Down
3 changes: 3 additions & 0 deletions test/system/252-quadlet.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,16 @@ function remove_secret() {
}

@test "quadlet - basic" {
# Network=none is to work around a Pasta bug, can be removed once a patched Pasta is available.
# Ref https://github.com/containers/podman/pull/21563#issuecomment-1965145324
Comment on lines +147 to +148
Copy link
Member

Choose a reason for hiding this comment

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

This is the sort of comment where I like to see "FIXME" or "TODO", because I have a script that looks for those.

I also like to see explicit comments with version strings, or "> 2024-03", or some sort of date or version string offering a hint about when it would be good to check. Someone reading this in the year 2050 might wonder what "patched Pasta" means.

local quadlet_file=$PODMAN_TMPDIR/basic_$(random_string).container
cat > $quadlet_file <<EOF
[Container]
Image=$IMAGE
Exec=sh -c "echo STARTED CONTAINER; echo "READY=1" | socat -u STDIN unix-sendto:\$NOTIFY_SOCKET; sleep inf"
Notify=yes
LogDriver=passthrough
Network=none
EOF

# FIXME: Temporary until podman fully removes cgroupsv1 support; see #21431
Expand Down
9 changes: 9 additions & 0 deletions test/system/270-socket-activation.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,17 @@
#

load helpers
load helpers.registry
load helpers.systemd

function setup_file() {
# We have to stop the background registry here. These tests kill the podman pause
# process which means commands after that are in a new one and when the cleanup
# later tries to stop the registry container it will be in the wrong ns and can fail.
# https://github.com/containers/podman/pull/21563#issuecomment-1960047648
stop_registry
}

SERVICE_NAME="podman_test_$(random_string)"

SERVICE_SOCK_ADDR="/run/podman/$SERVICE_NAME.sock"
Expand Down
2 changes: 1 addition & 1 deletion test/system/500-networking.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ EOF
cid=${output}
run_podman inspect --format '{{ .NetworkSettings.Networks }}' $cid
if is_rootless; then
is "$output" "map\[slirp4netns:.*" "NeworkSettings should contain one network named slirp4netns"
is "$output" "map\[pasta:.*" "NeworkSettings should contain one network named pasta"
else
is "$output" "map\[podman:.*" "NeworkSettings should contain one network named podman"
fi
Expand Down
26 changes: 11 additions & 15 deletions test/system/505-networking-pasta.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,6 @@ function setup() {
XFER_FILE="${PODMAN_TMPDIR}/pasta.bin"
}

function default_ifname() {
local ip_ver="${1}"

local expr='[.[] | select(.dst == "default").dev] | .[0]'
ip -j -"${ip_ver}" route show | jq -rM "${expr}"
}

function default_addr() {
local ip_ver="${1}"
local ifname="${2:-$(default_ifname "${ip_ver}")}"

local expr='.[0] | .addr_info[0].local'
ip -j -"${ip_ver}" addr show "${ifname}" | jq -rM "${expr}"
}

# _set_opt() - meta-helper for pasta_test_do.
#
# Sets an option, but panics if option is already set (e.g. UDP+TCP, IPv4/v6)
Expand Down Expand Up @@ -789,3 +774,14 @@ EOF
CONTAINERS_CONF_OVERRIDE=$containersconf run_podman run --net=pasta:--ns-mac-addr,"$mac2" $IMAGE ip link show myname
assert "$output" =~ "$mac2" "mac address from cli is set on custom interface"
}

### Rootless unshare testins

@test "Podman unshare --rootless-netns with Pasta" {
skip_if_remote "unshare is local-only"

pasta_iface=$(default_ifname)
Copy link
Member

Choose a reason for hiding this comment

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

Again, I really dislike the use of pasta in the variable name (and test name) because one day there will be a different default. I would much prefer rootless and/or an explicit skip if this is not pasta.

Copy link
Collaborator

Choose a reason for hiding this comment

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

one day there will be a different default.

Never. It's pasta e basta.


run_podman unshare --rootless-netns ip addr
is "$output" ".*${pasta_iface}.*"
}
9 changes: 9 additions & 0 deletions test/system/550-pause-process.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,17 @@
#

load helpers
load helpers.registry
load helpers.sig-proxy

function setup_file() {
mheon marked this conversation as resolved.
Show resolved Hide resolved
# We have to stop the background registry here. These tests kill the podman pause
# process which means commands after that are in a new one and when the cleanup
# later tries to stop the registry container it will be in the wrong ns and can fail.
# https://github.com/containers/podman/pull/21563#issuecomment-1960047648
stop_registry
}

function _check_pause_process() {
pause_pid=
if [[ -z "$pause_pid_file" ]]; then
Expand Down
17 changes: 17 additions & 0 deletions test/system/helpers.network.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,3 +369,20 @@ function tcp_port_probe() {

: | nc "${address}" "${1}"
}

### Pasta Helpers ##############################################################

function default_ifname() {
local ip_ver="${1}"

local expr='[.[] | select(.dst == "default").dev] | .[0]'
ip -j -"${ip_ver}" route show | jq -rM "${expr}"
}

function default_addr() {
local ip_ver="${1}"
local ifname="${2:-$(default_ifname "${ip_ver}")}"

local expr='.[0] | .addr_info[0].local'
ip -j -"${ip_ver}" addr show "${ifname}" | jq -rM "${expr}"
}
4 changes: 4 additions & 0 deletions vendor/github.com/containers/common/libimage/events.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading