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.

Sign up for GitHub

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

Jump to bottom

Zonelister logic prevents instance removal from instance group & instance group GC #50

Closed
bowei opened this issue Oct 11, 2017 · 21 comments
Closed

Zonelister logic prevents instance removal from instance group & instance group GC #50

bowei opened this issue Oct 11, 2017 · 21 comments
Assignees
@bowei
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@bowei
Copy link
Member

bowei commented Oct 11, 2017

From @nikhiljindal on July 25, 2017 1:38

Looking at the code (https://github.com/kubernetes/ingress/blob/a58b80017170eecbe8b2d6573b66192cafe0d32a/controllers/gce/controller/controller.go#L177), it looks like we are not doing anything when nodes are added and deleted. We should be updating the instance group when that happens.

Am I missing something?

Copied from original issue: kubernetes/ingress-nginx#1012
@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @nikhiljindal on July 25, 2017 1:39

@nicksardo pointed out that the logic was removed in kubernetes/ingress-nginx@c7c2a56#diff-9141c651905f3492033cf255f8e12fd7L176.

@aledbf Was that intentional?

@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @nicksardo on July 25, 2017 18:18

Quick FYI: the instance groups are eventually synced when the resync period occurs for any ingress. This may explain why nobody has noticed this in practice.

@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @nicksardo on July 31, 2017 23:47

This still occurs despite having the fix. I'm guessing that the handler is called when the node is added; however, it's NotReady state prevents the instance group from being updated. I don't know why the handler isn't called again when the node becomes Ready.

@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @aledbf on August 1, 2017 0:0

@nicksardo maybe because the handler does not have an UpdateFunc?

@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @nicksardo on August 1, 2017 0:32

Ah, that would certainly do it. According to the comment, we don't want to have an UpdateFunc due to the frequency. @nikhiljindal and I need to look more closely at the instance sync logic.

@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @nikhiljindal on August 1, 2017 17:8

If there was a way to find out the update, we could add an UpdateFunc that adds the node to queue only if there has been a "relevant change" in node. Node's heartbeat being updated, for example, is not a "relevant change".

@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @nicksardo on August 1, 2017 17:14

Right, not a quick change though.

@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @nicksardo on August 3, 2017 0:54

Looks like removing nodes from instances groups has never worked.

I0803 00:20:41.239780       5 instances.go:234] Syncing nodes [gke-testingress-default-pool-9818f010-zcxl gke-testingress-default-pool-9818f010-h3b8 gke-testingress-default-pool-9818f010-jk3f]
I0803 00:20:41.367945       5 instances.go:265] Removing nodes from IG: [gke-testingress-default-pool-9818f010-6znn gke-testingress-default-pool-9818f010-dz9r]
E0803 00:20:41.368941       5 instances.go:191] Failed to get zones for gke-testingress-default-pool-9818f010-6znn: node not found gke-testingress-default-pool-9818f010-6znn, skipping
E0803 00:20:41.369292       5 instances.go:191] Failed to get zones for gke-testingress-default-pool-9818f010-dz9r: node not found gke-testingress-default-pool-9818f010-dz9r, skipping
I0803 00:20:41.369599       5 utils.go:212] Syncing gke-testingress-default-pool-9818f010-8ffx
I0803 00:20:41.369912       5 instances.go:234] Syncing nodes [gke-testingress-default-pool-9818f010-zcxl gke-testingress-default-pool-9818f010-h3b8 gke-testingress-default-pool-9818f010-jk3f]
I0803 00:20:41.479824       5 instances.go:265] Removing nodes from IG: [gke-testingress-default-pool-9818f010-6znn gke-testingress-default-pool-9818f010-dz9r]
E0803 00:20:41.480328       5 instances.go:191] Failed to get zones for gke-testingress-default-pool-9818f010-6znn: node not found gke-testingress-default-pool-9818f010-6znn, skipping
E0803 00:20:41.481218       5 instances.go:191] Failed to get zones for gke-testingress-default-pool-9818f010-dz9r: node not found gke-testingress-default-pool-9818f010-dz9r, skipping
I0803 00:20:46.453664       5 utils.go:212] Syncing gke-testingress-default-pool-9818f010-dz9r
I0803 00:20:46.454195       5 instances.go:234] Syncing nodes [gke-testingress-default-pool-9818f010-h3b8 gke-testingress-default-pool-9818f010-jk3f gke-testingress-default-pool-9818f010-zcxl]
I0803 00:20:46.545582       5 utils.go:212] Syncing gke-testingress-default-pool-9818f010-6znn
I0803 00:20:46.546366       5 instances.go:234] Syncing nodes [gke-testingress-default-pool-9818f010-h3b8 gke-testingress-default-pool-9818f010-jk3f gke-testingress-default-pool-9818f010-zcxl]

@bowei
Copy link
Member Author

bowei commented Oct 11, 2017

From @nicksardo on September 14, 2017 20:42

Note: We also need to address node syncing with regards to large clusters. https://storage.googleapis.com/kubernetes-jenkins/logs/ci-kubernetes-e2e-gce-scale-correctness/12/artifacts/gce-scale-cluster-master/glbc.log

