diff --git a/controllers/nginx/pkg/cmd/controller/nginx.go b/controllers/nginx/pkg/cmd/controller/nginx.go index 133bc9a5516..2291a73066a 100644 --- a/controllers/nginx/pkg/cmd/controller/nginx.go +++ b/controllers/nginx/pkg/cmd/controller/nginx.go @@ -235,23 +235,6 @@ func (n *NGINXController) start(cmd *exec.Cmd, done chan error) { }() } -// Reload checks if the running configuration file is different -// to the specified and reload nginx if required -func (n NGINXController) Reload(data []byte) ([]byte, bool, error) { - if !n.isReloadRequired(data) { - return []byte("Reload not required"), false, nil - } - - err := ioutil.WriteFile(cfgPath, data, 0644) - if err != nil { - return nil, false, err - } - - o, e := exec.Command(n.binary, "-s", "reload", "-c", cfgPath).CombinedOutput() - - return o, true, e -} - // BackendDefaults returns the nginx defaults func (n NGINXController) BackendDefaults() defaults.Backend { if n.configmap == nil { @@ -262,36 +245,35 @@ func (n NGINXController) BackendDefaults() defaults.Backend { return ngx_template.ReadConfig(n.configmap.Data).Backend } -// isReloadRequired check if the new configuration file is different -// from the current one. -func (n NGINXController) isReloadRequired(data []byte) bool { +// printDiff returns the difference between the running configuration +// and the new one +func (n NGINXController) printDiff(data []byte) { in, err := os.Open(cfgPath) if err != nil { - return false + return } src, err := ioutil.ReadAll(in) in.Close() if err != nil { - return false + return } if !bytes.Equal(src, data) { - tmpfile, err := ioutil.TempFile("", "nginx-cfg-diff") if err != nil { glog.Errorf("error creating temporal file: %s", err) - return false + return } defer tmpfile.Close() err = ioutil.WriteFile(tmpfile.Name(), data, 0644) if err != nil { - return false + return } diffOutput, err := diff(src, data) if err != nil { glog.Errorf("error computing diff: %s", err) - return true + return } if glog.V(2) { @@ -299,9 +281,7 @@ func (n NGINXController) isReloadRequired(data []byte) bool { glog.Infof("%v", string(diffOutput)) } os.Remove(tmpfile.Name()) - return len(diffOutput) > 0 } - return false } // Info return build information @@ -404,7 +384,7 @@ func (n *NGINXController) SetListers(lister ingress.StoreLister) { // write the configuration file // returning nill implies the backend will be reloaded. // if an error is returned means requeue the update -func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) ([]byte, error) { +func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { var longestName int var serverNameBytes int for _, srv := range ingressCfg.Servers { @@ -540,15 +520,29 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) ([]byte, er Cfg: cfg, IsIPV6Enabled: n.isIPV6Enabled && !cfg.DisableIpv6, }) + + if err != nil { + return err + } + + err = n.testTemplate(content) + if err != nil { + return err + } + + n.printDiff(content) + + err = ioutil.WriteFile(cfgPath, content, 0644) if err != nil { - return nil, err + return err } - if err := n.testTemplate(content); err != nil { - return nil, err + o, e := exec.Command(n.binary, "-s", "reload", "-c", cfgPath).CombinedOutput() + if err != nil { + return fmt.Errorf("%v\n%v", e, string(o)) } - return content, nil + return nil } // nginxHashBucketSize computes the correct nginx hash_bucket_size for a hash with the given longest key diff --git a/core/pkg/ingress/controller/controller.go b/core/pkg/ingress/controller/controller.go index 9c96fe0dacb..6fb5c8c2127 100644 --- a/core/pkg/ingress/controller/controller.go +++ b/core/pkg/ingress/controller/controller.go @@ -153,7 +153,7 @@ func newIngressController(config *Configuration) *GenericController { cfg: config, stopLock: &sync.Mutex{}, stopCh: make(chan struct{}), - syncRateLimiter: flowcontrol.NewTokenBucketRateLimiter(0.5, 1), + syncRateLimiter: flowcontrol.NewTokenBucketRateLimiter(0.3, 1), recorder: eventBroadcaster.NewRecorder(scheme.Scheme, api.EventSource{ Component: "ingress-controller", }), @@ -400,27 +400,23 @@ func (ic *GenericController) syncIngress(key interface{}) error { } } - data, err := ic.cfg.Backend.OnUpdate(ingress.Configuration{ + err := ic.cfg.Backend.OnUpdate(ingress.Configuration{ Backends: upstreams, Servers: servers, TCPEndpoints: ic.getStreamServices(ic.cfg.TCPConfigMapName, api.ProtocolTCP), UDPEndpoints: ic.getStreamServices(ic.cfg.UDPConfigMapName, api.ProtocolUDP), PassthroughBackends: passUpstreams, }) - if err != nil { - return err - } - out, reloaded, err := ic.cfg.Backend.Reload(data) if err != nil { incReloadErrorCount() - glog.Errorf("unexpected failure restarting the backend: \n%v", string(out)) + glog.Errorf("unexpected failure restarting the backend: \n%v", err) return err } - if reloaded { - glog.Infof("ingress backend successfully reloaded...") - incReloadCount() - } + + glog.Infof("ingress backend successfully reloaded...") + incReloadCount() + return nil } diff --git a/core/pkg/ingress/types.go b/core/pkg/ingress/types.go index 3dcda05b41c..f0f75c87577 100644 --- a/core/pkg/ingress/types.go +++ b/core/pkg/ingress/types.go @@ -50,14 +50,6 @@ type Controller interface { // controller status healthz.HealthzChecker - // Reload takes a byte array representing the new loadbalancer configuration, - // and returns a byte array containing any output/errors from the backend and - // if a reload was required. - // Before returning the backend must load the configuration in the given array - // into the loadbalancer and restart it, or fail with an error and message string. - // If reloading fails, there should be not change in the running configuration or - // the given byte array. - Reload(data []byte) ([]byte, bool, error) // OnUpdate callback invoked from the sync queue https://k8s.io/ingress/core/blob/master/pkg/ingress/controller/controller.go#L387 // when an update occurs. This is executed frequently because Ingress // controllers watches changes in: @@ -78,12 +70,11 @@ type Controller interface { // servers (FQDN) and all the locations inside each server. Each // location contains information about all the annotations were configured // https://k8s.io/ingress/core/blob/master/pkg/ingress/types.go#L83 - // The backend returns the contents of the configuration file or an error - // with the reason why was not possible to generate the file. + // The backend returns an error if was not possible to update the configuration. // // The returned configuration is then passed to test, and then to reload // if there is no errors. - OnUpdate(Configuration) ([]byte, error) + OnUpdate(Configuration) error // ConfigMap content of --configmap SetConfig(*api.ConfigMap) // SetListers allows the access of store listers present in the generic controller diff --git a/examples/custom-controller/server.go b/examples/custom-controller/server.go index ee1a411a0ed..f1b4d6b84c0 100644 --- a/examples/custom-controller/server.go +++ b/examples/custom-controller/server.go @@ -52,20 +52,11 @@ func (dc DummyController) SetConfig(cfgMap *api.ConfigMap) { log.Printf("Config map %+v", cfgMap) } -func (dc DummyController) Reload(data []byte) ([]byte, bool, error) { - out, err := exec.Command("echo", string(data)).CombinedOutput() - if err != nil { - return out, false, err - } - log.Printf("Reloaded new config %s", out) - return out, true, err -} - func (dc DummyController) Test(file string) *exec.Cmd { return exec.Command("echo", file) } -func (dc DummyController) OnUpdate(updatePayload ingress.Configuration) ([]byte, error) { +func (dc DummyController) OnUpdate(updatePayload ingress.Configuration) error { log.Printf("Received OnUpdate notification") for _, b := range updatePayload.Backends { eps := []string{} @@ -74,7 +65,9 @@ func (dc DummyController) OnUpdate(updatePayload ingress.Configuration) ([]byte, } log.Printf("%v: %v", b.Name, strings.Join(eps, ", ")) } - return []byte(``), nil + + log.Printf("Reloaded new config") + return nil } func (dc DummyController) BackendDefaults() defaults.Backend {