From fce1e5474610dc672ab8241a3a38ec7099d7a482 Mon Sep 17 00:00:00 2001 From: Tolga Ceylan Date: Fri, 8 Jun 2018 15:57:06 -0700 Subject: [PATCH] fn: remove dead code in static pool (#1052) Static pool is oriented for testing/basic usage and as it's name implies it is a static pool. Therefore, removing unnecessary/dead code. --- api/agent/static_pool.go | 103 +-------------------------- api/agent/static_pool_test.go | 130 ++-------------------------------- 2 files changed, 9 insertions(+), 224 deletions(-) diff --git a/api/agent/static_pool.go b/api/agent/static_pool.go index 6f6a70661..b1c8c3c5e 100644 --- a/api/agent/static_pool.go +++ b/api/agent/static_pool.go @@ -2,26 +2,17 @@ package agent import ( "context" - "errors" - "sync" pool "github.com/fnproject/fn/api/runnerpool" "github.com/sirupsen/logrus" ) -var ( - ErrorPoolClosed = errors.New("Runner pool closed") - ErrorPoolRunnerExists = errors.New("Runner already exists") -) - // manages a single set of runners ignoring lb groups type staticRunnerPool struct { generator pool.MTLSRunnerFactory pki *pool.PKIData // can be nil when running in insecure mode runnerCN string - rMtx *sync.RWMutex runners []pool.Runner - isClosed bool } func DefaultStaticRunnerPool(runnerAddresses []string) pool.RunnerPool { @@ -41,7 +32,6 @@ func NewStaticRunnerPool(runnerAddresses []string, pki *pool.PKIData, runnerCN s runners = append(runners, r) } return &staticRunnerPool{ - rMtx: &sync.RWMutex{}, runners: runners, pki: pki, runnerCN: runnerCN, @@ -49,104 +39,15 @@ func NewStaticRunnerPool(runnerAddresses []string, pki *pool.PKIData, runnerCN s } } -func (rp *staticRunnerPool) shutdown() []pool.Runner { - rp.rMtx.Lock() - defer rp.rMtx.Unlock() - - if rp.isClosed { - return nil - } - - rp.isClosed = true - toRemove := rp.runners[:] - rp.runners = nil - - return toRemove -} - -func (rp *staticRunnerPool) addRunner(runner pool.Runner) error { - rp.rMtx.Lock() - defer rp.rMtx.Unlock() - - if rp.isClosed { - return ErrorPoolClosed - } - - for _, r := range rp.runners { - if r.Address() == runner.Address() { - return ErrorPoolRunnerExists - } - } - - rp.runners = append(rp.runners, runner) - return nil -} - -func (rp *staticRunnerPool) removeRunner(address string) pool.Runner { - rp.rMtx.Lock() - defer rp.rMtx.Unlock() - - for i, r := range rp.runners { - if r.Address() == address { - rp.runners = append(rp.runners[:i], rp.runners[i+1:]...) - return r - } - } - return nil -} - -func (rp *staticRunnerPool) getRunners() ([]pool.Runner, error) { - rp.rMtx.RLock() - defer rp.rMtx.RUnlock() - - if rp.isClosed { - return nil, ErrorPoolClosed - } - +func (rp *staticRunnerPool) Runners(call pool.RunnerCall) ([]pool.Runner, error) { r := make([]pool.Runner, len(rp.runners)) copy(r, rp.runners) - return r, nil } -func (rp *staticRunnerPool) Runners(call pool.RunnerCall) ([]pool.Runner, error) { - return rp.getRunners() -} - -func (rp *staticRunnerPool) AddRunner(address string) error { - r, err := rp.generator(address, rp.runnerCN, rp.pki) - if err != nil { - logrus.WithError(err).WithField("runner_addr", address).Warn("Failed to add runner") - return err - } - - err = rp.addRunner(r) - if err != nil { - err2 := r.Close(context.Background()) - if err2 != nil { - logrus.WithError(err2).WithField("runner_addr", address).Warn("Error closing runner on AddRunner failure") - } - } - return err -} - -func (rp *staticRunnerPool) RemoveRunner(address string) { - toRemove := rp.removeRunner(address) - if toRemove == nil { - return - } - - err := toRemove.Close(context.Background()) - if err != nil { - logrus.WithError(err).WithField("runner_addr", toRemove.Address()).Error("Error closing runner") - } -} - func (rp *staticRunnerPool) Shutdown(ctx context.Context) error { - toRemove := rp.shutdown() - var retErr error - for _, r := range toRemove { + for _, r := range rp.runners { err := r.Close(ctx) if err != nil { logrus.WithError(err).WithField("runner_addr", r.Address()).Error("Error closing runner") diff --git a/api/agent/static_pool_test.go b/api/agent/static_pool_test.go index c990a868a..07c179424 100644 --- a/api/agent/static_pool_test.go +++ b/api/agent/static_pool_test.go @@ -47,6 +47,11 @@ func TestNewStaticPool(t *testing.T) { if len(runners) != len(addrs) { t.Fatalf("Invalid number of runners %v", len(runners)) } + + err = np.Shutdown(context.Background()) + if err != ErrorGarbanzoBeans { + t.Fatalf("Expected garbanzo beans error from shutdown %v", err) + } } func TestEmptyPool(t *testing.T) { @@ -60,129 +65,8 @@ func TestEmptyPool(t *testing.T) { t.Fatalf("Invalid number of runners %v", len(runners)) } - err = np.AddRunner("127.0.0.1:8082") - if err != nil { - t.Fatalf("Failed to add runner %v", err) - } - - runners, err = np.Runners(nil) - if err != nil { - t.Fatalf("Failed to list runners %v", err) - } - if len(runners) != 1 { - t.Fatalf("Invalid number of runners %v", len(runners)) - } - err = np.Shutdown(context.Background()) - if err != ErrorGarbanzoBeans { - t.Fatalf("Expected garbanzo beans error from shutdown %v", err) - } - - runners, err = np.Runners(nil) - if err == nil { - t.Fatalf("Should fail to list runners (shutdown)") - } - if len(runners) != 0 { - t.Fatalf("Invalid number of runners %v", len(runners)) - } -} - -func TestAddNodeToPool(t *testing.T) { - addrs := []string{"127.0.0.1:8080", "127.0.0.1:8081"} - np := setupStaticPool(addrs).(*staticRunnerPool) - - err := np.AddRunner("127.0.0.1:8082") - if err != nil { - t.Fatalf("Add Should not fail %v", err) - } - - err = np.AddRunner("127.0.0.1:8082") - if err != ErrorPoolRunnerExists { - t.Fatalf("Add Should fail since duplicate %v", err) - } - - runners, err := np.Runners(nil) - if err != nil { - t.Fatalf("Failed to list runners %v", err) - } - if len(runners) != 3 { - t.Fatalf("Invalid number of runners %v", len(runners)) - } - - err = np.Shutdown(context.Background()) - if err != ErrorGarbanzoBeans { - t.Fatalf("Expected garbanzo beans error from shutdown %v", err) - } - - runners, err = np.Runners(nil) - if err == nil { - t.Fatalf("Should fail to list runners (shutdown)") - } - if len(runners) != 0 { - t.Fatalf("Invalid number of runners %v", len(runners)) - } -} - -func TestRemoveNodeFromPool(t *testing.T) { - addrs := []string{"127.0.0.1:8080", "127.0.0.1:8081"} - np := setupStaticPool(addrs).(*staticRunnerPool) - - np.RemoveRunner("127.0.0.1:8081") - - runners, err := np.Runners(nil) - if err != nil { - t.Fatalf("Failed to list runners %v", err) - } - - if len(runners) != 1 { - t.Fatalf("Invalid number of runners %v", len(runners)) - } - - np.RemoveRunner("127.0.0.1:8081") - - runners, err = np.Runners(nil) - if err != nil { - t.Fatalf("Failed to list runners %v", err) - } - if len(runners) != 1 { - t.Fatalf("Invalid number of runners %v", len(runners)) - } - - np.RemoveRunner("127.0.0.1:8080") - - runners, err = np.Runners(nil) - if err != nil { - t.Fatalf("Failed to list runners %v", err) - } - if len(runners) != 0 { - t.Fatalf("Invalid number of runners %v", len(runners)) - } - - np.RemoveRunner("127.0.0.1:8080") - - runners, err = np.Runners(nil) - if err != nil { - t.Fatalf("Failed to list runners %v", err) - } - if len(runners) != 0 { - t.Fatalf("Invalid number of runners %v", len(runners)) - } - - // Let's try a double shutdown - err = np.Shutdown(context.Background()) - if err != nil { - t.Fatalf("Not expected error from shutdown I (empty pool) %v", err) - } - err = np.Shutdown(context.Background()) - if err != nil { - t.Fatalf("Not expected error from shutdown II (empty pool) %v", err) - } - - runners, err = np.Runners(nil) - if err == nil { - t.Fatalf("Should fail to list runners (shutdown)") - } - if len(runners) != 0 { - t.Fatalf("Invalid number of runners %v", len(runners)) + if err != nil { + t.Fatalf("Unexpected error from shutdown %v", err) } }