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

Jump to bottom

Gate CNI support behind the cni build tag #1767

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Gate CNI support behind the cni build tag #1767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ run:
- systemd
- exclude_graphdriver_btrfs
- containers_image_openpgp
- cni
concurrency: 6
deadline: 5m
linters:
Expand Down
9 changes: 7 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,19 @@ build-cross:
$(call go-build,windows,386,${BUILDTAGS})

.PHONY: all
all: build-amd64 build-386
all: build-amd64 build-386 build-amd64-cni

.PHONY: build
build: build-amd64 build-386
build: build-amd64 build-386 build-amd64-cni

.PHONY: build-amd64
build-amd64:
GOARCH=amd64 $(GO_BUILD) -tags $(BUILDTAGS) ./...

.PHONY: build-amd64-cni
build-amd64-cni:
GOARCH=amd64 $(GO_BUILD) -tags $(BUILDTAGS),cni ./...

dcermak marked this conversation as resolved.
Show resolved Hide resolved
.PHONY: build-386
build-386:
ifneq ($(shell uname -s), Darwin)
Expand Down Expand Up @@ -100,6 +104,7 @@ test: test-unit
test-unit: netavark-testplugin
go test --tags seccomp,$(BUILDTAGS) -v ./...
go test --tags remote,$(BUILDTAGS) -v ./pkg/config
go test --tags cni,$(BUILDTAGS) -v ./libnetwork/cni

.PHONY: codespell
codespell:
Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/cni_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux || freebsd
//go:build (linux || freebsd) && cni

package cni

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/cni_exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build linux || freebsd
//go:build (linux || freebsd) && cni

package cni

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/cni_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux
//go:build (linux || freebsd) && cni

package cni_test

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/cni_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux || freebsd
//go:build (linux || freebsd) && cni

package cni

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux || freebsd
//go:build (linux || freebsd) && cni

package cni

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/config_freebsd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build freebsd
//go:build (linux || freebsd) && cni

package cni

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/config_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux
//go:build (linux || freebsd) && cni

package cni

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux
//go:build (linux || freebsd) && cni

package cni_test

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux || freebsd
//go:build (linux || freebsd) && cni

package cni

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux || freebsd
//go:build (linux || freebsd) && cni

package cni

Expand Down
2 changes: 1 addition & 1 deletion libnetwork/cni/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build linux
//go:build (linux || freebsd) && cni

package cni_test

Expand Down
179 changes: 61 additions & 118 deletions libnetwork/network/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,10 @@ import (
"os"
"path/filepath"

"github.com/containers/common/libnetwork/cni"
"github.com/containers/common/libnetwork/netavark"
"github.com/containers/common/libnetwork/types"
"github.com/containers/common/pkg/config"
"github.com/containers/common/pkg/machine"
"github.com/containers/storage"
"github.com/containers/storage/pkg/homedir"
"github.com/containers/storage/pkg/ioutils"
"github.com/containers/storage/pkg/unshare"
"github.com/sirupsen/logrus"
Expand All @@ -23,8 +20,6 @@ import (
const (
// defaultNetworkBackendFileName is the file name for sentinel file to store the backend
defaultNetworkBackendFileName = "defaultNetworkBackend"
// cniConfigDirRootless is the directory in XDG_CONFIG_HOME for cni plugins
cniConfigDirRootless = "cni/net.d/"

// netavarkBinary is the name of the netavark binary
netavarkBinary = "netavark"
Expand Down Expand Up @@ -52,146 +47,94 @@ func NetworkBackend(store storage.Store, conf *config.Config, syslog bool) (type
}
}

switch backend {
case types.Netavark:
netavarkBin, err := conf.FindHelperBinary(netavarkBinary, false)
if err != nil {
return "", nil, err
}
return backendFromType(backend, store, conf, syslog)
}

aardvarkBin, _ := conf.FindHelperBinary(aardvarkBinary, false)
func netavarkBackendFromConf(store storage.Store, conf *config.Config, syslog bool) (types.ContainerNetwork, error) {
netavarkBin, err := conf.FindHelperBinary(netavarkBinary, false)
if err != nil {
return nil, err
}

confDir := conf.Network.NetworkConfigDir
if confDir == "" {
confDir = getDefaultNetavarkConfigDir(store)
}
aardvarkBin, _ := conf.FindHelperBinary(aardvarkBinary, false)

// We cannot use the runroot for rootful since the network namespace is shared for all
// libpod instances they also have to share the same ipam db.
// For rootless we have our own network namespace per libpod instances,
// so this is not a problem there.
runDir := netavarkRunDir
if unshare.IsRootless() {
runDir = filepath.Join(store.RunRoot(), "networks")
}
confDir := conf.Network.NetworkConfigDir
if confDir == "" {
confDir = getDefaultNetavarkConfigDir(store)
}

netInt, err := netavark.NewNetworkInterface(&netavark.InitConfig{
Config: conf,
NetworkConfigDir: confDir,
NetworkRunDir: runDir,
NetavarkBinary: netavarkBin,
AardvarkBinary: aardvarkBin,
Syslog: syslog,
})
return types.Netavark, netInt, err
case types.CNI:
netInt, err := getCniInterface(conf)
return types.CNI, netInt, err

default:
return "", nil, fmt.Errorf("unsupported network backend %q, check network_backend in containers.conf", backend)
// We cannot use the runroot for rootful since the network namespace is shared for all
// libpod instances they also have to share the same ipam db.
// For rootless we have our own network namespace per libpod instances,
// so this is not a problem there.
runDir := netavarkRunDir
if unshare.IsRootless() {
runDir = filepath.Join(store.RunRoot(), "networks")
}

netInt, err := netavark.NewNetworkInterface(&netavark.InitConfig{
Config: conf,
NetworkConfigDir: confDir,
NetworkRunDir: runDir,
NetavarkBinary: netavarkBin,
AardvarkBinary: aardvarkBin,
Syslog: syslog,
})
return netInt, err
}

func defaultNetworkBackend(store storage.Store, conf *config.Config) (backend types.NetworkBackend, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this function would be a good candidate to have different build tagged versions - it would alleviate the need for the CNI supported const. The CNI unsupported version would just return netavark and do the update check/warn, while the supported version would go through the logic

Copy link
Member

Choose a reason for hiding this comment

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

Either this, or even one level above, the NetworkBackend() function, could be the diverging point for the buildtag. I think either of these might work better.

Copy link
Member

Choose a reason for hiding this comment

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

that is a good idea, although I am not sure the additional duplication of code is worth it.
Either way is fine for me so I wouldn't block on this.

// read defaultNetworkBackend file
err = nil

file := filepath.Join(store.GraphRoot(), defaultNetworkBackendFileName)
b, err := os.ReadFile(file)
if err == nil {
val := string(b)
if val == string(types.Netavark) {
return types.Netavark, nil
}
if val == string(types.CNI) {
return types.CNI, nil
}
Comment on lines -105 to -107
Copy link
Member

Choose a reason for hiding this comment

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

This will break backwards compatibility pretty hard. I haven't made up my mind yet what exactly should happen but this alone is broken because it will break even if CNI build tag is specified you will reject this old cni configuration which should not happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is why I have introduced the alternativeNetworkBackend function:
https://github.com/containers/common/pull/1767/files#diff-d9ace9efdaad94d582a127750587831dff5b06bd443107ba64557bd243bbcd72R57-R99

Copy link
Member

Choose a reason for hiding this comment

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

you will never end up in there because it now returns unknown network backend value "cni" ... driectly below here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right, my bad!

return "", fmt.Errorf("unknown network backend value %q in %q", val, file)
}
// fail for all errors except ENOENT
if !errors.Is(err, os.ErrNotExist) {
return "", fmt.Errorf("could not read network backend value: %w", err)
}

// cache the network backend to make sure always the same one will be used
defer func() {
writeBackendToFile := func(backendT types.NetworkBackend) {
// only write when there is no error
if err == nil {
if err := ioutils.AtomicWriteFile(file, []byte(backend), 0o644); err != nil {
if err := ioutils.AtomicWriteFile(file, []byte(backendT), 0o644); err != nil {
logrus.Errorf("could not write network backend to file: %v", err)
}
}
}()

_, err = conf.FindHelperBinary("netavark", false)
if err != nil {
// if we cannot find netavark use CNI
return types.CNI, nil
}

// If there are any containers then return CNI
cons, err := store.Containers()
if err != nil {
return "", err
}
if len(cons) != 0 {
return types.CNI, nil
}

// If there are any non ReadOnly images then return CNI
imgs, err := store.Images()
if err != nil {
return "", err
}
for _, i := range imgs {
if !i.ReadOnly {
return types.CNI, nil
}
}

// If there are CNI Networks then return CNI
cniInterface, err := getCniInterface(conf)
// read defaultNetworkBackend file
b, err := os.ReadFile(file)
if err == nil {
nets, err := cniInterface.NetworkList()
// there is always a default network so check > 1
if err != nil && !errors.Is(err, os.ErrNotExist) {
return "", err
}

if len(nets) > 1 {
// we do not have a fresh system so use CNI
return types.CNI, nil
}
}
return types.Netavark, nil
}
val := string(b)

func getCniInterface(conf *config.Config) (types.ContainerNetwork, error) {
confDir := conf.Network.NetworkConfigDir
if confDir == "" {
var err error
confDir, err = getDefaultCNIConfigDir()
if err != nil {
return nil, err
// if the network backend has been already set previously,
// handle the values depending on whether CNI is supported and
// whether the network backend is explicitly configured
if val == string(types.Netavark) {
// netavark is always good
return types.Netavark, nil
} else if val == string(types.CNI) {
if cniSupported {
return types.CNI, nil
}
// the user has *not* configured a network
// backend explicitly but used CNI in the past
// => we upgrade them in this case to netavark only
writeBackendToFile(types.Netavark)
logrus.Info("Migrating network backend to netavark as no backend has been configured previously")
return types.Netavark, nil
}
return "", fmt.Errorf("unknown network backend value %q in %q", val, file)
Copy link
Member

@ashley-cui ashley-cui Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this line is tripping our podman tests.

This test is running netavark with the netavark only build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To help in debugging: the failure is in f39 rootless and only in two of the e2e tests: add ssh socket path and kube play using a user namespace, and both failures look like this:

→ Enter [It] (test name)
Error: Error: failed to get default network backend: unknown network backend value "" in "[share]/containers/storage/defaultNetworkBackend"
...
No future change is possible.  Bailing out early after 0.741s.

I have no idea what "No future change is possible" means, it seems to be coming from ginkgo itself. All other e2e tests pass (1985 of them). I have no idea what is special about those two tests. I'm just posting direct links in hopes that someone else can understand the failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edsantiago The test run that @ashley-cui linked includes a lot more test failures. Could it be that there is some leftover polluted cache maybe?

Copy link
Member

Choose a reason for hiding this comment

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

The fact that it says unknown network backend value "" seems to suggest there is a conditioning in which it writes (or reads) an empty file which seems very bad.

Copy link
Member

Choose a reason for hiding this comment

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

@dcermak @edsantiago FWIW I changed the test pr to always build CNI, and it looks like all the rootless runs are failing. I think this file should only be created by this network backend code? I can try to help dig a little failure but @Luap99 's diagnosis seems correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashley-cui could you please rebase the test PRs? I've fixed the issue that @Luap99 found

Copy link
Member

Choose a reason for hiding this comment

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

Rebased!

}
return cni.NewCNINetworkInterface(&cni.InitConfig{
Config: conf,
CNIConfigDir: confDir,
RunDir: conf.Engine.TmpDir,
IsMachine: machine.IsGvProxyBased(),
})
}

func getDefaultCNIConfigDir() (string, error) {
if !unshare.IsRootless() {
return cniConfigDir, nil
// fail for all errors except ENOENT
if !errors.Is(err, os.ErrNotExist) {
return "", fmt.Errorf("could not read network backend value: %w", err)
}

configHome, err := homedir.GetConfigHome()
backend, err = networkBackendFromStore(store, conf)
if err != nil {
return "", err
}
return filepath.Join(configHome, cniConfigDirRootless), nil
// cache the network backend to make sure always the same one will be used
writeBackendToFile(backend)

return backend, nil
}

// getDefaultNetavarkConfigDir return the netavark config dir. For rootful it will
Expand Down
Loading
Loading