Changes to support debugging haproxy reload not loading correct config #123

westse · 2018-03-16T23:19:19Z

Under high load situations (hundreds of services, certs, and ingress rules), we have seen infrequent but consistent cases of haproxy not reflecting the on-disk config file. One theory is a race condition on new haproxy processes pointing to an ever-changing config file. This commit gives each haproxy process its own config file that does not change. Not only should this remove the risk of loading a partially written file, it will also help debug the issue because we see a snapshot of the config used for each haproxy process.
Config files are left on disk up to a max threshold, configurable via environment variable, to allow debugging issues.
Not supported under with reloadStrategy=multibinder since multibinder-haproxy-wrapper currently only supports receiving a USR2 signal to reload a fixed config file

- Under high load situations (hundreds of services, certs, and ingress rules), we have seen infrequent but consistent cases of haproxy not reflecting the on-disk config file. On theory is a race condition on new haproxy processes pointing to an ever-changing config file. This commit gives each haproxy process its own config file that does not change. Not only should this remove the risk of loading a partially written file, it will also help debug the issue because we see a snapshot of the config used for each haproxy process. - Config files are left on disk up to a max threshold, configurable via environment variable, to allow debugging issues. - Not supported under with reloadStrategy=multibinder since multibinder-haproxy-wrapper currently only supports receiving a USR2 signal to reload a fixed config file

ausmith · 2018-03-19T17:55:37Z

pkg/controller/controller.go

 	}
 	return err
 }

 // RewriteConfigFiles safely replaces configuration files with new contents after validation
-func rewriteConfigFiles(data []byte, reloadStrategy, configFile string) error {


I noticed that you are rewriting the multibinder and non-multibinder usages here, which concerns me mostly due to fear of change than anything else. Curious, did you consider the following flow instead and why would it be a bad approach?

func rewriteConfigFiles(data []byte, reloadStrategy, configFile string) error { // Determine temp file path using a timestamp // Write rendered template file there // if reload strategy == multibinder then do multibinder logic for moving it into place // else do non-multibinder work with config file // copy temp file to location for later investigation if required // rename file to the /etc/haproxy/haproxy.cfg }

Seems to me that this approach would minimize the amount of code you would need to touch and still give you a rendered template somewhere for investigation? Also unsure about where in that flow a cleanup step would be most efficient.

If I'm following your pseudo code, wouldn't the last step prevent each haproxy process from pointing to a unique timestamped config file? That would defeat the primary changed intended by this pull request (for the non-multibinder usage).

I agree with the sentiment to touch only the code that needs to change. I initially tried this but once I understood what it did I felt some bits of logic were confusing enough that it would be improved with what you see here. For example, I found it suprising that the non-multibinder case used the post-rename config file, while the multibinder case did not. I think this change makes the behavior in each usage more clear, but I may be wrong and am definitely open.

Thanks for taking a look.

I see the difference in approach now. My hope was that all reload strategies would be capable of at least storing copies of their config at reload time (by copying the file off before putting it into place), but understand that is simply adding files that provide limited value. Look forward to verifying this code when the new version gets built and tagged!

jcmoraisjr

I did some new tests and can confirm that everything related to reload works synchronously - from ingress events, config generation and also HAProxy reload, which only fork the process to the background after parses the config file.

There is only one issue here afaics: multibinder uses signal to restart the process which works asynchronously. Perhaps your use case was using multibinder? This could be patched eg at reload.sh.

Anyway I'm fine with this PR because one could want to have old config files for eg debugging his environment.

Following some changing suggestions, have a look if you agree.

jcmoraisjr · 2018-03-19T21:31:35Z

pkg/controller/controller.go

+	haproxy.configDir = "/etc/haproxy"
+	haproxy.configFilePrefix = "haproxy"
+	haproxy.configFileSuffix = ".cfg"
+	haproxy.template = newTemplate("haproxy.tmpl", "/etc/haproxy/template/haproxy.tmpl")


Use newTemplate as the else part of the if multibinder statement.

jcmoraisjr · 2018-03-19T21:34:28Z

pkg/controller/controller.go

+	envVar := os.Getenv("MAX_OLD_CONFIG_FILES")
+	if maxOldConfigFiles, err := strconv.Atoi(envVar); err == nil {
+		haproxy.maxOldConfigFiles = maxOldConfigFiles
+	}


use a command-line argument instead, declare at ConfigureFlags func just below reloadStrategy.

I'd also like to see a zero option, which means no old backup config should be used. I have some manual testing scenarios where I need a known filename, perhaps others could have similar issues.

jcmoraisjr · 2018-03-19T21:35:37Z

pkg/controller/controller.go

@@ -166,56 +181,87 @@ func (haproxy *HAProxyController) OnUpdate(cfg ingress.Configuration) error {
 		return nil
 	}

-	out, err := haproxy.reloadHaproxy()
+	reloadCmd := haproxy.reloadHaproxy(configFile)


reloadHaproxy func could be removed and it's single line placed here.

jcmoraisjr · 2018-03-19T21:36:00Z

pkg/controller/controller.go

+func (haproxy *HAProxyController) rewriteConfigFiles(data []byte) (string, error) {
+	// Include timestamp in config file name to aid troubleshooting. When using a single, ever-changing config file it
+	// was difficult to know what config was loaded by any given haproxy process
+	timestamp := time.Now().Format("-060102-150405.0000")


What about four digits to the year and three to the milisseconds?

@jcmoraisjr Do you mean a formate like -0102-150405.000, where year and nanoseconds are lost but month, day, hour, second, millisecond are retained? I think that will be fine since there is low risk of overwriting based on either of those.

I mean following this convention: -20060102-150405.000.

jcmoraisjr · 2018-03-19T21:36:39Z

pkg/controller/controller.go

 		}
 	}
-	err = os.Rename(tmpf, configFile)
+
+	haproxy.removeOldConfigFiles(haproxy.configFileSuffix)


removeOldCOnfigFiles func has already access to this argument.

jcmoraisjr · 2018-03-19T21:39:33Z

pkg/controller/controller.go

 	if err != nil {
-		glog.Warningln("Error updating config file")
 		return err
 	}


error isn't being used, so log a warningf("...: %v", err) here and return without an argument

westse · 2018-03-19T23:09:32Z

@jcmoraisjr I've incorporated each item you mentioned, except the timestamp format (see my question). So let me know how it looks. Thanks.

westse · 2018-03-19T23:18:12Z

Ok, I've fixed the timestamp format now as well.

jcmoraisjr · 2018-03-19T23:47:46Z

Just a few more issues:

please use zero old files as default
check for negative numbers, converting them to zero
you should use the pointer itself on flags.Int() otherwise the value won't be updated - see reloadStrategy usage
when zero is used the idea is to leave the timestamp empty - it's actually filling the timestamp
removeOld shouldn't be used if maxOldFiles is zero - it's removing the actual config file.

westse · 2018-03-20T01:04:51Z

Oops, I forgot to implement the "zero means no timestamp" part. Good catches - the code has been updated. Also feel free to tweak as you see fit.

jcmoraisjr

Two more issues and I think we're done

jcmoraisjr · 2018-03-20T01:16:01Z

pkg/controller/controller.go

+		`Maximum old haproxy timestamped config files to allow before being cleaned up. A value <= 0 indicates a single non-timestamped config file will be used`)
+	if *haproxy.maxOldConfigFiles < 0 {
+		*haproxy.maxOldConfigFiles = 0
+	}


