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

azure-image: switch to use the common make-disk-image.nix #25197

Merged
merged 1 commit into from Apr 25, 2017

Conversation

copumpkin
Copy link
Member

@copumpkin copumpkin commented Apr 25, 2017

Motivation for this change

More consistency. See #25165 and #25166 for more context.

Two things:

  1. I'd appreciate someone who uses Azure to give this a quick try. @rbvermaa @Phreedom @colemickens perhaps?
  2. There's some pretty arcane logic around size computation right now that doesn't make much sense to me, but isn't commented. I took it out, but I imagine I'll probably have to add it back. I tried perusing the Azure docs but couldn't find anything particularly unusual there. If one of you could elaborate on why it's there, it would help me understand and carry over to the common system.

cc @domenkozar

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@copumpkin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Phreedom, @rbvermaa and @dezgeg to be potential reviewers.

@copumpkin copumpkin added this to Improve consistency in Amazing images Apr 25, 2017
@colemickens
Copy link
Member

I've love to take a look (especially since upstream qemu-img now works fine with Azure) but I simply do not have the time. Hopefully @Phreedom can take a look potentially.

@copumpkin
Copy link
Member Author

@Phreedom doesn't seem to have time but mentioned on IRC that https://bugs.launchpad.net/qemu/+bug/1490611 was the source of the weird size calculations, so a recent version of qemu should be enough to fix it.

I'll see if I can get myself an Azure account in the next few days to spin up an image before I merge this, if someone else can't test it first.

@fadenb
Copy link
Contributor

fadenb commented Apr 25, 2017

Will test it sometime later today.
Regarding the size issues: The statement of the azure requirements on launchpad are not the whole truth. I have experienced that images were accepted even if they were not on the mentioned boundary and other images with the exact size required as stated by azure documentation that were not accepted. Unfortunately Microsoft engineers were unable to figure out what was the real reason behind this behavior (despite being a severity level A support incident). Nonetheless the old sizing calculation worked fine for me for several hundred images.

@copumpkin
Copy link
Member Author

@fadenb ouch, that doesn't make me feel much better about removing it, but at some point I guess we'll need to test the straightforward thing if we don't want to be stuck forever with arcane arithmetic that nobody understands 😄 note that I still haven't upgraded to the latest qemu, so it might still be odd until we do that. I'll be at my other computer this evening and can make that change then, but if you end up testing before that, can you make sure to check with the latest qemu?

@fadenb
Copy link
Contributor

fadenb commented Apr 25, 2017

PR as it is:
Test successfull (tested in region US West)

PR but with current qemu:
"message": "The VHD for disk 'nixosVMosdisk' with blob https://mfwestusimgtest.blob.core.windows.net/offloader-images/nixos-mayflower-osdisk-vm0.vhd has an unsupported virtual size of 30720.375 MB. The size must be a whole number in (MBs)."

PR with current qemu and force_size option:
Test successfull (tested in region US West)

@copumpkin
Copy link
Member Author

@fadenb thanks for all the tests. What do you think I should do? I'm tempted to merge this PR as it is if it works, then maybe let @colemickens or some other Azure expert deal with the the qemu and qemu-img parameter details. Seems unfortunate that it's so picky about sizes 😦

@fadenb
Copy link
Contributor

fadenb commented Apr 25, 2017

PR as it is seems reasonable safe to merge so I would say go for it.

@copumpkin copumpkin merged commit 1ec8afd into NixOS:master Apr 25, 2017
@copumpkin
Copy link
Member Author

Thanks!

@colemickens
Copy link
Member

Sorry, haven't reviewed the commits or anything. The only trick here should b.... use a new enough version of qemu and pass -o force_size so that it aligns on the correct boundary. I'd be shocked to hear that azure is allowing mis-rounded VHDs. Last time that was permitted by the platform it had some not good effects.

When I did this hacky stuff previously, I was just adding an extra qemu package that didn't have the regression and was using it to build the Azure image. Now, you should be able to just remove the custom qemu version and just make sure the correct -o force_size (or whatever, from the Launchpad Issue) is present on the call to qemu-img to convert.

@copumpkin
Copy link
Member Author

@colemickens alright, so I guess in theory we should emulate that rounding logic in our (eventual) VM test for this image so we don't break it in future? Either way, @fadenb says the current state works, so I'm inclined to leave out all the arcane arithmetic until the issue presents itself again.

The change you're describing sounds straightforward but I'm reluctant to make changes that I can't test, so I'm inclined to leave it to someone who can test it.

@copumpkin
Copy link
Member Author

@colemickens To put it differently, is there a simple open source tool/incantation we can run against a VHD file to see if Azure will consider it to have a non-integer size?

I see some people in the launchpad issue using stat on the raw image file, but is that the actual issue, or is it complaining about the size of the volume encoded inside the image file?

@colemickens
Copy link
Member

In theory, you really shouldn't need to emulate anything... you should just use the latest qemu and use -o force_size with the vpc type. That will produce an Azure-compatible VHD.

And yes, I think the stat verification are valid... this is the key post: https://bugs.launchpad.net/qemu/+bug/1490611/comments/36

I'd say I'd try to test this this weekend, but my list of extra things to test for random projects is already too long.

@fadenb fadenb mentioned this pull request May 10, 2017
7 tasks
@fadenb
Copy link
Contributor

fadenb commented May 19, 2017

As this PR is now merged for some time I used the new code several times to build azure images.
I do not have proper time measurements from before but image creation for me is now at least double the time it took before the changes. Anyone else noticing this?

@copumpkin
Copy link
Member Author

@fadenb sorry about that! I can't test it easily but if you can provide some sort of measurement I'd be very curious about it. Are you generating images from physical hardware or on Azure itself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Amazing images
Improve consistency
Development

Successfully merging this pull request may close these issues.

None yet

4 participants