UPSTREAM: 21373: kubelet: reading cloudinfo from cadvisor #8062
Conversation
|
[test] |
| node *api.Node, | ||
| info *cadvisorapi.MachineInfo) { | ||
|
|
||
| if info.CloudProvider != cadvisorapi.UnkownProvider && |
There was a problem hiding this comment.
The typo causes me pain here.
There was a problem hiding this comment.
Pretty sure we'll pick up the typo fix with the imminent cadvisor bump
|
Looks ok to me. Is the holdup upstream convincing vish? |
|
typo fix is in #8072 which bases us on the upstream v1.2.0 tag |
Brian punted it from 1.2 (not important enough I'd guess), so no one's looked at it again. Looked reasonable to me too. I'll update the typo when the new level merges. |
|
Updated on top of the 1.2 tag. @smarterclayton anything else holding this up? |
|
Evaluated for origin test up to d54ed4e |
| if info.CloudProvider != cadvisorapi.UnknownProvider && | ||
| info.CloudProvider != cadvisorapi.Baremetal { | ||
| node.Spec.ProviderID = strings.ToLower(string(info.CloudProvider)) + | ||
| ":////" + string(info.InstanceID) |
There was a problem hiding this comment.
Shouldn't this be //? That's what GetInstanceProviderID() returns
There was a problem hiding this comment.
Usually it returns ://<project_name>/<avilability_zone>/<instance_id> . we dont have the project name or the availability zone so they are left empty (kubelet's cloud providers may also return them empty when they are not available)
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2262/) |
|
one green, one infrastructure flake. |
|
LGTM [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5362/) (Image: devenv-rhel7_3772) |
|
Evaluated for origin merge up to d54ed4e |
Merged by openshift-bot
|
This either needs to be reverted or we need to push for google/cadvisor#1171 to be merged ASAP so we can pull it in to origin. https://bugzilla.redhat.com/show_bug.cgi?id=1319439 |
If cadvisor actually merges that pull, I'm ok picking it down here. Otherwise, I'd revert this. I planned to give it until tomorrow PM. |
When no --cloud-provider flag is given, try to use data from cadvisor to
determine the ProviderID.
@smarterclayton ptal
@simon3z @enoodle fyi