This is just the initialization, the args wasn't parsed yet, so this if should be at OverrideFlags which happens after the parse.

Anyway afaics you are using >0 or <=0 everywhere so it's pointless to check it's value. I think it's safe to just remove it.

jcmoraisjr · 2018-03-20T01:18:01Z

pkg/controller/controller.go

 			matchesFound = matchesFound + 1
-			if matchesFound > haproxy.maxOldConfigFiles {
+			if matchesFound > *haproxy.maxOldConfigFiles {
 				filePath := haproxy.configDir + "/" + f.Name()
 				glog.Infof("Removing old config file (%v). maxOldConfigFiles=%v", filePath, haproxy.maxOldConfigFiles)


Missing a * on maxOldConfFiles or you'd output the memory address.

westse · 2018-03-20T03:12:30Z

thanks, updated

jcmoraisjr · 2018-03-20T16:00:00Z

Lgtm, thanks for the updates!

ausmith reviewed Mar 19, 2018

View reviewed changes

jcmoraisjr reviewed Mar 19, 2018

View reviewed changes

westse force-pushed the per-process-config branch 2 times, most recently from 2e24dd3 to 9e43c6a Compare March 19, 2018 23:17

westse force-pushed the per-process-config branch from 9e43c6a to a9f5f1b Compare March 20, 2018 01:01

jcmoraisjr reviewed Mar 20, 2018

View reviewed changes

Changes per review

c9f232d

westse force-pushed the per-process-config branch from a9f5f1b to c9f232d Compare March 20, 2018 03:10

westse mentioned this pull request Mar 20, 2018

Workaround haproxy hitless reload limit on file descriptors #124

Closed

jcmoraisjr merged commit 84a989c into jcmoraisjr:master Mar 20, 2018

matthewpucc mentioned this pull request Mar 22, 2018

Under high load, haproxy reloads using incomplete config files #127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to support debugging haproxy reload not loading correct config #123

Changes to support debugging haproxy reload not loading correct config #123

westse commented Mar 16, 2018

ausmith Mar 19, 2018

westse Mar 19, 2018

ausmith Mar 20, 2018

jcmoraisjr left a comment

jcmoraisjr Mar 19, 2018

jcmoraisjr Mar 19, 2018

jcmoraisjr Mar 19, 2018

jcmoraisjr Mar 19, 2018

westse Mar 19, 2018

jcmoraisjr Mar 19, 2018

jcmoraisjr Mar 19, 2018

jcmoraisjr Mar 19, 2018

westse commented Mar 19, 2018

westse commented Mar 19, 2018

jcmoraisjr commented Mar 19, 2018

westse commented Mar 20, 2018

jcmoraisjr left a comment

jcmoraisjr Mar 20, 2018

jcmoraisjr Mar 20, 2018

westse commented Mar 20, 2018

jcmoraisjr commented Mar 20, 2018

Changes to support debugging haproxy reload not loading correct config #123

Changes to support debugging haproxy reload not loading correct config #123

Conversation

westse commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcmoraisjr 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

westse commented Mar 19, 2018

westse commented Mar 19, 2018

jcmoraisjr commented Mar 19, 2018

westse commented Mar 20, 2018

jcmoraisjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westse commented Mar 20, 2018

jcmoraisjr commented Mar 20, 2018