Addressing PR feedback

Makefile

@@ -458,12 +458,12 @@ static-checks:
 .PHONY: ut-no-cover
 ut-no-cover: vendor/.up-to-date $(FELIX_GO_FILES)
 	@echo Running Go UTs without coverage.
-	$(DOCKER_GO_BUILD) ginkgo -r -skipPackage fv,k8sfv,windataplane $(GINKGO_OPTIONS)
+	$(DOCKER_GO_BUILD) ginkgo -r -skipPackage fv,k8sfv,windows $(GINKGO_OPTIONS)
 
 .PHONY: ut-watch
 ut-watch: vendor/.up-to-date $(FELIX_GO_FILES)
 	@echo Watching go UTs for changes...
-	$(DOCKER_GO_BUILD) ginkgo watch -r -skipPackage fv,k8sfv,windataplane $(GINKGO_OPTIONS)
+	$(DOCKER_GO_BUILD) ginkgo watch -r -skipPackage fv,k8sfv,windows $(GINKGO_OPTIONS)
 
 # Launch a browser with Go coverage stats for the whole project.
 .PHONY: cover-browser

dataplane-drivers/driver.go → dataplane/driver.go

@@ -14,7 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package dataplanedriver
+package dataplane
 
 import (
 	"net"
@@ -23,8 +23,8 @@ import (
 	log "github.com/sirupsen/logrus"
 
 	"github.com/projectcalico/felix/config"
-	"github.com/projectcalico/felix/dataplane-drivers/extdataplane"
-	"github.com/projectcalico/felix/dataplane-drivers/intdataplane"
+	"github.com/projectcalico/felix/dataplane/external"
+	"github.com/projectcalico/felix/dataplane/linux"
 	"github.com/projectcalico/felix/ifacemonitor"
 	"github.com/projectcalico/felix/ipsets"
 	"github.com/projectcalico/felix/logutils"
@@ -34,7 +34,7 @@ import (
 
 func StartDataplaneDriver(configParams *config.Config, healthAggregator *health.HealthAggregator) (DataplaneDriver, *exec.Cmd) {
 	if configParams.UseInternalDataplaneDriver {
-		log.Info("Using internal dataplane driver.")
+		log.Info("Using internal (linux) dataplane driver.")
 		// Dedicated mark bits for accept and pass actions.  These are long lived bits
 		// that we use for communicating between chains.
 		markAccept := configParams.NextIptablesMark()

dataplane-drivers/driver_defs.go → dataplane/driver_defs.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package dataplanedriver
+package dataplane
 
 type DataplaneDriver interface {
 	SendMessage(msg interface{}) error

dataplane-drivers/driver_windows.go → dataplane/driver_windows.go

@@ -12,23 +12,24 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package dataplanedriver
+package dataplane
 
 import (
 	"os/exec"
 
 	log "github.com/sirupsen/logrus"
 
 	"github.com/projectcalico/felix/config"
-	"github.com/projectcalico/felix/dataplane-drivers/windataplane"
+	"github.com/projectcalico/felix/dataplane/windows"
 	"github.com/projectcalico/libcalico-go/lib/health"
 )
 
 func StartDataplaneDriver(configParams *config.Config, healthAggregator *health.HealthAggregator) (DataplaneDriver, *exec.Cmd) {
 	log.Info("Using Windows dataplane driver.")
 
 	dpConfig := windataplane.Config{
-		IPv6Enabled: configParams.Ipv6Support,
+		IPv6Enabled:      configParams.Ipv6Support,
+		HealthAggregator: healthAggregator,
 	}
 
 	winDP := windataplane.NewWinDataplaneDriver(dpConfig)

dataplane-drivers/intdataplane/int_dataplane.go → dataplane/linux/int_dataplane.go

@@ -15,7 +15,6 @@
 package intdataplane
 
 import (
-	"fmt"
 	"io/ioutil"
 	"os"
 	"reflect"
@@ -571,7 +570,7 @@ func (d *InternalDataplane) loopUpdatingDataplane() {
 	datastoreInSync := false
 
 	processMsgFromCalcGraph := func(msg interface{}) {
-		log.WithField("msg", msgStringer{msg: msg}).Infof(
+		log.WithField("msg", proto.MsgStringer{Msg: msg}).Infof(
 			"Received %T update from calculation graph", msg)
 		d.recordMsgStat(msg)
 		for _, mgr := range d.allManagers {
@@ -943,42 +942,6 @@ type iptablesTable interface {
 	RemoveChainByName(name string)
 }
 
-// msgStringer wraps an API message to customise how we stringify it.  For example, it truncates
-// the lists of members in the (potentially very large) IPSetsUpdate messages.
-type msgStringer struct {
-	msg interface{}
-}
-
-func (m msgStringer) String() string {
-	if log.GetLevel() < log.DebugLevel && m.msg != nil {
-		const truncateAt = 10
-		switch msg := m.msg.(type) {
-		case *proto.IPSetUpdate:
-			if len(msg.Members) < truncateAt {
-				return fmt.Sprintf("%v", msg)
-			}
-			return fmt.Sprintf("id:%#v members(%d):%#v(truncated)",
-				msg.Id, len(msg.Members), msg.Members[:truncateAt])
-		case *proto.IPSetDeltaUpdate:
-			if len(msg.AddedMembers) < truncateAt && len(msg.RemovedMembers) < truncateAt {
-				return fmt.Sprintf("%v", msg)
-			}
-			addedNum := truncateAt
-			removedNum := truncateAt
-			if len(msg.AddedMembers) < addedNum {
-				addedNum = len(msg.AddedMembers)
-			}
-			if len(msg.RemovedMembers) < removedNum {
-				removedNum = len(msg.RemovedMembers)
-			}
-			return fmt.Sprintf("id:%#v addedMembers(%d):%#v(truncated) removedMembers(%d):%#v(truncated)",
-				msg.Id, len(msg.AddedMembers), msg.AddedMembers[:addedNum],
-				len(msg.RemovedMembers), msg.RemovedMembers[:removedNum])
-		}
-	}
-	return fmt.Sprintf("%v", m.msg)
-}
-
 func (d *InternalDataplane) reportHealth() {
 	if d.config.HealthAggregator != nil {
 		d.config.HealthAggregator.Report(

dataplane-drivers/intdataplane/int_dataplane_test.go → dataplane/linux/int_dataplane_test.go

@@ -21,7 +21,7 @@ import (
 	. "github.com/onsi/gomega"
 
 	"github.com/projectcalico/felix/config"
-	"github.com/projectcalico/felix/dataplane-drivers/intdataplane"
+	"github.com/projectcalico/felix/dataplane/linux"
 	"github.com/projectcalico/felix/ifacemonitor"
 	"github.com/projectcalico/felix/ipsets"
 	"github.com/projectcalico/felix/rules"

dataplane-drivers/windataplane/endpoint_mgr.go → dataplane/windows/endpoint_mgr.go

@@ -20,13 +20,12 @@ import (
 	"errors"
 	"os"
 	"strings"
-	"sync"
 	"time"
 
 	hns "github.com/Microsoft/hcsshim"
 	log "github.com/sirupsen/logrus"
 
-	"github.com/projectcalico/felix/dataplane-drivers/windataplane/policysets"
+	"github.com/projectcalico/felix/dataplane/windows/policysets"
 	"github.com/projectcalico/felix/proto"
 )
 
@@ -65,8 +64,6 @@ type endpointManager struct {
 	// addressToEndpointId serves as a hns endpoint id cache. It enables us to lookup the hns
 	// endpoint id for a given endpoint ip address.
 	addressToEndpointId map[string]string
-	// read-write lock to protect access to the addressToEndpointId map during cache updates.
-	endpointLock sync.RWMutex
 	// lastCacheUpdate records the last time that the addressToEndpointId map was refreshed.
 	lastCacheUpdate time.Time
 }
@@ -100,6 +97,9 @@ func (m *endpointManager) OnUpdate(msg interface{}) {
 	case *proto.WorkloadEndpointRemove:
 		log.WithField("workloadEndpointId", msg.Id).Info("Processing WorkloadEndpointRemove")
 		m.pendingWlEpUpdates[*msg.Id] = nil
+	case *proto.IPSetUpdate:
+		log.WithField("ipSetId", msg.Id).Info("Processing IPSetUpdate")
+		m.ProcessIpSetUpdate(msg.Id)
 	case *proto.IPSetDeltaUpdate:
 		log.WithField("ipSetId", msg.Id).Info("Processing IPSetDeltaUpdate")
 		m.ProcessIpSetUpdate(msg.Id)
@@ -121,9 +121,6 @@ func (m *endpointManager) RefreshHnsEndpointCache(forceRefresh bool) error {
 		return err
 	}
 
-	// lock is taken to pend any read attempts while the cache is being cleared + repopulated
-	m.endpointLock.Lock()
-
 	log.Debug("Clearing the endpoint cache")
 	m.addressToEndpointId = make(map[string]string)
 
@@ -138,8 +135,6 @@ func (m *endpointManager) RefreshHnsEndpointCache(forceRefresh bool) error {
 	log.Infof("Cache refresh is complete. %v endpoints were cached", len(m.addressToEndpointId))
 	m.lastCacheUpdate = time.Now()
 
-	m.endpointLock.Unlock()
-
 	return nil
 }
 
@@ -163,7 +158,6 @@ func (m *endpointManager) ProcessIpSetUpdate(ipSetId string) {
 
 		var activePolicyNames []string
 		if len(workload.Tiers) > 0 {
-			// Todo: Check on this merge approach, as rules also have direction specified
 			activePolicyNames = append(activePolicyNames, workload.Tiers[0].IngressPolicies...)
 			activePolicyNames = append(activePolicyNames, workload.Tiers[0].EgressPolicies...)
 		} else {
@@ -260,8 +254,10 @@ func (m *endpointManager) applyRules(workloadId proto.WorkloadEndpointID, endpoi
 	var rules []*hns.ACLPolicy
 	if len(policyNames) > 0 {
 		rules = m.policysetsDataplane.GetPolicySetRules(policyNames)
-		for _, rule := range rules {
-			logCxt.WithField("rule", rule).Debug("Complete set of rules to be applied")
+		if log.GetLevel() >= log.DebugLevel {
+			for _, rule := range rules {
+				logCxt.WithField("rule", rule).Debug("Complete set of rules to be applied")
+			}
 		}
 	} else {
 		logCxt.Info("No policies/profiles were specified, all rules will be removed from this endpoint")
@@ -287,9 +283,7 @@ func (m *endpointManager) getHnsEndpointId(ip string) (string, error) {
 	allowRefresh := true
 	for {
 		// First check the endpoint cache
-		m.endpointLock.RLock()
 		id, ok := m.addressToEndpointId[ip]
-		m.endpointLock.RUnlock()
 		if ok {
 			log.WithFields(log.Fields{"ip": ip, "id": id}).Info("Resolved hns endpoint id")
 			return id, nil

dataplane-drivers/windataplane/ipsets/ipset_defs.go → dataplane/windows/ipsets/ipset_defs.go

@@ -17,7 +17,7 @@
 package ipsets
 
 import (
-	"github.com/projectcalico/felix/dataplane-drivers/windataplane/set"
+	"github.com/projectcalico/libcalico-go/lib/set"
 )
 
 // IPSetMetadata contains the metadata for a particular IP set, such as its name and type.