From 4401ccac2202eda7f28df26d23f4ac78570cb07a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 16 Aug 2023 20:17:35 +0200 Subject: [PATCH 1/4] libnetwork: Sandbox: remove some intermediate vars - remove some intermediate vars, or move them closer to where they're used. - ResolveService: use strings.SplitN to limit number of elements. This code is only used to validate the input, results are not used. - ResolveService: return early instead of breaking the loop. This makes it clearer from the code that were not returning anything (nil, nil). - Controller.sandboxCleanup(): rename a var, and slight refactor of error-handling. Signed-off-by: Sebastiaan van Stijn --- libnetwork/sandbox.go | 12 ++++------ libnetwork/sandbox_store.go | 48 +++++++++++++++++++++---------------- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/libnetwork/sandbox.go b/libnetwork/sandbox.go index e53aa7d35f..bf1bdf4fc6 100644 --- a/libnetwork/sandbox.go +++ b/libnetwork/sandbox.go @@ -369,28 +369,24 @@ func (sb *Sandbox) ResolveIP(ip string) string { // ResolveService returns all the backend details about the containers or hosts // backing a service. Its purpose is to satisfy an SRV query. func (sb *Sandbox) ResolveService(name string) ([]*net.SRV, []net.IP) { - srv := []*net.SRV{} - ip := []net.IP{} - log.G(context.TODO()).Debugf("Service name To resolve: %v", name) // There are DNS implementations that allow SRV queries for names not in // the format defined by RFC 2782. Hence specific validations checks are // not done - parts := strings.Split(name, ".") - if len(parts) < 3 { + if parts := strings.SplitN(name, ".", 3); len(parts) < 3 { return nil, nil } for _, ep := range sb.Endpoints() { n := ep.getNetwork() - srv, ip = n.ResolveService(name) + srv, ip := n.ResolveService(name) if len(srv) > 0 { - break + return srv, ip } } - return srv, ip + return nil, nil } func getDynamicNwEndpoints(epList []*Endpoint) []*Endpoint { diff --git a/libnetwork/sandbox_store.go b/libnetwork/sandbox_store.go index 08cee95668..cc138e2273 100644 --- a/libnetwork/sandbox_store.go +++ b/libnetwork/sandbox_store.go @@ -143,12 +143,10 @@ retry: continue } - eps := epState{ + sbs.Eps = append(sbs.Eps, epState{ Nid: ep.getNetwork().ID(), Eid: ep.ID(), - } - - sbs.Eps = append(sbs.Eps, eps) + }) } err := sb.controller.updateToStore(sbs) @@ -164,15 +162,13 @@ retry: } func (sb *Sandbox) storeDelete() error { - sbs := &sbState{ + return sb.controller.deleteFromStore(&sbState{ c: sb.controller, ID: sb.id, Cid: sb.containerID, dbIndex: sb.dbIndex, dbExists: sb.dbExists, - } - - return sb.controller.deleteFromStore(sbs) + }) } func (c *Controller) sandboxCleanup(activeSandboxes map[string]interface{}) { @@ -182,20 +178,18 @@ func (c *Controller) sandboxCleanup(activeSandboxes map[string]interface{}) { return } - kvol, err := store.List(datastore.Key(sandboxPrefix), &sbState{c: c}) - if err != nil && err != datastore.ErrKeyNotFound { + sandboxStates, err := store.List(datastore.Key(sandboxPrefix), &sbState{c: c}) + if err != nil { + if err == datastore.ErrKeyNotFound { + // It's normal for no sandboxes to be found. Just bail out. + return + } log.G(context.TODO()).Errorf("failed to get sandboxes for scope %s: %v", store.Scope(), err) return } - // It's normal for no sandboxes to be found. Just bail out. - if err == datastore.ErrKeyNotFound { - return - } - - for _, kvo := range kvol { - sbs := kvo.(*sbState) - + for _, s := range sandboxStates { + sbs := s.(*sbState) sb := &Sandbox{ id: sbs.ID, controller: sbs.c, @@ -235,13 +229,25 @@ func (c *Controller) sandboxCleanup(activeSandboxes map[string]interface{}) { var ep *Endpoint if err != nil { log.G(context.TODO()).Errorf("getNetworkFromStore for nid %s failed while trying to build sandbox for cleanup: %v", eps.Nid, err) - n = &Network{id: eps.Nid, ctrlr: c, drvOnce: &sync.Once{}, persist: true} - ep = &Endpoint{id: eps.Eid, network: n, sandboxID: sbs.ID} + ep = &Endpoint{ + id: eps.Eid, + network: &Network{ + id: eps.Nid, + ctrlr: c, + drvOnce: &sync.Once{}, + persist: true, + }, + sandboxID: sbs.ID, + } } else { ep, err = n.getEndpointFromStore(eps.Eid) if err != nil { log.G(context.TODO()).Errorf("getEndpointFromStore for eid %s failed while trying to build sandbox for cleanup: %v", eps.Eid, err) - ep = &Endpoint{id: eps.Eid, network: n, sandboxID: sbs.ID} + ep = &Endpoint{ + id: eps.Eid, + network: n, + sandboxID: sbs.ID, + } } } if _, ok := activeSandboxes[sb.ID()]; ok && err != nil { From 9249b34be8d5419f2f677e4a5426369b6a4716b2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 17 Aug 2023 12:43:34 +0200 Subject: [PATCH 2/4] libnetwork: Sandbox.resolveName: rename vars for clarity - use `nameOrAlias` for the name (or alias) to resolve - use `lookupAlias` to indicate what the intent is; this function is either looking up aliases or "regular" names. Ideally we would split the function, but let's keep that for a future exercise. - name the `ipv6Miss` output variable. The "ipv6 miss" logic is rather confusing, and should probably be revisited, but let's start with giving the variable a name to make it more apparent what it is. - use `nw` for networks, which is the more common local name Signed-off-by: Sebastiaan van Stijn --- libnetwork/sandbox.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/libnetwork/sandbox.go b/libnetwork/sandbox.go index bf1bdf4fc6..52627da8d0 100644 --- a/libnetwork/sandbox.go +++ b/libnetwork/sandbox.go @@ -488,25 +488,23 @@ func (sb *Sandbox) ResolveName(name string, ipType int) ([]net.IP, bool) { return nil, false } -func (sb *Sandbox) resolveName(req string, networkName string, epList []*Endpoint, alias bool, ipType int) ([]net.IP, bool) { - var ipv6Miss bool - +func (sb *Sandbox) resolveName(nameOrAlias string, networkName string, epList []*Endpoint, lookupAlias bool, ipType int) (_ []net.IP, ipv6Miss bool) { for _, ep := range epList { - name := req - n := ep.getNetwork() + name := nameOrAlias + nw := ep.getNetwork() - if networkName != "" && networkName != n.Name() { + if networkName != "" && networkName != nw.Name() { continue } - if alias { + if lookupAlias { if ep.aliases == nil { continue } var ok bool ep.mu.Lock() - name, ok = ep.aliases[req] + name, ok = ep.aliases[nameOrAlias] ep.mu.Unlock() if !ok { continue @@ -515,19 +513,17 @@ func (sb *Sandbox) resolveName(req string, networkName string, epList []*Endpoin // If it is a regular lookup and if the requested name is an alias // don't perform a svc lookup for this endpoint. ep.mu.Lock() - if _, ok := ep.aliases[req]; ok { + if _, ok := ep.aliases[nameOrAlias]; ok { ep.mu.Unlock() continue } ep.mu.Unlock() } - ip, miss := n.ResolveName(name, ipType) - + ip, miss := nw.ResolveName(name, ipType) if ip != nil { return ip, false } - if miss { ipv6Miss = miss } From f549aaa20589c82737c93a5297b5416487a588ae Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 17 Aug 2023 12:48:37 +0200 Subject: [PATCH 3/4] libnetwork: Sandbox.resolveName: add fast-path for alias lookups Skip faster when we're looking for aliases. Also check for the list of aliases to be empty, not just `nil` (although in practice it should be equivalent). Signed-off-by: Sebastiaan van Stijn --- libnetwork/sandbox.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/libnetwork/sandbox.go b/libnetwork/sandbox.go index 52627da8d0..1e5714d136 100644 --- a/libnetwork/sandbox.go +++ b/libnetwork/sandbox.go @@ -490,18 +490,17 @@ func (sb *Sandbox) ResolveName(name string, ipType int) ([]net.IP, bool) { func (sb *Sandbox) resolveName(nameOrAlias string, networkName string, epList []*Endpoint, lookupAlias bool, ipType int) (_ []net.IP, ipv6Miss bool) { for _, ep := range epList { - name := nameOrAlias - nw := ep.getNetwork() + if lookupAlias && len(ep.aliases) == 0 { + continue + } + nw := ep.getNetwork() if networkName != "" && networkName != nw.Name() { continue } + name := nameOrAlias if lookupAlias { - if ep.aliases == nil { - continue - } - var ok bool ep.mu.Lock() name, ok = ep.aliases[nameOrAlias] From d7a31cfb2d662f347f3cb220685ea814b917b3fa Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 17 Aug 2023 12:50:54 +0200 Subject: [PATCH 4/4] libnetwork: Sandbox.resolveName: slightly simplify locking Simplify the lock/unlock cycle, and make the "lookupAlias" branch more similar to the non-lookupAlias variant. Signed-off-by: Sebastiaan van Stijn --- libnetwork/sandbox.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libnetwork/sandbox.go b/libnetwork/sandbox.go index 1e5714d136..8e4c008ff1 100644 --- a/libnetwork/sandbox.go +++ b/libnetwork/sandbox.go @@ -501,22 +501,22 @@ func (sb *Sandbox) resolveName(nameOrAlias string, networkName string, epList [] name := nameOrAlias if lookupAlias { - var ok bool ep.mu.Lock() - name, ok = ep.aliases[nameOrAlias] + alias, ok := ep.aliases[nameOrAlias] ep.mu.Unlock() if !ok { continue } + name = alias } else { // If it is a regular lookup and if the requested name is an alias // don't perform a svc lookup for this endpoint. ep.mu.Lock() - if _, ok := ep.aliases[nameOrAlias]; ok { - ep.mu.Unlock() + _, ok := ep.aliases[nameOrAlias] + ep.mu.Unlock() + if ok { continue } - ep.mu.Unlock() } ip, miss := nw.ResolveName(name, ipType)