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.

Sign up for GitHub

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

Jump to bottom

cloudinfo: Adding AWS, Azure support and InstanceID #1109

Merged
merged 2 commits into from
Feb 24, 2016
Merged

cloudinfo: Adding AWS, Azure support and InstanceID #1109

merged 2 commits into from
Feb 24, 2016

Conversation

enoodle
Copy link
Contributor

@enoodle enoodle commented Feb 11, 2016

No description provided.

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@k8s-bot
Copy link
Collaborator

k8s-bot commented Feb 11, 2016

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@enoodle
Copy link
Contributor Author

enoodle commented Feb 11, 2016

what is the Authorized Contributors Group for the CLA ?

@enoodle enoodle force-pushed the cloudinfo_add_aws_and_instance_id branch 3 times, most recently from 9320eeb to 1ccd1da Compare February 11, 2016 14:31
@enoodle
Copy link
Contributor Author

enoodle commented Feb 11, 2016

@simon3z

@vishh
Copy link
Contributor

vishh commented Feb 11, 2016

ok to test

@vishh
Copy link
Contributor

vishh commented Feb 11, 2016

Overall lgtm

@k8s-bot
Copy link
Collaborator

k8s-bot commented Feb 11, 2016

Jenkins GCE e2e

Build/test failed for commit 1ccd1da.

simon3z
simon3z reviewed Feb 11, 2016
View reviewed changes
utils/cloudinfo/cloudinfo.go
}

func NewRealCloudInfo() CloudInfo {
cloudProvider := detectCloudProvider()
instanceType := detectInstanceType(cloudProvider)
instanceID := detectInstanceID(cloudProvider)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this take a long time? (e.g. is it polling ip addresses that may not be always available?)
Would that be a problem? Should this be async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think detectCloudProvider could take some time. The others will be almost immediate as we are accessing the correct API server. In short this function could block for some time and its users should be aware of this as it was already blocking before this patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

@enoodle ok 👍

@enoodle enoodle changed the title [WIP] cloudinfo: Adding AWS support and InstanceID [WIP] cloudinfo: Adding AWS, Azure support and InstanceID Feb 14, 2016
@enoodle enoodle force-pushed the cloudinfo_add_aws_and_instance_id branch from 1ccd1da to fd2efdd Compare February 14, 2016 09:13
@k8s-bot
Copy link
Collaborator

k8s-bot commented Feb 14, 2016

Jenkins GCE e2e

Build/test failed for commit fd2efdd.

@enoodle enoodle force-pushed the cloudinfo_add_aws_and_instance_id branch from fd2efdd to 683fdfa Compare February 15, 2016 11:27
@k8s-bot
Copy link
Collaborator

k8s-bot commented Feb 15, 2016

Jenkins GCE e2e

Build/test failed for commit 683fdfa.

simon3z
simon3z reviewed Feb 15, 2016
View reviewed changes
utils/cloudinfo/azure.go
if err != nil {
return false
}
return strings.Contains(string(data), MicrosoftCorporation)
Copy link
Contributor

Choose a reason for hiding this comment

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

@enoodle is this a stable check? Is it documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I couldn't find any api service comparable to aws/gce 's for Azure, nor any other official documentation on ways to detect if an instance is running on Azure. I did find [1] which explains how to use dmidecode to get the instance ID (hence /sys/class/dmi/id/produect_uuid) and I noticed that on all my instances I had that string for sys_vendor so I used this.

[1] https://azure.microsoft.com/en-us/blog/accessing-and-using-azure-vm-unique-id/

@k8s-bot
Copy link
Collaborator

k8s-bot commented Feb 17, 2016

Jenkins GCE e2e

Build/test passed for commit 8d017e4.

@enoodle
Copy link
Contributor Author

enoodle commented Feb 17, 2016

@deads2k can you review this? Thanks

@enoodle enoodle mentioned this pull request Feb 17, 2016
Merged
@deads2k
Copy link

deads2k commented Feb 17, 2016

@googlebot @enoodle is covered under the Red Hat CLA.

@deads2k
Copy link

deads2k commented Feb 17, 2016

@enoodle When you add the godep packages, can you name the commit bump(github.com/aws/aws-sdk-go) 72440a9c5a884936f1c679591fcf04e31a9b772a (that looks like what you were trying to get). If the rest are just transitive that's fine, but that name will help track back to the target.

@deads2k
Copy link

deads2k commented Feb 17, 2016

Is this still a WIP? If so, what is remaining?

@enoodle enoodle changed the title [WIP] cloudinfo: Adding AWS, Azure support and InstanceID cloudinfo: Adding AWS, Azure support and InstanceID Feb 17, 2016
@enoodle
Copy link
Contributor Author

enoodle commented Feb 17, 2016

@deads2k Nothing more to implement. It was wip for the dependencies.

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@enoodle
Copy link
Contributor Author

enoodle commented Feb 17, 2016

@deads2k I will rebase to change the dependencies commit name in a moment.

@enoodle enoodle force-pushed the cloudinfo_add_aws_and_instance_id branch from 8d017e4 to c35ede7 Compare February 17, 2016 15:07
@k8s-bot
Copy link
Collaborator

k8s-bot commented Feb 17, 2016

Jenkins GCE e2e

Build/test passed for commit c35ede7.

@deads2k
Copy link

deads2k commented Feb 17, 2016

@vishh Implementation is done. This enables fallback behavior for ProviderID determination in the kubelet (see kubernetes/kubernetes#21373). Take another look?

@deads2k
Copy link

deads2k commented Feb 18, 2016

@jimmidyson ?

@enoodle enoodle force-pushed the cloudinfo_add_aws_and_instance_id branch from c35ede7 to a32a138 Compare February 18, 2016 13:49
@k8s-bot
Copy link
Collaborator

k8s-bot commented Feb 18, 2016

Jenkins GCE e2e

Build/test passed for commit a32a138.

@jimmidyson
Copy link
Collaborator

@deads2k Sorry on vacation.

@vishh Could you review when you get a chance please?

@enoodle
Copy link
Contributor Author

enoodle commented Feb 24, 2016

@vishh ping? you actually lgtm'ed it before, I only added a tiny fix to remove \n char when reading the azure instance ID from file.

@vishh
Copy link
Contributor

vishh commented Feb 24, 2016

Sorry. This somehow fell off my radar. LGTM again

vishh added a commit that referenced this pull request Feb 24, 2016
@vishh
Merge pull request #1109 from enoodle/cloudinfo_add_aws_and_instance_id
eab201f 
cloudinfo: Adding AWS, Azure support and InstanceID
@vishh vishh merged commit eab201f into google:master Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cherrypick-candidate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@enoodle @googlebot @k8s-bot @vishh @deads2k @jimmidyson @simon3z