Add support for Gen2 VM resource disks #1654

trstringer · 2019-10-03T16:53:06Z

Description

The current implementation for resource disks expects (requires) that the device ID has a prefix of 00000000-0001. For Gen2 VMs, this is not the case. They have a device ID of f8b3781a-1e82-4818-a1c3-63d806ec15bb. Because of this, prior to this PR waagent does not find the resource disk and therefore doesn't mount it.

Here is the output of lsblk:

    NAME   MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
    sda      8:0    0   30G  0 disk
    ├─sda1   8:1    0    2M  0 part
    ├─sda2   8:2    0  512M  0 part /boot/efi
    ├─sda3   8:3    0    1G  0 part /boot
    └─sda4   8:4    0 28.5G  0 part /
    sdb      8:16   0   64G  0 disk
    └─sdb1   8:17   0   64G  0 part
    sr0     11:0    1  628K  0 rom

As you can see, there is no mountpoint for sdb1.

This PR fixes that by looking first for a Gen1 VM resource disk device, and then trying a Gen2 VM resource disk device.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

This change is

azurelinuxagent/common/osutil/default.py

trstringer · 2019-10-03T21:51:58Z

@jasonzio can I get a review as well? Thanks!

codecov · 2019-10-03T21:53:39Z

Codecov Report

Merging #1654 into develop will increase coverage by 0.18%.
The diff coverage is 73.68%.

@@             Coverage Diff             @@
##           develop    #1654      +/-   ##
===========================================
+ Coverage    66.86%   67.05%   +0.18%     
===========================================
  Files           78       78              
  Lines        11242    11272      +30     
  Branches      1572     1576       +4     
===========================================
+ Hits          7517     7558      +41     
+ Misses        3402     3383      -19     
- Partials       323      331       +8

