Bug 1538389 - Allow node IP change to update Host IP in HostSubnet resource #18281
Conversation
openshift-ci-robot
added
the
size/M
openshift-ci-robot
added
the
approved
pravisankar
force-pushed
the
fix-master-nodeevent
branch
from
f97e1ab
to
79fc3bb
Compare
pravisankar
added
kind/bug
|
@openshift/sig-networking PTAL |
|
LGTM |
|
/retest |
pkg/network/master/subnets.go
Outdated
|
|
||
| var nodeIP string | ||
| if len(node.Status.Addresses) > 0 && node.Status.Addresses[0].Address != "" { | ||
| nodeIP = node.Status.Addresses[0].Address |
There was a problem hiding this comment.
Do we want to check on NodeAddressType here now, to eg prefer NodeInternalAddress? I'm assuming that netutils.GetNodeIP() would most likely return the private/internal address for the node given it's a DNS lookup and a cloud provider would likely return the private/internal address rather than the public one.
Otherwise LGTM; better to be more explicit in the node config than rely on "magic" in the master to figure out this stuff.
There was a problem hiding this comment.
It is possible that node address got from DNS lookup can differ from what cloud provider returns. Currently, kubelet is populating first node address as the NodeInternalIP but I agree that we should not depend on the kubelet internals.
Yes, I will make it explicit here.
|
@rajatchopra PTAL |
- If IP addr is not populated in node object, master should not try to resolve the node address because it may not match exact IP addr used by node. - We can safely ignore the event in the case and when the IP is populated in then node status object, we will get another event and that is the right time to create/update hostsubnet.
- Node status addresses may have both old and new IP address and by validating against these addrs we may not be able to update HostSubnet with new node IP. Openshift node service waits for its HostSubnet resource with new Host IP and eventually fails. - This change fixes the above issue by reverting the change for node flip/flop. Recommended/correct way to handle node flip/flop case is to specify nodeIP config option in node-config.yaml
pravisankar
force-pushed
the
fix-master-nodeevent
branch
from
79fc3bb
to
31fcad5
Compare
|
Updated, address with NodeInternalIP type is used instead of node.Status.Addresses[0].Address to fetch the node IP. |
| } | ||
| } | ||
| if len(nodeIP) == 0 { | ||
| glog.Errorf("Node IP is not set for node %s, skipping %s event, node: %v", node.Name, eventType, node) |
There was a problem hiding this comment.
This entire file needs to use utilruntime.HandleError. Any kube code that eats an error (that uses glog.Errorf) should be utilruntime.HandleError instead. That ensures that we get rate limiting, that errors are reported to sentry, and also that hotloop requests get processed.
Can be a follow up PR but please correct that before we ship 3.9
There was a problem hiding this comment.
Created follow up pr: #18343
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pravisankar, rajatchopra The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest |
|
Automatic merge from submit-queue. |
Node status addresses may have both old and new IP address and by
validating against these addrs we may not be able to update HostSubnet
with new node IP. Openshift node service waits for its HostSubnet resource
with new Host IP and eventually fails.
This change fixes the above issue by reverting the change for node flip/flop.
Recommended/correct way to handle node flip/flop case is to specify nodeIP config option in node-config.yaml