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

Implement GetZoneByProviderID and GetZoneByNodeName #18

Closed
andrewsykim opened this issue Aug 18, 2017 · 5 comments
Closed

Implement GetZoneByProviderID and GetZoneByNodeName #18

andrewsykim opened this issue Aug 18, 2017 · 5 comments
Assignees

Comments

@andrewsykim
Copy link
Contributor

As per kubernetes/kubernetes#50926, DO CCM should implement those methods so we accurately represent the region of droplets instead of assuming droplets are in the same region as CCM.

bhcleek added a commit to bhcleek/digitalocean-cloud-controller-manager that referenced this issue Sep 27, 2017
bhcleek added a commit to bhcleek/digitalocean-cloud-controller-manager that referenced this issue Sep 28, 2017
bhcleek added a commit to bhcleek/digitalocean-cloud-controller-manager that referenced this issue Oct 2, 2017
@klausenbusk
Copy link
Contributor

klausenbusk commented Dec 6, 2017

Did this fix the issue mentioned in #24 (comment) ?
The code still seems to contain the "broken" logic?

@andrewsykim
Copy link
Contributor Author

@klausenbusk this should have been fixed. What code are you referring to? GetZone()?

@klausenbusk
Copy link
Contributor

@klausenbusk this should have been fixed. What code are you referring to? GetZone()?

This code:

func dropletRegion() (string, error) {
return httpGet(dropletRegionMetadataURL)
}
which is used here:
region, err := dropletRegion()
and then passed as arguments here:
instances: newInstances(doClient, region),
zones: newZones(doClient, region),
loadbalancers: newLoadbalancers(doClient, region),

Maybe I just don't understand the code :)

@andrewsykim
Copy link
Contributor Author

The confusion there is how the Zones interface was designed. The GetZone method was originally for kubelets to get their own zones, usually using a meta data service. External cloud controllers have to adopt the same interfaces for compatibility. We technically don't even need to implement that method since it would never be called in our context, but it was used since GetZone was the only method at the time.

In v1.8, GetZoneByProviderID and GetZoneByName were added which is what's really called by cloud controllers upstream. Before v1.8, this was considered a bug as per kubernetes/kubernetes#49308

@andrewsykim
Copy link
Contributor Author

andrewsykim commented Dec 6, 2017

Our implementation for GetZoneByProviderID and GetZoneByName https://github.com/digitalocean/digitalocean-cloud-controller-manager/blob/master/do/zones.go#L43-L70

And the only piece of code using the region field is GetZone https://github.com/digitalocean/digitalocean-cloud-controller-manager/blob/master/do/zones.go#L40 which as I mentioned previously is not really used in our context, might be worthwhile to remove the implementation there altogether to avoid future confusion :)

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

No branches or pull requests

3 participants