-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[nginx-ingress-controller] Refactoring of template handling #1498
[nginx-ingress-controller] Refactoring of template handling #1498
Conversation
64ba353
to
dcd5ca3
Compare
watcher: watcher, | ||
} | ||
|
||
go func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vastly prefer if we did this in a compsable way. Perhaps we can have a sidecar that watches the config map, notices a change, and forces a reload on the nginx controller through a /reload http endpoint ? that would make for a nice reusable component.
$ watch-configmaps --reload-url="url-in-same-netns" --file="/path/to/file"
When we upgrade to docker1.12 we will get shared pid namespaces and can consider replacing the --reload-url arg with just pkill SIGHIUP nginx
.
The most reusable way to do this right now would be to avoid even the http server, and do:
$ watch-configmaps --pod-name="foopod, defaults to self" \
--container-name="nginx, defaults to first peer container in list of own pod" \
--file="/path/to/file" \
--script="/reload.sh"
That would kubectl exec in the give pod and exec the the signal to the given container/process. If you're interested you can download the kubectl client from:
wget https://storage.googleapis.com/kubernetes-release/release/${KUBERNETES_VERSION}/bin/linux/amd64/kubectl
Or exec through a REST client like we do in our e2es:
https://github.com/kubernetes/kubernetes/blob/4bb5fdc47f4c409f7d290b3af348fb135d0daf0f/test/e2e/framework/util.go#L3756
Quite a few people have asked for something like the last one. I would put this under ingress/ so it doesn't get lost when we split ingress out of contrib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vastly prefer if we did this in a composable way
Agree. I'll start working on this.
@bprashanth the last commit contains a sidecar that watch changes in a file or a kubernetes resource ( Is this change acceptable just to be able to use a sidecar and not add this code in the controller? |
@aledbf PR needs rebase |
5cc7cf7
to
77b87d0
Compare
@aledbf PR needs rebase |
@bprashanth ping |
name: watch-template | ||
command: | ||
- /watch-resource-cmdrunner | ||
- --command="echo 'nginx template updated'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you want to reload nginx when this happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. The correct content should be curl http://localhost:10254/reload-template
Suggest simplifying the pr to just watch configmaps mounted into 2 containers, and send a signal from the first to the second through kubeClient. Then write a good example that uses a configmap, modifies it , and has an nginx reload. We can iterate on growing that out as required. |
Just in case if an user just upgrades the current version (without a volume) the controller behaves like now (no reload if the template changes)
|
90865b6
to
c66133e
Compare
done Here is the important part Basically I watch for the template file for changes. Is not important from where the file comes (emptyDir, another volume, configmap, etc). |
This is an example of the change
|
61d00a7
to
aa8fd13
Compare
Also contains update to godeps required by #1623 |
// checkTemplate verifies the NGINX template exists (file /etc/nginx/template/nginx.tmpl) | ||
// If the file does not exists it means: | ||
// a. custom docker image | ||
// b. standard image using watch-resource sidecar with emptyDir volume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why emptyDir volume? isn't this just like the nginx example in main repo, except this sidecar is reusable with any existing pod that mounts a config map?
LGTM |
Automatic merge from submit-queue |
@aledbf , I change the content of /etc/nginx/template/nginx.tmpl in a running container, but no new nginx.conf is generated. What do I miss? |
This also watch for changes in the template
fixes #1457
This change is