Watch controller Pods and make then available in k8sStore #3455

wayt · 2018-11-21T19:54:45Z

What this PR does / why we need it:

This add controller pod tracking for ingress-nginx. The list is available from the store ListControllerPods() function.

The purpose behind this is to track the number of running replicas.

A PR will follow to push this into a lua shared dict.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

wayt · 2018-11-21T19:56:00Z

internal/ingress/controller/store/store.go

+}
+
+// hasSameOwner returns true if both Pods share at least 1 owner
+func hasSameOwner(pod1, pod2 *corev1.Pod) bool {


@aledbf do you know any better way of validating two Pods are deployed by the same Deployment?

wayt · 2018-11-21T19:56:26Z

internal/ingress/controller/store/store.go

+
+// getCurrentPod returns the current Pod based on environment namespace and name
+func (s k8sStore) getCurrentPod() *corev1.Pod {
+	ns := os.Getenv("POD_NAMESPACE")


@aledbf do you know any better of getting current controller Pod?

I've replaced this with k8s.GetPodDetails https://github.com/kubernetes/ingress-nginx/pull/3455/files#diff-ece58b3bebec02a6d3173c8d8099d298R257

wayt · 2018-11-21T20:26:45Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -924,6 +924,10 @@ stream {

        {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders (collectCustomErrorsPerServer $server)) }}

+        location /debug {


This is for example purpose, I'll remove that

aledbf · 2018-11-22T01:44:13Z

@wayt what are you trying to do exactly?

I don't want to add an additional listener (pod listener will increase the apiserver pressure considerably)

wayt · 2018-11-22T14:09:06Z

We would like to have the controller pod list available within the nginx configuration templating.
In order to use them for Lua modules.

I understand that would add pressure on the apiserver, would you have any better idea regarding this?

This doesn't require a high reactivity and could be done much more lazily.

ElvinEfendi · 2018-11-22T14:15:10Z

Is there an HTTP endpoint to get current number of replicas (in corresponding deployment resource) from API server?

Or maybe just HTTP GET current list of replicas from API server?

The problem with this is when a replica dies we won't know it.

aledbf · 2018-11-22T14:33:39Z

@wayt with the current information we already have about the pod, using the pod-template-hash label to build a watch. This approach is a lot better than adding an informer for all the pods.

wayt · 2018-11-22T14:38:42Z

@aledbf oh great! I didn't know about that. I'll have a look.
Thanks!

aledbf · 2018-11-22T14:46:54Z

@wayt using the current information about the pod to build a watcher using the same labels that the pod is also another option (cleaner)

wayt · 2018-11-26T20:24:44Z

I've updated the PR, it now keeps track of the controller Pods based on labels.

/assign @ElvinEfendi

aledbf · 2018-11-26T20:25:57Z

internal/ingress/controller/store/pod.go

+)
+
+// PodLister makes a Store that lists Pods.
+type PodLister struct {


~~Why are you adding a podlister?~~

For https://github.com/kubernetes/ingress-nginx/pull/3455/files#diff-ece58b3bebec02a6d3173c8d8099d298R141

Do you think I can only use a cache.Store ? Since we don't need any custom function.

No, it's fine. I posted the comment before reading the complete PR, sorry :)

internal/ingress/controller/store/store.go

aledbf · 2018-11-26T20:40:37Z

/approve

aledbf · 2018-11-26T20:41:00Z

@wayt waiting @ElvinEfendi lgtm

ElvinEfendi · 2018-11-27T11:05:32Z

internal/ingress/controller/store/store.go

@@ -773,3 +835,20 @@ func (s k8sStore) Run(stopCh chan struct{}) {
 		go wait.Until(s.checkSSLChainIssues, 60*time.Second, stopCh)
 	}
 }
+
+// ListControllerPods returns a list of ingress-nginx controller Pods
+func (s k8sStore) ListControllerPods() []*corev1.Pod {


Can you make this clear that it returns only Running pods? Something like ListRunningControllerPods

ElvinEfendi · 2018-11-27T11:06:54Z

internal/ingress/controller/store/store.go

+				return client.CoreV1().Pods(store.pod.Namespace).List(options)
+			},
+			WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
+


no need for this newline. same as above

ElvinEfendi · 2018-11-27T11:08:04Z

/lgtm

The two comments can be addressed in the subsequent PR when we implement posting this data to Lua.

k8s-ci-robot · 2018-11-27T11:08:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi, wayt

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:

~~OWNERS~~ [ElvinEfendi,aledbf]

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

ElvinEfendi · 2018-11-27T11:08:30Z

/remove hold

ElvinEfendi · 2018-11-27T11:08:42Z

/hold cancel

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 21, 2018

k8s-ci-robot requested review from antoineco and ElvinEfendi November 21, 2018 19:54

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2018

wayt mentioned this pull request Nov 21, 2018

[WIP] Add controller pod list in nginx configuration templating Shopify/ingress#158

Closed

wayt commented Nov 21, 2018

View reviewed changes

wayt force-pushed the controller-pod branch from ea13d91 to 76c2f1c Compare November 21, 2018 20:25

wayt commented Nov 21, 2018

View reviewed changes

wayt force-pushed the controller-pod branch from 8728d3b to e9ce82c Compare November 21, 2018 21:57

aledbf added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2018

wayt force-pushed the controller-pod branch 2 times, most recently from 41362de to 8eb5eb2 Compare November 26, 2018 20:11

wayt changed the title ~~[WIP] Add controller pod list in nginx configuration templating~~ Watch controller Pods and make then available in k8sStore Nov 26, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 26, 2018

k8s-ci-robot assigned ElvinEfendi Nov 26, 2018

aledbf reviewed Nov 26, 2018

View reviewed changes

internal/ingress/controller/store/store.go Outdated Show resolved Hide resolved

Watch controller Pods list

b6b221a

wayt force-pushed the controller-pod branch from 8eb5eb2 to b6b221a Compare November 26, 2018 20:35

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2018

ElvinEfendi reviewed Nov 27, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2018

k8s-ci-robot merged commit 8299652 into kubernetes:master Nov 27, 2018

ElvinEfendi deleted the controller-pod branch November 27, 2018 11:16

wayt mentioned this pull request Dec 3, 2018

Update lua configuration_data when number of controller pod change #3505

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Watch controller Pods and make then available in k8sStore #3455

Watch controller Pods and make then available in k8sStore #3455

wayt commented Nov 21, 2018 •

edited

Loading

wayt Nov 21, 2018

wayt Nov 21, 2018

wayt Nov 21, 2018

wayt Nov 21, 2018

aledbf commented Nov 22, 2018

wayt commented Nov 22, 2018

ElvinEfendi commented Nov 22, 2018 •

edited

Loading

aledbf commented Nov 22, 2018

wayt commented Nov 22, 2018

aledbf commented Nov 22, 2018

wayt commented Nov 26, 2018

aledbf Nov 26, 2018 •

edited

Loading

wayt Nov 26, 2018

aledbf Nov 26, 2018

aledbf commented Nov 26, 2018

aledbf commented Nov 26, 2018

ElvinEfendi Nov 27, 2018

ElvinEfendi Nov 27, 2018

ElvinEfendi commented Nov 27, 2018

k8s-ci-robot commented Nov 27, 2018

ElvinEfendi commented Nov 27, 2018

ElvinEfendi commented Nov 27, 2018

		@@ -924,6 +924,10 @@ stream {

		{{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders (collectCustomErrorsPerServer $server)) }}

		location /debug {

Watch controller Pods and make then available in k8sStore #3455

Watch controller Pods and make then available in k8sStore #3455

Conversation

wayt commented Nov 21, 2018 • edited Loading

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

aledbf commented Nov 22, 2018

wayt commented Nov 22, 2018

ElvinEfendi commented Nov 22, 2018 • edited Loading

aledbf commented Nov 22, 2018

wayt commented Nov 22, 2018

aledbf commented Nov 22, 2018

wayt commented Nov 26, 2018

aledbf Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Nov 26, 2018

aledbf commented Nov 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Nov 27, 2018

k8s-ci-robot commented Nov 27, 2018

ElvinEfendi commented Nov 27, 2018

ElvinEfendi commented Nov 27, 2018

wayt commented Nov 21, 2018 •

edited

Loading

ElvinEfendi commented Nov 22, 2018 •

edited

Loading

aledbf Nov 26, 2018 •

edited

Loading