Service / LoadBalancer synchronization fixes #1352

ianvernon · 2017-08-23T01:54:41Z

common/types: add String() function for LBBackend & L3n4AddrID, add Slave
member to L3n4AddrID, add debug logs throughout, and update LBSVC object
with its SHA256 hash.
daemon: clean all LB, RevNAT maps on host when restore mode is not enabled.
Add debug logs in the service-related code. When a service is updated or
deleted, clean BPF LB maps of old map entries for the given service. Fix
logic for syncing Cilium's LB maps with the BPF maps on disk when Cilium is
restarted and restore mode is enabled.
pkg/maps/lbmap: add logs when services or RevNAT entries change state.
tests: refactor 06-lb.sh into functions to reduce duplicate code, and add
test that ensures restore mode works appropriately for LB Maps when Cilium
is restarted.

Signed-off by: Ian Vernon [email protected]
Fixes #1295

ianvernon · 2017-08-24T15:44:19Z

Hitting an issue in the K8s multinode tests which I haven't seen before, so I'm removing this from pending-review until I can get those tests passing.

aanm

OOOI - Out Of Order Imports

aanm · 2017-08-28T19:44:41Z

common/types/loadbalancer.go

@@ -47,6 +48,10 @@ type LBBackEnd struct {
 	Weight uint16
 }

