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

Handle mismatched nodenames in etcd mode IPAM cleanup #467

Conversation

caseydavenport
Copy link
Member

@caseydavenport caseydavenport commented Apr 14, 2020

Description

Looks like our IPAM GC controller has a bug in etcd mode when the name
of the Node in Calico's etcd doesn't match the name of the node in the
Kubernetes API.

It causes the controller to wrongfully release IP addresses that should
not yet be released. This change updates the controller to map nodenames
from Calico node names to Kubernetes node names before doing the
existence check.

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Fix IPAM garbage collection in etcd mode on clusters where node name does not match Kubernetes node name. 

@@ -63,14 +63,14 @@ func (c *NodeController) OnUpdates(updates []bapi.Update) {
case bapi.UpdateTypeKVUpdated:
n := upd.KVPair.Value.(*apiv3.Node)

if c.config.SyncLabels {
Copy link
Member Author

Choose a reason for hiding this comment

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

This change just makes sure we're always populating the nodemapper map (we're already always running the syncer.)

@caseydavenport caseydavenport force-pushed the casey-handle-mismatched-nodenames branch from 0145628 to cb9e2a6 Compare April 14, 2020 21:43
Copy link
Contributor

@robbrockbank robbrockbank left a comment

Choose a reason for hiding this comment

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

See inline comment.

@caseydavenport caseydavenport force-pushed the casey-handle-mismatched-nodenames branch 2 times, most recently from b38afae to b15c86a Compare April 15, 2020 16:37
@caseydavenport caseydavenport force-pushed the casey-handle-mismatched-nodenames branch from b15c86a to 7a910e0 Compare April 15, 2020 17:09
@caseydavenport caseydavenport force-pushed the casey-handle-mismatched-nodenames branch from 7a910e0 to 1e2e6b4 Compare April 15, 2020 17:19
Copy link
Contributor

@robbrockbank robbrockbank left a comment

Choose a reason for hiding this comment

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

A few minor nits and possible very minor bugettes.

Copy link
Contributor

@robbrockbank robbrockbank left a comment

Choose a reason for hiding this comment

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

UT needs fixing. Possible minor bug related to tracking node tidy up.

Copy link
Contributor

@robbrockbank robbrockbank left a comment

Choose a reason for hiding this comment

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

LGTM - as soon as tests pass :-)

@caseydavenport caseydavenport merged commit 4da400c into projectcalico:master Apr 15, 2020
@caseydavenport caseydavenport deleted the casey-handle-mismatched-nodenames branch April 15, 2020 21:58
caseydavenport added a commit to caseydavenport/kube-controllers that referenced this pull request Apr 15, 2020
)

* Handle mismatched nodenames in etcd mode IPAM cleanup

* Add IPAM GC test for etcd mismatched nodenames

* Don't do anything for nodes which don't have a k8s equivalent

* Handle case where no Calico node exists for IPAM entry

* Add some more IPAM GC FV tests

* Review comments

* Update go modules

* More feedback
caseydavenport added a commit to caseydavenport/kube-controllers that referenced this pull request Apr 15, 2020
)

* Handle mismatched nodenames in etcd mode IPAM cleanup

* Add IPAM GC test for etcd mismatched nodenames

* Don't do anything for nodes which don't have a k8s equivalent

* Handle case where no Calico node exists for IPAM entry

* Add some more IPAM GC FV tests

* Review comments

* Update go modules

* More feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants