Append both the zone and the region to the federation query responses, not just the zone. #27960
Conversation
madhusudancs
added
priority/critical-urgent
k8s-github-robot
added
the
size/M
…, not just the zone.
madhusudancs
force-pushed
the
fed-kube-dns-append-region
branch
from
02e8ad0 to
6894e74
Compare
|
Rebased. |
| // that complexity is going to buy us. So taking a simpler approach here. | ||
| // Also note that zone here means the zone in cloud provider terminology, not | ||
| // the DNS zone. | ||
| func (kd *KubeDNS) getClusterZoneAndRegion() (string, string, error) { |
There was a problem hiding this comment.
Two notes on this:
- A cluster can exist in more than 1 zone.
- The code to find all zones and the region already exists in https://github.com/kubernetes/kubernetes/blob/master/federation/pkg/federation-controller/service/dns.go#L79 . It's not currently in a form where you can call it directly here, but we should refactor it so that you can (in a followup, non v1.3 PR).
Happy to leave as is for this PR.
There was a problem hiding this comment.
For some additional context and clarification, the actual code that fetches the zones and regions is here - https://github.com/kubernetes/kubernetes/blob/master/federation/pkg/federation-controller/cluster/cluster_client.go#L143. It is doing the same thing we do here (except it stores all the zones and regions and we arbitrarily pick one here). We should definitely merge the two implementations.
|
LGTM. Please open an issue for the nit. |
ghost
added
lgtm
| @@ -643,31 +646,37 @@ func (kd *KubeDNS) getClusterZone() (string, error) { | |||
| // TODO(madhusudancs): Move this to external/v1 API. | |||
| nodeList, err := kd.kubeClient.Core().Nodes().List(kapi.ListOptions{}) | |||
| if err != nil || len(nodeList.Items) == 0 { | |||
| return "", fmt.Errorf("failed to retrieve the cluster nodes: %v", err) | |||
| return "", "", fmt.Errorf("failed to retrieve the cluster nodes: %v", err) | |||
| } | |||
|
|
|||
| // Select a node (arbitrarily the first node) that has `LabelZoneFailureDomain` set. | |||
There was a problem hiding this comment.
femto nit : Update comment that we are looking for both LabelZoneRegion and LabelZoneFailureDomain has to be set?
|
@quinton-hoole filed the issue here - #27966 |
|
@quinton-hoole @dims Addressed the comments. PTAL again. |
| @@ -661,6 +662,8 @@ func (kd *KubeDNS) getClusterZoneAndRegion() (string, string, error) { | |||
| if err := kd.nodesStore.Add(node); err != nil { | |||
| return "", "", fmt.Errorf("couldn't add the retrieved node to the cache: %v", err) | |||
| } | |||
| // Node is found, break out of the loop. | |||
| break | |||
There was a problem hiding this comment.
Thanks for incorporating the feedback @madhusudancs !
k8s-github-robot
removed
the
lgtm
ghost
added
the
lgtm
|
GCE e2e build/test passed for commit a827ef0. |
|
Automatic merge from submit-queue |
This PR is based on @mml's #27896. I will rebase once that PR is merged.