diff --git a/pkg/agent/cniserver/ipam/hostlocal/gc.go b/pkg/agent/cniserver/ipam/hostlocal/gc.go index 3555488611e..7c0a30e1a40 100644 --- a/pkg/agent/cniserver/ipam/hostlocal/gc.go +++ b/pkg/agent/cniserver/ipam/hostlocal/gc.go @@ -22,12 +22,14 @@ import ( "runtime" "strings" + "github.com/containernetworking/plugins/plugins/ipam/host-local/backend/disk" "github.com/spf13/afero" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" ) -const dataDir = "/var/lib/cni/networks" +// dataDir is a variable so it can be overridden by tests if needed +var dataDir = "/var/lib/cni/networks" func networkDir(network string) string { return filepath.Join(dataDir, network) @@ -48,7 +50,7 @@ func GarbageCollectContainerIPs(network string, desiredIPs sets.Set[string]) err return fmt.Errorf("path '%s' is not a directory: %w", dir, err) } - lk, err := NewFileLock(dataDir) + lk, err := disk.NewFileLock(dir) if err != nil { return err } @@ -75,33 +77,32 @@ func gcContainerIPs(fs afero.Fs, dir string, desiredIPs sets.Set[string]) error return fmt.Errorf("error when gathering IP filenames in the host-local data directory: %w", err) } - allocatedIPs := sets.New[string]() + hasRemovalError := false for _, p := range paths { ip := getIPFromPath(p) if net.ParseIP(ip) == nil { // not a valid IP, nothing to do continue } - allocatedIPs.Insert(ip) if desiredIPs.Has(ip) { // IP is in-use continue } if err := fs.Remove(p); err != nil { klog.ErrorS(err, "Failed to release unused IP from host-local IPAM plugin", "IP", ip) + hasRemovalError = true continue } - allocatedIPs.Delete(ip) klog.InfoS("Unused IP was successfully released from host-local IPAM plugin", "IP", ip) } - if allocatedIPs.Difference(desiredIPs).Len() > 0 { + if hasRemovalError { return fmt.Errorf("not all unused IPs could be released from host-local IPAM plugin, some IPs may be leaked") } - // Note that it is perfectly possible for some IPs to be in desiredIPs but not in - // allocatedIPs. This can be the case when another IPAM plugin (e.g., AntreaIPAM) is also - // used. + // Note that it is perfectly possible for some IPs to be in desiredIPs but not in the + // host-local data directory. This can be the case when another IPAM plugin (e.g., + // AntreaIPAM) is also used. return nil } diff --git a/pkg/agent/cniserver/ipam/hostlocal/gc_test.go b/pkg/agent/cniserver/ipam/hostlocal/gc_test.go index 6ac29abd7a8..f59fbf754af 100644 --- a/pkg/agent/cniserver/ipam/hostlocal/gc_test.go +++ b/pkg/agent/cniserver/ipam/hostlocal/gc_test.go @@ -176,3 +176,55 @@ func TestGcContainerIPs(t *testing.T) { runTests(t, false) runTests(t, true) } + +// TestGarbageCollectContainerIPs tests some edge cases and logic that depends on the real OS +// filesystem. The actual GC logic is tested by TestGcContainerIPs. +func TestGarbageCollectContainerIPs(t *testing.T) { + ips := sets.New[string]() + tempDir, err := os.MkdirTemp("", "test-networks") + require.NoError(t, err) + savedDir := dataDir + defer func() { + dataDir = savedDir + }() + dataDir = tempDir + defer os.RemoveAll(tempDir) + + idx := 0 + networkName := func() string { + return fmt.Sprintf("net%d", idx) + } + + lockFile := func(network string) string { + return filepath.Join(tempDir, network, "lock") + } + + t.Run("missing directory", func(t *testing.T) { + network := networkName() + // there is no directory in tempDir for the "antrea" network + // we don't expect an error, and the lock file should not be created + require.NoError(t, GarbageCollectContainerIPs(network, ips)) + assert.NoFileExists(t, lockFile(network)) + }) + + t.Run("not a directory", func(t *testing.T) { + network := networkName() + netDir := filepath.Join(tempDir, network) + // create a file instead of a directory: GarbageCollectContainerIPs should return an + // error + _, err := os.Create(netDir) + require.NoError(t, err) + defer os.Remove(netDir) + assert.ErrorContains(t, GarbageCollectContainerIPs(network, ips), "not a directory") + }) + + t.Run("lock file created", func(t *testing.T) { + network := networkName() + netDir := filepath.Join(tempDir, network) + require.NoError(t, os.Mkdir(netDir, 0o755)) + defer os.RemoveAll(netDir) + // make sure that the lock file is created in the right place + require.NoError(t, GarbageCollectContainerIPs(network, ips)) + assert.FileExists(t, lockFile(network)) + }) +} diff --git a/pkg/agent/cniserver/ipam/hostlocal/lock.go b/pkg/agent/cniserver/ipam/hostlocal/lock.go deleted file mode 100644 index e2eada4ff62..00000000000 --- a/pkg/agent/cniserver/ipam/hostlocal/lock.go +++ /dev/null @@ -1,62 +0,0 @@ -// Copyright 2023 Antrea Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package hostlocal - -import ( - "os" - "path" - - "github.com/alexflint/go-filemutex" -) - -// This code was copied from https://github.com/containernetworking/plugins/blob/v1.3.0/plugins/ipam/host-local/backend/disk/lock.go - -// FileLock wraps os.File to be used as a lock using flock -type FileLock struct { - f *filemutex.FileMutex -} - -// NewFileLock opens file/dir at path and returns unlocked FileLock object -func NewFileLock(lockPath string) (*FileLock, error) { - fi, err := os.Stat(lockPath) - if err != nil { - return nil, err - } - - if fi.IsDir() { - lockPath = path.Join(lockPath, "lock") - } - - f, err := filemutex.New(lockPath) - if err != nil { - return nil, err - } - - return &FileLock{f}, nil -} - -func (l *FileLock) Close() error { - return l.f.Close() -} - -// Lock acquires an exclusive lock -func (l *FileLock) Lock() error { - return l.f.Lock() -} - -// Unlock releases the lock -func (l *FileLock) Unlock() error { - return l.f.Unlock() -}