Retry ipset DestroySet/DestroyAllSets if error indicates in use

go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/projectcalico/api v0.0.0-20230602153125-fb7148692637
 	github.com/prometheus-community/pro-bing v0.4.0
 	github.com/prometheus/client_golang v1.19.0
-	github.com/submariner-io/admiral v0.18.0-m2
+	github.com/submariner-io/admiral v0.18.0-m2.0.20240417062804-5cfcef6bc3ba
 	github.com/submariner-io/shipyard v0.18.0-m2
 	github.com/vishvananda/netlink v1.2.1-beta.2
 	golang.org/x/net v0.24.0
@@ -27,7 +27,7 @@ require (
 	k8s.io/client-go v0.29.3
 	k8s.io/component-helpers v0.29.3
 	k8s.io/utils v0.0.0-20230726121419-3b25d923346b
-	sigs.k8s.io/controller-runtime v0.17.2
+	sigs.k8s.io/controller-runtime v0.17.3
 	sigs.k8s.io/knftables v0.0.16
 	sigs.k8s.io/mcs-api v0.1.0
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.1

go.sum

@@ -503,8 +503,8 @@ github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO
 github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
 github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
 github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
-github.com/submariner-io/admiral v0.18.0-m2 h1:SGW+azCnbCAe/K1pAC1NeKJ3kR+poY1d99Gq8ZGAMsY=
-github.com/submariner-io/admiral v0.18.0-m2/go.mod h1:yaPz+wX65Ew3iVulB3T7r/JqYUwcT6RxTS/oGnZaNTo=
+github.com/submariner-io/admiral v0.18.0-m2.0.20240417062804-5cfcef6bc3ba h1:2ERBixskBiw1BsTtPt2XKKtAiUg/GofmxCwPqNNnL98=
+github.com/submariner-io/admiral v0.18.0-m2.0.20240417062804-5cfcef6bc3ba/go.mod h1:GR4J3O4aLA5VXQyWcFgvfqn4ahWbfh93a2TIISPO4k8=
 github.com/submariner-io/shipyard v0.18.0-m2 h1:9jnyioyAvotl6GMb/6gHAFoEcOFmjR0ixUl4AqzJGiA=
 github.com/submariner-io/shipyard v0.18.0-m2/go.mod h1:gqN3jA+kUYv1CDS95Wopfj8vXCfBJmL/NupebzB8KBc=
 github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww=
@@ -783,8 +783,8 @@ k8s.io/api v0.29.3 h1:2ORfZ7+bGC3YJqGpV0KSDDEVf8hdGQ6A03/50vj8pmw=
 k8s.io/api v0.29.3/go.mod h1:y2yg2NTyHUUkIoTC+phinTnEa3KFM6RZ3szxt014a80=
 k8s.io/apiextensions-apiserver v0.18.2/go.mod h1:q3faSnRGmYimiocj6cHQ1I3WpLqmDgJFlKL37fC4ZvY=
 k8s.io/apiextensions-apiserver v0.18.4/go.mod h1:NYeyeYq4SIpFlPxSAB6jHPIdvu3hL0pc36wuRChybio=
-k8s.io/apiextensions-apiserver v0.29.0 h1:0VuspFG7Hj+SxyF/Z/2T0uFbI5gb5LRgEyUVE3Q4lV0=
-k8s.io/apiextensions-apiserver v0.29.0/go.mod h1:TKmpy3bTS0mr9pylH0nOt/QzQRrW7/h7yLdRForMZwc=
+k8s.io/apiextensions-apiserver v0.29.2 h1:UK3xB5lOWSnhaCk0RFZ0LUacPZz9RY4wi/yt2Iu+btg=
+k8s.io/apiextensions-apiserver v0.29.2/go.mod h1:aLfYjpA5p3OwtqNXQFkhJ56TB+spV8Gc4wfMhUA3/b8=
 k8s.io/apimachinery v0.18.2/go.mod h1:9SnR/e11v5IbyPCGbvJViimtJ0SwHG4nfZFjU77ftcA=
 k8s.io/apimachinery v0.18.4/go.mod h1:OaXp26zu/5J7p0f92ASynJa1pZo06YlV9fG7BoWbCko=
 k8s.io/apimachinery v0.29.3 h1:2tbx+5L7RNvqJjn7RIuIKu9XTsIZ9Z5wX2G22XAa5EU=
@@ -819,8 +819,8 @@ k8s.io/utils v0.0.0-20230726121419-3b25d923346b h1:sgn3ZU783SCgtaSJjpcVVlRqd6GSn
 k8s.io/utils v0.0.0-20230726121419-3b25d923346b/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
 sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.7/go.mod h1:PHgbrJT7lCHcxMU+mDHEm+nx46H4zuuHZkDP6icnhu0=
 sigs.k8s.io/controller-runtime v0.6.1/go.mod h1:XRYBPdbf5XJu9kpS84VJiZ7h/u1hF3gEORz0efEja7A=
-sigs.k8s.io/controller-runtime v0.17.2 h1:FwHwD1CTUemg0pW2otk7/U5/i5m2ymzvOXdbeGOUvw0=
-sigs.k8s.io/controller-runtime v0.17.2/go.mod h1:+MngTvIQQQhfXtwfdGw/UOQ/aIaqsYywfCINOtwMO/s=
+sigs.k8s.io/controller-runtime v0.17.3 h1:65QmN7r3FWgTxDMz9fvGnO1kbf2nu+acg9p2R9oYYYk=
+sigs.k8s.io/controller-runtime v0.17.3/go.mod h1:N0jpP5Lo7lMTF9aL56Z/B2oWBJjey6StQM0jRbKQXtY=
 sigs.k8s.io/controller-tools v0.3.0/go.mod h1:enhtKGfxZD1GFEoMgP8Fdbu+uKQ/cq1/WGJhdVChfvI=
 sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo=
 sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0=

pkg/ipset/ipset.go

@@ -53,13 +53,17 @@ import (
 	"bytes"
 	"fmt"
 	"net"
+	"os/exec"
 	"regexp"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/pkg/errors"
+	"github.com/submariner-io/admiral/pkg/command"
 	"github.com/submariner-io/admiral/pkg/log"
-	utilexec "k8s.io/utils/exec"
+	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/client-go/util/retry"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -331,27 +335,17 @@ func (e *Entry) String() string {
 	return ""
 }
 
-type runner struct {
-	exec utilexec.Interface
-}
-
-var NewFunc func() Interface
+type runner struct{}
 
 // New returns a new Interface which will exec ipset.
-func New(exec utilexec.Interface) Interface {
-	if NewFunc != nil {
-		return NewFunc()
-	}
-
-	return &runner{
-		exec: exec,
-	}
+func New() Interface {
+	return &runner{}
 }
 
 func (runner *runner) runWithOutput(args []string, errFormat string, a ...interface{}) (string, error) {
 	logger.V(log.DEBUG).Infof("Running ipset %v", args)
 
-	out, err := runner.exec.Command(IPSetCmd, args...).CombinedOutput()
+	out, err := command.New(exec.Command(IPSetCmd, args...)).CombinedOutput()
 	if err != nil {
 		return "", fmt.Errorf("%s: %w (%s)", fmt.Sprintf(errFormat, a...), err, out)
 	}
@@ -489,9 +483,24 @@ func (runner *runner) FlushSet(set string) error {
 	return err
 }
 
+func (runner *runner) retryIfInUse(args []string, errFormat string, a ...interface{}) error {
+	backoff := wait.Backoff{
+		Duration: 100 * time.Millisecond,
+		Factor:   1.0,
+		Steps:    20,
+	}
+
+	//nolint:wrapcheck // No need to wrap.
+	return retry.OnError(backoff, func(err error) bool {
+		return strings.Contains(err.Error(), "is in use")
+	}, func() error {
+		return runner.run(args, errFormat, a...)
+	})
+}
+
 // DestroySet is used to destroy a named set.
 func (runner *runner) DestroySet(set string) error {
-	err := runner.run([]string{"destroy", set}, "error destroying set %q", set)
+	err := runner.retryIfInUse([]string{"destroy", set}, "error destroying set %q", set)
 	if IsNotFoundError(err) {
 		return nil
 	}
@@ -501,7 +510,7 @@ func (runner *runner) DestroySet(set string) error {
 
 // DestroyAllSets is used to destroy all sets.
 func (runner *runner) DestroyAllSets() error {
-	return runner.run([]string{"destroy"}, "error destroying all sets")
+	return runner.retryIfInUse([]string{"destroy"}, "error destroying all sets")
 }
 
 // ListSets list all set names from kernel.
@@ -545,13 +554,14 @@ func (runner *runner) ListAllSetInfo() (string, error) {
 
 // GetVersion returns the version string.
 func (runner *runner) GetVersion() (string, error) {
-	return getIPSetVersionString(runner.exec)
+	return getIPSetVersionString()
 }
 
 // getIPSetVersionString runs "ipset --version" to get the version string  in the form of "X.Y", i.e "6.19".
-func getIPSetVersionString(exec utilexec.Interface) (string, error) {
-	cmd := exec.Command(IPSetCmd, "--version")
-	cmd.SetStdin(bytes.NewReader([]byte{}))
+func getIPSetVersionString() (string, error) {
+	osCmd := exec.Command(IPSetCmd, "--version")
+	osCmd.Stdin = bytes.NewReader([]byte{})
+	cmd := command.New(osCmd)
 
 	cmdBytes, err := cmd.CombinedOutput()
 	if err != nil {

pkg/ipset/ipset_test.go

@@ -0,0 +1,101 @@
+/*
+SPDX-License-Identifier: Apache-2.0
+
+Copyright Contributors to the Submariner project.
+
+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 ipset_test
+
+import (
+	"errors"
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	fakecommand "github.com/submariner-io/admiral/pkg/command/fake"
+	"github.com/submariner-io/submariner/pkg/ipset"
+)
+
+const testSet = "test-set"
+
+var IPSetPathMatcher = HaveSuffix(ipset.IPSetCmd)
+
+func TestIPSet(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "IP Set Suite")
+}
+
+var _ = Describe("DestroySet", func() {
+	t := newTestDriver()
+
+	It("should invoke the correct command", func() {
+		err := t.ipSet.DestroySet(testSet)
+		Expect(err).To(Succeed())
+		t.cmdExecutor.AwaitCommand(IPSetPathMatcher, "destroy", testSet)
+	})
+
+	When("the set does not exist", func() {
+		It("should succeed", func() {
+			t.cmdExecutor.SetupCommandOutputWithError("The set with the given name does not exist",
+				errors.New("exit status 1"), IPSetPathMatcher, "destroy")
+			err := t.ipSet.DestroySet(testSet)
+			Expect(err).To(Succeed())
+		})
+	})
+
+	When("the set is initially in use", func() {
+		It("should retry and succeed", func() {
+			t.cmdExecutor.SetupCommandOutputWithError("Set cannot be destroyed: it is in use by a kernel component",
+				errors.New("exit status 1"), IPSetPathMatcher, "destroy")
+			err := t.ipSet.DestroySet(testSet)
+			Expect(err).To(Succeed())
+		})
+	})
+})
+
+var _ = Describe("DestroyAllSets", func() {
+	t := newTestDriver()
+
+	It("should invoke the correct command", func() {
+		err := t.ipSet.DestroyAllSets()
+		Expect(err).To(Succeed())
+		t.cmdExecutor.AwaitCommand(IPSetPathMatcher, "destroy")
+	})
+
+	When("a set is initially in use", func() {
+		It("should retry and succeed", func() {
+			t.cmdExecutor.SetupCommandOutputWithError("Set cannot be destroyed: it is in use by a kernel component",
+				errors.New("exit status 1"), IPSetPathMatcher, "destroy")
+			err := t.ipSet.DestroyAllSets()
+			Expect(err).To(Succeed())
+		})
+	})
+})
+
+type testDriver struct {
+	cmdExecutor *fakecommand.Executor
+	ipSet       ipset.Interface
+}
+
+func newTestDriver() *testDriver {
+	t := &testDriver{}
+
+	BeforeEach(func() {
+		t.cmdExecutor = fakecommand.New()
+		t.ipSet = ipset.New()
+	})
+
+	return t
+}

pkg/packetfilter/iptables/iptables.go

@@ -26,7 +26,6 @@ import (
 	"github.com/submariner-io/admiral/pkg/log"
 	"github.com/submariner-io/submariner/pkg/ipset"
 	"github.com/submariner-io/submariner/pkg/packetfilter"
-	utilexec "k8s.io/utils/exec"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -79,7 +78,7 @@ func New() (packetfilter.Driver, error) {
 		return nil, errors.Wrap(err, "error creating IP tables")
 	}
 
-	ipSetIface := ipset.New(utilexec.New())
+	ipSetIface := ipset.New()
 
 	return &packetFilter{
 		ipt:        ipt,