@bowei bowei mentioned this issue Oct 11, 2017
Closed
@gkop
Copy link

gkop commented Nov 29, 2017

What are the workarounds for this in practice? Does everybody follow up the k8s delete node API call with a second call to the GCP API? (that's what I do - it's annoying...)

@nicksardo nicksardo assigned bowei and unassigned nikhiljindal Jan 11, 2018
@nicksardo nicksardo changed the title GCE instance groups are not updated when nodes are added/removed Zonelister logic prevents instance removal from instance group & instance group GC Jan 11, 2018
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 11, 2018
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 11, 2018
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed bug labels Jun 5, 2018
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@nicksardo nicksardo removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 16, 2018
@nicksardo nicksardo reopened this Jul 16, 2018
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 14, 2018
@rramkumar1
Copy link
Contributor

/help-wanted
/good-first-issue
/remove-lifecycle stale

@k8s-ci-robot
Copy link
Contributor

@rramkumar1:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/help-wanted
/good-first-issue
/remove-lifecycle stale

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 31, 2018
@rramkumar1
Copy link
Contributor

/kind bug

@bowei
Copy link
Member Author

bowei commented Nov 6, 2018

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Nov 6, 2018
@agadelshin agadelshin mentioned this issue Dec 11, 2018
Merged
@agadelshin
Copy link
Contributor

From @nicksardo on August 3, 2017 0:54

Looks like removing nodes from instances groups has never worked.

I0803 00:20:41.239780       5 instances.go:234] Syncing nodes [gke-testingress-default-pool-9818f010-zcxl gke-testingress-default-pool-9818f010-h3b8 gke-testingress-default-pool-9818f010-jk3f]
I0803 00:20:41.367945       5 instances.go:265] Removing nodes from IG: [gke-testingress-default-pool-9818f010-6znn gke-testingress-default-pool-9818f010-dz9r]
E0803 00:20:41.368941       5 instances.go:191] Failed to get zones for gke-testingress-default-pool-9818f010-6znn: node not found gke-testingress-default-pool-9818f010-6znn, skipping
E0803 00:20:41.369292       5 instances.go:191] Failed to get zones for gke-testingress-default-pool-9818f010-dz9r: node not found gke-testingress-default-pool-9818f010-dz9r, skipping
I0803 00:20:41.369599       5 utils.go:212] Syncing gke-testingress-default-pool-9818f010-8ffx
I0803 00:20:41.369912       5 instances.go:234] Syncing nodes [gke-testingress-default-pool-9818f010-zcxl gke-testingress-default-pool-9818f010-h3b8 gke-testingress-default-pool-9818f010-jk3f]
I0803 00:20:41.479824       5 instances.go:265] Removing nodes from IG: [gke-testingress-default-pool-9818f010-6znn gke-testingress-default-pool-9818f010-dz9r]
E0803 00:20:41.480328       5 instances.go:191] Failed to get zones for gke-testingress-default-pool-9818f010-6znn: node not found gke-testingress-default-pool-9818f010-6znn, skipping
E0803 00:20:41.481218       5 instances.go:191] Failed to get zones for gke-testingress-default-pool-9818f010-dz9r: node not found gke-testingress-default-pool-9818f010-dz9r, skipping
I0803 00:20:46.453664       5 utils.go:212] Syncing gke-testingress-default-pool-9818f010-dz9r
I0803 00:20:46.454195       5 instances.go:234] Syncing nodes [gke-testingress-default-pool-9818f010-h3b8 gke-testingress-default-pool-9818f010-jk3f gke-testingress-default-pool-9818f010-zcxl]
I0803 00:20:46.545582       5 utils.go:212] Syncing gke-testingress-default-pool-9818f010-6znn
I0803 00:20:46.546366       5 instances.go:234] Syncing nodes [gke-testingress-default-pool-9818f010-h3b8 gke-testingress-default-pool-9818f010-jk3f gke-testingress-default-pool-9818f010-zcxl]

node can't be found because GetZoneFromNode filters nodes with not ready statuses. I opened PR to fix this particular issue. I'm working on Add/Update/Delete functions but it might take a lot of time.

I've found the way how I can connect node object from nodeInformer with ingresses:

  1. find instance group for this node
  2. iterate through backendPool.List() (BackendService) to find Backend with the Group field with instance group name from 1.
  3. pass this BackendService to find ingresses with this BackendService in label "ingress.kubernetes.io/backends"

It looks ugly right now, but it's working. But I didn't find the way to track only relevant node updates.

@Aut0R3V
Copy link

Aut0R3V commented Jan 7, 2021

@bowei anything that can be done here?

@swetharepakula
Copy link
Member

There have been various fixes in this area to make the ingress controllers react to zone changes. If the issues still exist, please open a new issue with details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bowei bowei

Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@nicksardo @gkop @bowei @agadelshin @swetharepakula @nikhiljindal @k8s-ci-robot @rramkumar1 @fejta-bot @Aut0R3V