Refactor metadata fetching into `MetadataProvider` trait #88

csssuf · 2018-06-18T22:10:58Z

Instead of fetch_metadata() fetching all potentially needed metadata, return a MetadataProvider trait object which has drop-in replacements for Metadata's functions. This allows only the requested metadata types to be fetched and avoids potentially expensive fetching work.

There's still a little bit of cleanup work that could take place in some of the providers (mostly rolling now-duplicate functions into the trait implementation), but those seemed like they should wait for another PR. This PR contains roughly the minimal amount of refactoring necessary to switch over to the MetadataProvider trait.

Fixes coreos/bugs#2362.

crawford · 2018-06-18T22:15:45Z

src/lib.rs

 /// `fetch_metadata` is the generic, top-level function that is used by the main
 /// function to fetch metadata. The configured provider is passed in and this
 /// function dispatches the call to the correct provider-specific fetch function
-pub fn fetch_metadata(provider: &str) -> Result<Metadata> {
+pub fn fetch_metadata(provider: &str) -> Result<Box<providers::MetadataProvider>> {


Why is the Box needed?

Since we're returning multiple different concrete types that all implement MetadataProvider, impl Trait can't be used and we need a Box to create an actual trait object.

I thought that was the whole point of impl Trait...

Unfortunately not. From the RFC:

If a function returns impl Trait, its body can return values of any type that implements Trait, but all return values need to be of the same type.

@csssuf for my own reference: pushing the -> impl Trait one more level down too (i.e. returned by new()), would that also not work?

Nope. The compiler needs to be able to figure out static dispatch to use impl Trait, so there's no way to trick it into doing the thing we want here.

https://www.reddit.com/r/rust/comments/8sqiin/newbie_question_why_is_the_impl_and_dyn_in_the/ is also useful on this topic (there's various others too)

With rustc beta today for example, one can write:

diff --git a/src/lib.rs b/src/lib.rs index 0f80d1c..2ec3af3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -90,7 +90,7 @@ macro_rules! box_result { /// `fetch_metadata` is the generic, top-level function that is used by the main /// function to fetch metadata. The configured provider is passed in and this /// function dispatches the call to the correct provider-specific fetch function -pub fn fetch_metadata(provider: &str) -> Result<Box<providers::MetadataProvider>> { +pub fn fetch_metadata(provider: &str) -> Result<Box<dyn providers::MetadataProvider>> { match provider { "azure" => box_result!(azure::Azure::new()), "cloudstack-metadata" => box_result!(cloudstack::network::CloudstackNetwork::new()),

Then it's explicitly clear the goal is dynamic dispatch.

BTW I love how much discussion there's been over a single heap allocation 😄

without having looked into it at all, I wonder if it would be possible to simultaneously use dyn and impl Trait to do what we want without a heap allocation? mostly curious, the current version is totally fine by me too.

No, they're mutually exclusive. We could avoid the Box by using an enum as the function's return type, and essentially provide our own dynamic dispatch by implementing the trait for the enum, but IMO that's even less clean than simply returning a Boxed trait object.

cgwalters · 2018-06-20T19:01:51Z

This is the first time I've ever looked at this code, and my Rust is beginner-intermediate-ish...so take my comments with that in mind.

Overall, makes a lot of sense. A personal style thing, I would tend to use a single commit that adds and deletes code at once, but eh.

I was about to ask about tests, but I found they exist just for EC2. I assume Kola will cover other cases?

arithx · 2018-06-20T19:05:08Z

@cgwalters kola does indeed cover a number of cases. The tests are located here (also some fields are used in other individual tests).

lucab · 2018-06-20T20:30:47Z

Fully testing for this basically means reproducing all API and quirks of all supported cloud providers, so the mocking is mostly assuring that it doesn't break at a very coarse level. It only covers EC2 mostly due to time limitations, and because real/subtle regressions will hopefully break the integration CI on the relevant cloud provider.

lucab · 2018-06-21T19:32:20Z

src/providers/azure/mod.rs

+impl MetadataProvider for Azure {
+    fn attributes(&self) -> Result<HashMap<String, String>> {
+        let attributes = self.get_attributes()?;
+        let mut out = HashMap::new();


Here and in all the other providers too, what about with_capacity(2) instead?

lucab · 2018-06-21T19:36:08Z

src/providers/cloudstack/configdrive.rs

+impl ::std::ops::Drop for ConfigDrive {
+    fn drop(&mut self) {
+        if self.temp_dir.is_some() {
+            ConfigDrive::unmount(&self.path).unwrap();


I'm not sure we should panic here. (This is carried-over code, feel free to defer this)

lucab · 2018-06-21T19:46:52Z

@csssuf I did a cursory review of this, and it seems overall fine. Most of this is just moving functions around, I didn't dig into those.

sdemos

lgtm after we get positive tests on all the major cloud providers (it might need a rebase on master, shouldn't be any conflicts though)

sdemos · 2018-06-22T19:09:40Z

src/providers/gce/mod.rs

+    fn attributes(&self) -> Result<HashMap<String, String>> {
+        let mut out = HashMap::with_capacity(3);
+
+        let add_value = |map: &mut HashMap<_, _>, key: &str, name| -> Result<()> {


Unfortunate that there is so much duplication with the add_value closure, but I'm not sure if there is a better way to do it after thinking through some of the alternatives.

Add a new trait `MetadataProvider` with functions for fetching each type of metadata coreos-metadata knows about, and helper functions for writing metadata to disk.

Add an implementation of `MetadataProvider` for each existing metadata provider.

Now that we expose an Ec2Provider struct in providers/ec2, use it in the ec2 mock tests.

Instead of fetching all possible metadata in fetch_metadata(), return a trait object which provides the same functions for writing to disk as the original `Metadata` struct, but only fetches metadata when necessary.

Drop the unused fetch_metadata() function from each provider, as the `MetadataProvider` trait is now used in their place.

`Metadata` is now unused after the switch to `MetadataProvider`, so drop it.

csssuf · 2018-06-22T23:18:23Z

Verified that the refactor's output matches the current release's (but without the unnecessary requests) on:

EC2
Azure
DigitalOcean
Packet
GCE

csssuf requested a review from sdemos June 18, 2018 22:10

csssuf force-pushed the partial-metadata branch from 6414773 to 4b38706 Compare June 18, 2018 22:14

crawford reviewed Jun 18, 2018

View reviewed changes

csssuf force-pushed the partial-metadata branch 4 times, most recently from c52250e to 89485a9 Compare June 18, 2018 22:40

csssuf force-pushed the partial-metadata branch from 89485a9 to 7729afb Compare June 20, 2018 20:18

lucab reviewed Jun 21, 2018

View reviewed changes

csssuf force-pushed the partial-metadata branch from 7729afb to 0538c5e Compare June 21, 2018 21:58

lucab approved these changes Jun 22, 2018

View reviewed changes

sdemos approved these changes Jun 22, 2018

View reviewed changes

csssuf added 6 commits June 22, 2018 16:16

providers: add MetadataProvider trait

b5a5f80

Add a new trait `MetadataProvider` with functions for fetching each type of metadata coreos-metadata knows about, and helper functions for writing metadata to disk.

providers/*: implement MetadataProvider trait

f071969

Add an implementation of `MetadataProvider` for each existing metadata provider.

providers/ec2: use Ec2Provider in mock tests

445830a

Now that we expose an Ec2Provider struct in providers/ec2, use it in the ec2 mock tests.

lib: use MetadataProvider trait for fetch_metadata

ced7381

Instead of fetching all possible metadata in fetch_metadata(), return a trait object which provides the same functions for writing to disk as the original `Metadata` struct, but only fetches metadata when necessary.

providers/*: drop fetch_metadata() functions

920d929

Drop the unused fetch_metadata() function from each provider, as the `MetadataProvider` trait is now used in their place.

metadata: drop metadata module

1d6629f

`Metadata` is now unused after the switch to `MetadataProvider`, so drop it.

csssuf force-pushed the partial-metadata branch from 0538c5e to 1d6629f Compare June 22, 2018 23:16

csssuf mentioned this pull request Jun 26, 2018

coreos-metadata fails as non-root user on Azure coreos/bugs#2468

Closed

csssuf merged commit 4c0ca16 into coreos:master Jul 2, 2018

csssuf deleted the partial-metadata branch July 2, 2018 18:30

csssuf mentioned this pull request Jul 2, 2018

version: release v2.0.0 #93

Merged

lucab mentioned this pull request Aug 3, 2018

azure/crypto: remove all unwraps #110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor metadata fetching into `MetadataProvider` trait #88

Refactor metadata fetching into `MetadataProvider` trait #88

csssuf commented Jun 18, 2018

crawford Jun 18, 2018

csssuf Jun 18, 2018

crawford Jun 18, 2018

csssuf Jun 18, 2018

lucab Jun 19, 2018

csssuf Jun 19, 2018 •

edited

Loading

cgwalters Jun 21, 2018

sdemos Jun 22, 2018

csssuf Jun 22, 2018

cgwalters commented Jun 20, 2018

arithx commented Jun 20, 2018

lucab commented Jun 20, 2018

lucab Jun 21, 2018

lucab Jun 21, 2018

lucab commented Jun 21, 2018

sdemos left a comment

sdemos Jun 22, 2018

csssuf commented Jun 22, 2018

Refactor metadata fetching into MetadataProvider trait #88

Refactor metadata fetching into MetadataProvider trait #88

Conversation

csssuf commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csssuf Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Jun 20, 2018

arithx commented Jun 20, 2018

lucab commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Jun 21, 2018

sdemos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csssuf commented Jun 22, 2018

Refactor metadata fetching into `MetadataProvider` trait #88

Refactor metadata fetching into `MetadataProvider` trait #88

csssuf Jun 19, 2018 •

edited

Loading