diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 8d6311a6db2..0e4a435a912 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -21,8 +21,6 @@ updates: - dependency-name: "antrea.io/ofnet" - dependency-name: "antrea.io/libOpenflow" - dependency-name: "github.com/ClickHouse/clickhouse-go/v2" # auto-upgrade involves dependency conflicts - - dependency-name: "github.com/alexflint/go-filemutex" - update-types: ["version-update:semver-major"] # ignore major updates only - package-ecosystem: "github-actions" # Workflow files stored in the default location of `.github/workflows` directory: "/" diff --git a/build/yamls/antrea.yml b/build/yamls/antrea.yml index 3227814289f..7aa1d191934 100644 --- a/build/yamls/antrea.yml +++ b/build/yamls/antrea.yml @@ -5528,7 +5528,7 @@ data: # Enable flowexporter which exports polled conntrack connections as IPFIX flow records from each # agent to a configured collector. - # FlowExporter: false + FlowExporter: true # Enable collecting and exposing NetworkPolicy statistics. # NetworkPolicyStats: true @@ -5700,7 +5700,7 @@ data: # IPFIX flow records from each agent to a configured collector. To enable this # feature, you need to set "enable" to true, and ensure that the FlowExporter # feature gate is also enabled. - enable: false + enable: true # Provide the IPFIX collector address as a string with format :[][:]. # HOST can either be the DNS name, IP, or Service name of the Flow Collector. If # using an IP, it can be either IPv4 or IPv6. However, IPv6 address should be diff --git a/build/yamls/flow-aggregator.yml b/build/yamls/flow-aggregator.yml index 07e677d83ad..87c5d291efd 100644 --- a/build/yamls/flow-aggregator.yml +++ b/build/yamls/flow-aggregator.yml @@ -292,7 +292,7 @@ data: # FlowLogger contains configuration options for writing flow records to a local log file. flowLogger: # Enable is the switch to enable writing flow records to a local log file. - enable: false + enable: true # Path is the path to the local log file. path: "/tmp/antrea-flows.log" diff --git a/go.mod b/go.mod index 254d7e0b2ac..b710584292f 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,6 @@ require ( github.com/Microsoft/go-winio v0.6.1 github.com/Microsoft/hcsshim v0.9.10 github.com/TomCodeLV/OVSDB-golang-lib v0.0.0-20200116135253-9bbdfadcd881 - github.com/alexflint/go-filemutex v1.2.0 github.com/aws/aws-sdk-go-v2 v1.16.10 github.com/aws/aws-sdk-go-v2/config v1.16.0 github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.11.23 @@ -91,6 +90,7 @@ require ( github.com/MakeNowJust/heredoc v1.0.0 // indirect github.com/NYTimes/gziphandler v1.1.1 // indirect github.com/VividCortex/ewma v1.2.0 // indirect + github.com/alexflint/go-filemutex v1.2.0 // indirect github.com/andybalholm/brotli v1.0.4 // indirect github.com/antlr/antlr4/runtime/Go/antlr v1.4.10 // indirect github.com/armon/go-metrics v0.0.0-20180917152333-f0300d1749da // indirect 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..463ba7a4a81 100644 --- a/pkg/agent/cniserver/ipam/hostlocal/gc_test.go +++ b/pkg/agent/cniserver/ipam/hostlocal/gc_test.go @@ -176,3 +176,56 @@ 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 { + idx++ + 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() -}