diff --git a/pkg/haproxy/config.go b/pkg/haproxy/config.go index 0f1ee06b6..5b19436d8 100644 --- a/pkg/haproxy/config.go +++ b/pkg/haproxy/config.go @@ -40,6 +40,7 @@ type Config interface { Backends() *hatypes.Backends Userlists() *hatypes.Userlists Clear() + Shrink() Commit() } @@ -360,6 +361,11 @@ func (c *config) Clear() { *c = *config } +func (c *config) Shrink() { + c.hosts.Shrink() + c.backends.Shrink() +} + func (c *config) Commit() { if !reflect.DeepEqual(c.globalOld, c.global) { // globals still uses the old deepCopy+fullParsing+deepEqual strategy diff --git a/pkg/haproxy/instance.go b/pkg/haproxy/instance.go index 42ec95b9d..28c91dcac 100644 --- a/pkg/haproxy/instance.go +++ b/pkg/haproxy/instance.go @@ -21,6 +21,7 @@ import ( "os/exec" "path/filepath" "regexp" + "sort" "strconv" "strings" @@ -238,6 +239,7 @@ func (i *instance) haproxyUpdate(timer *utils.Timer) { // defer i.config.Commit() i.config.SyncConfig() + i.config.Shrink() if err := i.config.WriteFrontendMaps(); err != nil { i.logger.Error("error building frontend maps: %v", err) i.metrics.IncUpdateNoop() @@ -248,9 +250,13 @@ func (i *instance) haproxyUpdate(timer *utils.Timer) { i.metrics.IncUpdateNoop() return } + timer.Tick("write_maps") + if i.options.HAProxyCmd != "" { + // TODO update tests and remove `if cmd!=""` above + i.logChanged() + } updater := i.newDynUpdater() updated := updater.update() - timer.Tick("write_maps") if !updated || updater.cmdCnt > 0 { // only need to rewrtite config files if: // - !updated - there are changes that cannot be dynamically applied @@ -294,6 +300,43 @@ func (i *instance) haproxyUpdate(timer *utils.Timer) { timer.Tick("reload_haproxy") } +func (i *instance) logChanged() { + hostsAdd := i.config.Hosts().ItemsAdd() + if len(hostsAdd) < 100 { + hostsDel := i.config.Hosts().ItemsDel() + hosts := make([]string, 0, len(hostsAdd)) + for host := range hostsAdd { + hosts = append(hosts, host) + } + for host := range hostsDel { + if _, found := hostsAdd[host]; !found { + hosts = append(hosts, host) + } + } + sort.Strings(hosts) + i.logger.InfoV(2, "updating %d host(s): %v", len(hosts), hosts) + } else { + i.logger.InfoV(2, "updating %d hosts", len(hostsAdd)) + } + backsAdd := i.config.Backends().ItemsAdd() + if len(backsAdd) < 100 { + backsDel := i.config.Backends().ItemsDel() + backs := make([]string, 0, len(backsAdd)) + for back := range backsAdd { + backs = append(backs, back) + } + for back := range backsDel { + if _, found := backsAdd[back]; !found { + backs = append(backs, back) + } + } + sort.Strings(backs) + i.logger.InfoV(2, "updating %d backend(s): %v", len(backs), backs) + } else { + i.logger.InfoV(2, "updating %d backends", len(backsAdd)) + } +} + func (i *instance) writeConfig() (err error) { // // modsec template execution diff --git a/pkg/haproxy/types/backends.go b/pkg/haproxy/types/backends.go index 8d7d882c2..115868e80 100644 --- a/pkg/haproxy/types/backends.go +++ b/pkg/haproxy/types/backends.go @@ -18,6 +18,7 @@ package types import ( "crypto/md5" + "reflect" "sort" ) @@ -51,6 +52,74 @@ func (b *Backends) ItemsDel() map[string]*Backend { return b.itemsDel } +// Shrink compares deleted and added backends with the same name - ie changed +// objects - and remove both from the changing hashmap tracker when they match. +func (b *Backends) Shrink() { + changed := false + for name, del := range b.itemsDel { + if add, found := b.itemsAdd[name]; found { + if backendsMatch(add, del) { + // Such changed backend, when removed from the tracking, need to + // be reincluded into the current state hashmap `items` and also + // into its shard hashmap when backend sharding is enabled. + if len(b.shards) > 0 { + b.shards[del.shard][del.ID] = del + } + b.items[name] = del + delete(b.itemsAdd, name) + delete(b.itemsDel, name) + changed = true + } + } + } + // Backends removed from the changing tracker might clean a shard state if it + // was the only one changed into the shard. Recalc changedShards if anything + // was changed. + if changed { + b.changedShards = map[int]bool{} + for _, back := range b.itemsAdd { + b.changedShards[back.shard] = true + } + for _, back := range b.itemsDel { + b.changedShards[back.shard] = true + } + } +} + +// backendsMatch returns true if two backends match. This comparison +// ignores empty endpoints and its order and it's cheaper than leave +// the backend dirty. +func backendsMatch(back1, back2 *Backend) bool { + if reflect.DeepEqual(back1, back2) { + return true + } + b1copy := *back1 + b1copy.Endpoints = back2.Endpoints + if !reflect.DeepEqual(&b1copy, back2) { + return false + } + epmap := make(map[Endpoint]bool, len(back1.Endpoints)) + for _, ep := range back1.Endpoints { + if !ep.IsEmpty() { + epmap[*ep] = false + } + } + for _, ep := range back2.Endpoints { + if !ep.IsEmpty() { + if _, found := epmap[*ep]; !found { + return false + } + epmap[*ep] = true + } + } + for _, found := range epmap { + if !found { + return false + } + } + return true +} + // Commit ... func (b *Backends) Commit() { b.itemsAdd = map[string]*Backend{} diff --git a/pkg/haproxy/types/backends_test.go b/pkg/haproxy/types/backends_test.go index a1380531a..d195803ce 100644 --- a/pkg/haproxy/types/backends_test.go +++ b/pkg/haproxy/types/backends_test.go @@ -128,6 +128,155 @@ func TestBackendCrud(t *testing.T) { } } +func TestShrinkBackends(t *testing.T) { + ep0 := &Endpoint{IP: "127.0.0.1"} + ep11 := &Endpoint{IP: "192.168.0.11"} + ep21 := &Endpoint{IP: "192.168.0.21"} + app11 := &Backend{Name: "default_app1_8080", Endpoints: []*Endpoint{ep11}} + app12 := &Backend{Name: "default_app1_8080", Endpoints: []*Endpoint{ep11, ep0}} + app21 := &Backend{Name: "default_app2_8080", Endpoints: []*Endpoint{ep21}} + testCases := []struct { + add, del []*Backend + expAdd, expDel []*Backend + }{ + // 0 + {}, + // 1 + { + add: []*Backend{app11}, + expAdd: []*Backend{app11}, + }, + // 2 + { + add: []*Backend{app11}, + del: []*Backend{app11}, + }, + // 3 + { + add: []*Backend{app11, app21}, + del: []*Backend{app21}, + expAdd: []*Backend{app11}, + }, + // 4 + { + add: []*Backend{app11}, + del: []*Backend{app11, app21}, + expDel: []*Backend{app21}, + }, + // 5 + { + add: []*Backend{app11}, + del: []*Backend{app12}, + }, + } + for i, test := range testCases { + c := setup(t) + b := CreateBackends(0) + for _, add := range test.add { + b.itemsAdd[add.Name] = add + } + for _, del := range test.del { + b.itemsDel[del.Name] = del + } + expAdd := map[string]*Backend{} + for _, add := range test.expAdd { + expAdd[add.Name] = add + } + expDel := map[string]*Backend{} + for _, del := range test.expDel { + expDel[del.Name] = del + } + b.Shrink() + c.compareObjects("add", i, b.itemsAdd, expAdd) + c.compareObjects("del", i, b.itemsDel, expDel) + c.teardown() + } +} + +func TestBackendsMatch(t *testing.T) { + ep0_1 := &Endpoint{IP: "127.0.0.1"} + ep0_2 := &Endpoint{IP: "127.0.0.1"} + ep1_1 := &Endpoint{IP: "192.168.0.1"} + ep1_2 := &Endpoint{IP: "192.168.0.1"} + ep2_1 := &Endpoint{IP: "192.168.0.2"} + ep2_2 := &Endpoint{IP: "192.168.0.2"} + testCases := []struct { + back1, back2 *Backend + expected bool + }{ + // 0 + { + expected: true, + }, + // 1 + { + back1: &Backend{}, + back2: &Backend{}, + expected: true, + }, + // 2 + { + back1: &Backend{Endpoints: []*Endpoint{ep0_1}}, + back2: &Backend{Endpoints: []*Endpoint{ep0_2}}, + expected: true, + }, + // 3 + { + back1: &Backend{CustomConfig: []string{"http-request"}, Endpoints: []*Endpoint{ep0_1}}, + back2: &Backend{CustomConfig: []string{"http-response"}, Endpoints: []*Endpoint{ep0_2}}, + expected: false, + }, + // 4 + { + back1: &Backend{Endpoints: []*Endpoint{ep0_1, ep1_1}}, + back2: &Backend{Endpoints: []*Endpoint{ep0_2, ep1_2}}, + expected: true, + }, + // 5 + { + back1: &Backend{Endpoints: []*Endpoint{ep0_1, ep1_1}}, + back2: &Backend{Endpoints: []*Endpoint{ep0_2, ep2_2}}, + expected: false, + }, + // 6 + { + back1: &Backend{Endpoints: []*Endpoint{ep0_1, ep1_1, ep2_1}}, + back2: &Backend{Endpoints: []*Endpoint{ep0_2, ep2_2, ep1_2}}, + expected: true, + }, + // 7 + { + back1: &Backend{Endpoints: []*Endpoint{ep2_1, ep1_1}}, + back2: &Backend{Endpoints: []*Endpoint{ep1_2, ep2_2}}, + expected: true, + }, + // 8 + { + back1: &Backend{Endpoints: []*Endpoint{ep2_1, ep0_1, ep1_1}}, + back2: &Backend{Endpoints: []*Endpoint{ep1_2, ep2_2, ep0_2}}, + expected: true, + }, + // 9 + { + back1: &Backend{Endpoints: []*Endpoint{ep2_1, ep0_1}}, + back2: &Backend{Endpoints: []*Endpoint{ep1_2, ep2_2, ep0_2}}, + expected: false, + }, + // 10 + { + back1: &Backend{Endpoints: []*Endpoint{ep2_1, ep0_1, ep1_1}}, + back2: &Backend{Endpoints: []*Endpoint{ep1_2, ep0_2}}, + expected: false, + }, + } + for i, test := range testCases { + c := setup(t) + result := backendsMatch(test.back1, test.back2) + c.compareObjects("match", i, result, test.expected) + c.teardown() + } +} + func TestBuildID(t *testing.T) { testCases := []struct { namespace string diff --git a/pkg/haproxy/types/host.go b/pkg/haproxy/types/host.go index 494eac5df..2be804a9b 100644 --- a/pkg/haproxy/types/host.go +++ b/pkg/haproxy/types/host.go @@ -64,6 +64,21 @@ func (h *Hosts) RemoveAll(hostnames []string) { } } +// Shrink removes matching added and deleted hosts from the changing hashmap +// tracker that has the same content. A matching added+deleted pair means +// that a hostname was reparsed but its content wasn't changed. +func (h *Hosts) Shrink() { + for name, del := range h.itemsDel { + if add, found := h.itemsAdd[name]; found { + if reflect.DeepEqual(add, del) { + h.items[name] = del + delete(h.itemsAdd, name) + delete(h.itemsDel, name) + } + } + } +} + // Commit ... func (h *Hosts) Commit() { h.itemsAdd = map[string]*Host{} @@ -72,7 +87,7 @@ func (h *Hosts) Commit() { // Changed ... func (h *Hosts) Changed() bool { - return !reflect.DeepEqual(h.itemsAdd, h.itemsDel) + return len(h.itemsAdd) > 0 || len(h.itemsDel) > 0 } func (h *Hosts) createHost(hostname string) *Host { diff --git a/pkg/haproxy/types/host_test.go b/pkg/haproxy/types/host_test.go new file mode 100644 index 000000000..372ca4b96 --- /dev/null +++ b/pkg/haproxy/types/host_test.go @@ -0,0 +1,77 @@ +/* +Copyright 2020 The HAProxy Ingress Controller Authors. + +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 types + +import ( + "testing" +) + +func TestShrinkHosts(t *testing.T) { + app1 := &Host{Hostname: "app1.localdomain"} + app2 := &Host{Hostname: "app2.localdomain"} + testCases := []struct { + add, del []*Host + expAdd, expDel []*Host + }{ + // 0 + {}, + // 1 + { + add: []*Host{app1}, + expAdd: []*Host{app1}, + }, + // 2 + { + add: []*Host{app1}, + del: []*Host{app1}, + }, + // 3 + { + add: []*Host{app1, app2}, + del: []*Host{app2}, + expAdd: []*Host{app1}, + }, + // 4 + { + add: []*Host{app1}, + del: []*Host{app1, app2}, + expDel: []*Host{app2}, + }, + } + for i, test := range testCases { + c := setup(t) + h := CreateHosts() + for _, add := range test.add { + h.itemsAdd[add.Hostname] = add + } + for _, del := range test.del { + h.itemsDel[del.Hostname] = del + } + expAdd := map[string]*Host{} + for _, add := range test.expAdd { + expAdd[add.Hostname] = add + } + expDel := map[string]*Host{} + for _, del := range test.expDel { + expDel[del.Hostname] = del + } + h.Shrink() + c.compareObjects("add", i, h.itemsAdd, expAdd) + c.compareObjects("del", i, h.itemsDel, expDel) + c.teardown() + } +}