+func (lbbe *LBBackEnd) String() string {
+	return fmt.Sprintf("%s, weight: %d", lbbe.L3n4Addr.String(), lbbe.Weight)


Isn't there a String() already for this? I was 80% sure there was.

Since LBBackEnd is embedded with an L3n4Addr, I guess we could call String() on it, and it would use the L3n4Addr.String() function, but we wouldn't get the value for the weight for the LBBackEnd.

I mean for LBBackend. It's fine now

aanm · 2017-08-28T19:54:55Z

daemon/loadbalancer.go

+	log.Debugf("adding service %d to BPF maps", feCilium.ID)
+
+	// Try to delete service before adding it and ignore errors as it might not exist.
+	_ = d.svcDeleteByFrontendLocked(&feCilium.L3n4Addr)


at least run the erros in debug mode.

aanm · 2017-08-28T19:57:24Z

daemon/loadbalancer.go

+	numBackends := uint16(vval.GetCount())
+
+	// ServiceKeys are unique by their slave number, which corresponds to the number of backends. Delete each of these.
+	for i := numBackends; i > 0; i-- {


Do you need to delete all map entries?

If we're deleting the service, we should clean all of the entries out of the BPF map. Why would we not want to delete all of the entries?

aanm · 2017-08-28T19:58:55Z

daemon/loadbalancer.go

-		if err := lbmap.DeleteService(svcKey); err != nil {
+	// Clean services and rev nats from BPF maps that failed to be restored.
+	for _, svc := range failedSyncSVC {
+		log.Debugf("Unable to restore, so rmoving service: %s", svc.FE)


typo s/rmoving/removing

aanm · 2017-08-28T20:00:32Z

pkg/maps/lbmap/lbmap.go

@@ -19,6 +19,7 @@ import (
 	"net"
 	"unsafe"

+	log "github.com/Sirupsen/logrus"


aanm · 2017-08-28T20:00:55Z

pkg/maps/lbmap/lbmap.go

@@ -361,17 +368,18 @@ func AddSVC2BPFMap(fe ServiceKey, besValues []ServiceValue, addRevNAT bool, revN

 // L3n4Addr2ServiceKey converts the given l3n4Addr to a ServiceKey with the slave ID
 // set to 0.


Update function's comment.

Dismissing review - I have addressed the comments and have requested another review.

tgraf · 2017-08-30T15:42:05Z

common/types/loadbalancer.go

@@ -372,7 +391,12 @@ func (a *L3n4Addr) IsIPv6() bool {
 // KVStore.
 type L3n4AddrID struct {
 	L3n4Addr
-	ID ServiceID
+	ID    ServiceID
+	Slave uint16 // Number of slaves associated with this L3n4AddrID


This changes the structure used in the kvstore. Is this safe? What will happen if we upgrade Cilium and the kvstore contains old services entries?

You're right. This actually isn't used anywhere in the cilium-agent code, so I'm going to revert it. I figured it would be nice to have to add it now and then actually implement it at a later date, but if it's going to mess with the kv-store, I don't think it's worth it at this time. I'll have this removed in the next upload.

tgraf · 2017-08-30T15:45:42Z

daemon/loadbalancer.go

+// Deletes a service by the frontend address
+func (d *Daemon) svcDeleteByFrontend(frontend *types.L3n4Addr) error {
+	d.loadBalancer.BPFMapMU.Lock()
+	defer d.loadBalancer.BPFMapMU.Unlock()


This is confusing, can't we take this lock in the loadbalancer itself? Is this lock protecting the data structures in the daemon as well?

I made this function to give flexibility when we need to delete a service by its L3n4Addr depending on if we have already acquired BPFMapMU. This is similar to other functions throughout the daemon. Can we sync up over video chat about this? It would be easier to explain that way.

* common/types: add String() function for LBBackend, add debug logs throughout, and update LBSVC object with its SHA256 hash. * daemon: clean all LB, RevNAT maps on host when restore mode is not enabled. Add debug logs in the service-related code. When a service is updated or deleted, clean BPF LB maps of old map entries for the given service. Fix logic for syncing Cilium's LB maps with the BPF maps on disk when Cilium is restarted and restore mode is enabled. * pkg/maps/lbmap: add logs when services or RevNAT entries change state. * tests: refactor 06-lb.sh into functions to reduce duplicate code, and add test that ensures restore mode works appropriately for LB Maps when Cilium is restarted. Signed-off by: Ian Vernon <[email protected]>

ianvernon · 2017-09-11T18:13:06Z

Please review.

I have addressed comments and have added more changes to the code since the previous review.

aanm

Can we check if we actually need all debug messages?

aanm · 2017-09-14T07:59:06Z

common/types/loadbalancer.go

@@ -47,6 +48,10 @@ type LBBackEnd struct {
 	Weight uint16
 }

+func (lbbe *LBBackEnd) String() string {
+	return fmt.Sprintf("%s, weight: %d", lbbe.L3n4Addr.String(), lbbe.Weight)


I mean for LBBackend. It's fine now

aanm · 2017-09-14T07:59:32Z

common/types/loadbalancer.go

 		delete(lb.SVCMap, oldSvc.Sha256)
 	}
+	log.Debugf("adding service %s with SHA %s to lb.SVCMap", svc.FE.String(), svc.Sha256)


Do we still need all this debug messages?

I found that the debug messages I added were useful in determining what was going on in this code. There were very few logs when I started out with this branch in the service code. I don't see a downside to having this log - it provides important information about the state managed in Cilium.

aanm · 2017-09-14T07:59:46Z

common/types/loadbalancer.go

-	return &L3n4Addr{IP: ip, L4Addr: *lbport}, nil
+
+	addr := L3n4Addr{IP: ip, L4Addr: *lbport}
+	log.Debugf("created new L3n4Addr %s", addr)


here as well

I don't see the disadvantage of having more debug logs - see my comments above.

Global NodePort services are not supported when they are managed by iptables/ipvs, since they do not know about remote endpoints. Hence, let's skip the related connectivity tests when running in multicluster mode and KPR NodePort support is disabled, to prevent spurious failures. Related: cilium#23128 Related: cilium#23266 Fixes: cilium#1352 Signed-off-by: Marco Iorio <[email protected]>

ianvernon added the wip label Aug 23, 2017

ianvernon force-pushed the fix-service-delete branch 3 times, most recently from f26798a to 5197676 Compare August 23, 2017 21:37

ianvernon changed the title ~~WIP: fixing cilium service delete~~ Service / LoadBalancer synchronization fixes Aug 23, 2017

ianvernon added pending-review and removed wip labels Aug 23, 2017

ianvernon requested review from tgraf and aanm August 23, 2017 21:42

ianvernon added the kind/bug This is a bug in the Cilium logic. label Aug 23, 2017

ianvernon force-pushed the fix-service-delete branch from 5197676 to 42fd6fb Compare August 23, 2017 22:38

ianvernon added wip and removed pending-review labels Aug 24, 2017

ianvernon force-pushed the fix-service-delete branch 4 times, most recently from cd07c03 to 2ae19fc Compare August 28, 2017 13:46

ianvernon mentioned this pull request Aug 28, 2017

Test flake in k8s-multi-node-tests #1407

Closed

aanm previously requested changes Aug 28, 2017

View reviewed changes

ianvernon force-pushed the fix-service-delete branch from 2ae19fc to 7c2e074 Compare August 28, 2017 20:37

ianvernon requested a review from aanm August 28, 2017 20:38

ianvernon force-pushed the fix-service-delete branch 2 times, most recently from 51bf723 to 1a45982 Compare August 29, 2017 20:09

ianvernon added pending-review and removed wip labels Aug 29, 2017

tgraf previously requested changes Aug 30, 2017

View reviewed changes

ianvernon force-pushed the fix-service-delete branch 2 times, most recently from 891aa36 to d0fd73e Compare September 1, 2017 17:35

ianvernon force-pushed the fix-service-delete branch from d0fd73e to 81df82f Compare September 5, 2017 22:38

ianvernon added wip and removed pending-review labels Sep 6, 2017

ianvernon force-pushed the fix-service-delete branch 8 times, most recently from ee2d9e6 to 4e964c5 Compare September 11, 2017 16:34

ianvernon force-pushed the fix-service-delete branch from 4e964c5 to 216d5a6 Compare September 11, 2017 16:44

ianvernon requested a review from tgraf September 11, 2017 18:12

cilium deleted a comment from houndci-bot Sep 11, 2017

ianvernon added pending-review and removed wip labels Sep 11, 2017

aanm reviewed Sep 14, 2017

View reviewed changes

tgraf approved these changes Sep 14, 2017

View reviewed changes

tgraf merged commit 31a88cf into master Sep 14, 2017

tgraf deleted the fix-service-delete branch September 14, 2017 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service / LoadBalancer synchronization fixes #1352

Service / LoadBalancer synchronization fixes #1352

ianvernon commented Aug 23, 2017 •

edited

Loading

ianvernon commented Aug 24, 2017

aanm left a comment

aanm Aug 28, 2017

ianvernon Aug 28, 2017

aanm Sep 14, 2017

aanm Aug 28, 2017

ianvernon Aug 28, 2017

aanm Aug 28, 2017

ianvernon Aug 28, 2017

aanm Aug 28, 2017

ianvernon Aug 28, 2017

aanm Aug 28, 2017

ianvernon Aug 28, 2017

aanm Aug 28, 2017

ianvernon Aug 28, 2017

tgraf Aug 30, 2017

ianvernon Aug 30, 2017

tgraf Aug 30, 2017

ianvernon Aug 30, 2017

ianvernon commented Sep 11, 2017

aanm left a comment

aanm Sep 14, 2017

aanm Sep 14, 2017

ianvernon Sep 14, 2017 •

edited

Loading

aanm Sep 14, 2017

ianvernon Sep 14, 2017

		@@ -361,17 +368,18 @@ func AddSVC2BPFMap(fe ServiceKey, besValues []ServiceValue, addRevNAT bool, revN

		// L3n4Addr2ServiceKey converts the given l3n4Addr to a ServiceKey with the slave ID
		// set to 0.

Service / LoadBalancer synchronization fixes #1352

Service / LoadBalancer synchronization fixes #1352

Conversation

ianvernon commented Aug 23, 2017 • edited Loading

ianvernon commented Aug 24, 2017

aanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon commented Sep 11, 2017

aanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon commented Aug 23, 2017 •

edited

Loading

ianvernon Sep 14, 2017 •

edited

Loading