Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabled the dynamic reload of GeoIP data #2107

Merged
merged 5 commits into from
Feb 17, 2018
Merged

Enabled the dynamic reload of GeoIP data #2107

merged 5 commits into from
Feb 17, 2018

Conversation

Stono
Copy link
Contributor

@Stono Stono commented Feb 16, 2018

Hey,
This attempts to fix #2100

In summary I have:

  • Moved the geoip data to /etc/nginx/geoip
  • Added Filesystem watchers on each of the files in that directory.

Things I don't like:

  • I've hard coded the files in /etc/nginx/geoip, it'd be nice to probably just enumerate the files in that directory and watch them all (in case they change name/are added/removed)
  • I've probably wrote terrible golang, this is literally my first golang change PR ever - so I apologise!

I've tested it on my controller, this was me modifying each file:

I0216 19:46:45.439078       7 nginx.go:158] Triggering NGINX reload
I0216 19:46:50.054260       7 controller.go:180] ingress backend successfully reloaded...
I0216 19:47:05.182746       7 nginx.go:158] Triggering NGINX reload
I0216 19:47:05.183088       7 controller.go:171] backend reload required
I0216 19:47:09.787091       7 controller.go:180] ingress backend successfully reloaded...
I0216 19:47:14.246166       7 nginx.go:158] Triggering NGINX reload
I0216 19:47:14.246540       7 controller.go:171] backend reload required
I0216 19:47:19.135127       7 controller.go:180] ingress backend successfully reloaded...

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 16, 2018
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 16, 2018
@Stono
Copy link
Contributor Author

Stono commented Feb 16, 2018

/assign @aledbf

}
geoip_get "GeoIP.dat.gz" "https://geolite.maxmind.com/download/geoip/database/GeoLiteCountry/GeoIP.dat.gz"
geoip_get "GeoLiteCity.dat.gz" "https://geolite.maxmind.com/download/geoip/database/GeoLiteCity.dat.gz"
geoip_get "GeoIPASNum.dat.gz" "http://download.maxmind.com/download/geoip/database/asnum/GeoIPASNum.dat.gz"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea 👍

var watchPath string
var watchAction func()
var addWatcher func()
addWatcher = func() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't work out how to define a function with parameters!? hence why I had to do variables, set them, and invoke the function. It works, but it doesn't feel right?

addWatcher()

// Add the GeoIP data to the watcher
geoData := [3]string{"/etc/nginx/geoip/GeoIP.dat", "/etc/nginx/geoip/GeoIPASNum.dat", "/etc/nginx/geoip/GeoLiteCity.dat"}
Copy link
Contributor Author

@Stono Stono Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to enumerate the directory instead of hard coding files, as I couldn't sus our how to recursively watch a folder

@aledbf
Copy link
Member

aledbf commented Feb 16, 2018

@Stono how about

		_, err = watch.NewFileWatcher(tmplPath, onChange)
		if err != nil {
			glog.Fatalf("unexpected error creating file watcher: %v", err)
		}

		filesToWatch := []string{}
		err := filepath.Walk("/etc/nginx/geoip/", func(path string, info os.FileInfo, err error) error {
			if info.IsDir() {
				return nil
			}

			filesToWatch = append(filesToWatch, path)
			return nil
		})

		if err != nil {
			glog.Fatalf("unexpected error creating file watcher: %v", err)
		}

		for _, f := range filesToWatch {
			_, err = watch.NewFileWatcher(f, func() {
				glog.Info("file %v changed. Reloading NGINX")
				n.SetForceReload(true)
			})
			if err != nil {
				glog.Fatalf("unexpected error creating file watcher: %v", err)
			}
		}

(inside the else)

@Stono
Copy link
Contributor Author

Stono commented Feb 16, 2018

@aledbf thanks! I had to change: glog.Info("file %v changed. Reloading NGINX") to glog.Info("file %v changed. Reloading NGINX", f) as the log was coming out as: I0216 20:31:49.852270 7 nginx.go:208] file %v changed. Reloading NGINX but other than that it worked a treat!

@Stono
Copy link
Contributor Author

Stono commented Feb 17, 2018

@aledbf are the e2e tests flakey?

Not sure how this change would have caused:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x116038f]

@aledbf
Copy link
Member

aledbf commented Feb 17, 2018

Not sure how this change would have caused:

The update of the nginx image will solve this issue.

@aledbf
Copy link
Member

aledbf commented Feb 17, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, Stono

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@aledbf aledbf merged commit d1b6f32 into kubernetes:master Feb 17, 2018
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow run time updating of GeoIP databases
4 participants