Impacted Files	Coverage Δ
azurelinuxagent/common/osutil/debian.py	`79.54% <100%> (ø)`	⬆️
azurelinuxagent/common/osutil/factory.py	`97.29% <100%> (ø)`	⬆️
azurelinuxagent/common/osutil/default.py	`60.02% <65.51%> (+1.75%)`	⬆️
azurelinuxagent/ga/exthandlers.py	`84.05% <0%> (-0.02%)`	⬇️
azurelinuxagent/common/protocol/imds.py	`96.29% <0%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eddbf1...5808390. Read the comment docs.

jasonzio

Functionally adequate, but some refactoring, as described, would improve long-term maintainability.

azurelinuxagent/common/osutil/default.py

azurelinuxagent/common/osutil/factory.py

vrdmr · 2019-10-04T01:15:59Z

@trstringer - FYI - The Py2 builds are failing. Could you please take a look?

trstringer · 2019-10-04T19:46:33Z

@vrdmr Yep I'll check that out and get it fixed up.

azurelinuxagent/common/osutil/debian.py

azurelinuxagent/common/osutil/default.py

jasonzio

More nits.

jasonzio

Looks good now.

trstringer · 2019-10-14T14:35:23Z

@anhvoms could I get a final review on this?

anhvoms · 2019-10-14T15:56:47Z

azurelinuxagent/common/osutil/default.py

+                yield vmbus, guid
+
+    @staticmethod
+    def search_for_resource_disk(gen1_device_prefix, gen2_device_id):


def search_for_resource_disk(gen1_device_prefix, gen2_device_id): [](start = 4, length = 65)

Is there an easy way to detect if this is a Gen2VM, if so would it make more sense to refactor this into a flow where we detect whether this is a Gen2 or Gen1, if it's Gen2 there is a different search_for_resource_disk function (sort of like function pointers, one for Gen1, one for Gen2).

Good question, I'll see if I can find that answer.

Not sure you still need it, just for your reference.
if [ -d /sys/firmware/efi/ ]; then
os_GENERATION=2
else
os_GENERATION=1
fi

anhvoms

trstringer · 2019-10-17T18:22:28Z

@narrieta let me know if you anything else from my end to get this PR merged in. Thanks!

narrieta

LGTM; running automation, will merge after that.

narrieta · 2019-10-21T18:14:58Z

Automation OK

vrdmr

Reviewed 1 of 1 files at r3, 3 of 3 files at r5, 1 of 1 files at r9.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @anhvoms, @jasonzio, @larohra, @pgombar, and @trstringer)

trstringer added 2 commits September 26, 2019 12:41

(wip) add gen2 resource disk support

c6902ad

remove device names

d1433a6

trstringer requested review from larohra, narrieta, pgombar and vrdmr as code owners October 3, 2019 16:53

vrdmr suggested changes Oct 3, 2019

View reviewed changes

azurelinuxagent/common/osutil/default.py Outdated Show resolved Hide resolved

azurelinuxagent/common/osutil/default.py Outdated Show resolved Hide resolved

trstringer added 3 commits October 3, 2019 15:40

move device id constant

ed7987f

add debian testing and unstable support

13d25e8

add tests for resource disk discovery

a6b007a

trstringer requested a review from vrdmr October 3, 2019 21:52

remove unnecessary patch for test

f699d83

jasonzio requested changes Oct 3, 2019

View reviewed changes

azurelinuxagent/common/osutil/default.py Outdated Show resolved Hide resolved

azurelinuxagent/common/osutil/factory.py Outdated Show resolved Hide resolved

trstringer added 2 commits October 3, 2019 20:31

refactor checking for both gen devices

910fcea

rename debian util classes to reflect current

a233754

trstringer requested a review from jasonzio October 4, 2019 00:53

jasonzio reviewed Oct 4, 2019

View reviewed changes

azurelinuxagent/common/osutil/debian.py Show resolved Hide resolved

jasonzio reviewed Oct 4, 2019

View reviewed changes

azurelinuxagent/common/osutil/debian.py Show resolved Hide resolved

trstringer added 2 commits October 4, 2019 18:27

catch file not found errors for older python versions

334f827

refactor out filesystem search for device

5ea46df

trstringer requested a review from jasonzio October 7, 2019 17:36

jasonzio reviewed Oct 7, 2019

View reviewed changes

azurelinuxagent/common/osutil/default.py Outdated Show resolved Hide resolved

fix typo

683c6d3

jasonzio reviewed Oct 7, 2019

View reviewed changes

azurelinuxagent/common/osutil/default.py Outdated Show resolved Hide resolved

jasonzio reviewed Oct 7, 2019

View reviewed changes

azurelinuxagent/common/osutil/default.py Outdated Show resolved Hide resolved

jasonzio requested changes Oct 7, 2019

View reviewed changes

trstringer added 2 commits October 10, 2019 13:34

remove newline from input string

c2c8a06

refactor device enumeration to own method

1c77533

trstringer changed the title ~~Add support for Gen2 VM resource disks~~ (WIP) Add support for Gen2 VM resource disks Oct 10, 2019

fix bug with breaking from loop

5808390

trstringer changed the title ~~(WIP) Add support for Gen2 VM resource disks~~ Add support for Gen2 VM resource disks Oct 10, 2019

trstringer requested a review from jasonzio October 10, 2019 22:21

jasonzio approved these changes Oct 10, 2019

View reviewed changes

anhvoms reviewed Oct 14, 2019

View reviewed changes

anhvoms approved these changes Oct 14, 2019

View reviewed changes

narrieta reviewed Oct 18, 2019

View reviewed changes

narrieta approved these changes Oct 21, 2019

View reviewed changes

vrdmr reviewed Oct 21, 2019

View reviewed changes

vrdmr approved these changes Oct 21, 2019

View reviewed changes

narrieta merged commit 93ecdc4 into Azure:develop Oct 21, 2019

trstringer mentioned this pull request Oct 23, 2019

[BUG] waagent doesn't mount temporary storage on Gen2 VMs #1645

Closed

vrdmr mentioned this pull request Oct 24, 2019

Support ephemeral disk handling on Gen2 VMs #1362

Closed

trstringer mentioned this pull request Nov 18, 2019

Unable to detect disk topology resource disk of 8TB of ram #1711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Gen2 VM resource disks #1654

Add support for Gen2 VM resource disks #1654

trstringer commented Oct 3, 2019 •

edited

trstringer commented Oct 3, 2019

codecov bot commented Oct 3, 2019 •

edited

jasonzio left a comment

vrdmr commented Oct 4, 2019

trstringer commented Oct 4, 2019

jasonzio left a comment

jasonzio left a comment

trstringer commented Oct 14, 2019

anhvoms Oct 14, 2019

trstringer Oct 14, 2019

lixuemin2016 Feb 11, 2020

anhvoms left a comment

trstringer commented Oct 17, 2019

narrieta left a comment

narrieta commented Oct 21, 2019

vrdmr left a comment

Add support for Gen2 VM resource disks #1654

Add support for Gen2 VM resource disks #1654

Conversation

trstringer commented Oct 3, 2019 • edited

Description

PR information

Quality of Code and Contribution Guidelines

trstringer commented Oct 3, 2019

codecov bot commented Oct 3, 2019 • edited

Codecov Report

jasonzio left a comment

Choose a reason for hiding this comment

vrdmr commented Oct 4, 2019

trstringer commented Oct 4, 2019

jasonzio left a comment

Choose a reason for hiding this comment

jasonzio left a comment

Choose a reason for hiding this comment

trstringer commented Oct 14, 2019

anhvoms Oct 14, 2019

Choose a reason for hiding this comment

trstringer Oct 14, 2019

Choose a reason for hiding this comment

lixuemin2016 Feb 11, 2020

Choose a reason for hiding this comment

anhvoms left a comment

Choose a reason for hiding this comment

trstringer commented Oct 17, 2019

narrieta left a comment

Choose a reason for hiding this comment

narrieta commented Oct 21, 2019

vrdmr left a comment

Choose a reason for hiding this comment

trstringer commented Oct 3, 2019 •

edited

codecov bot commented Oct 3, 2019 •

edited