[filesystem] Convert windows to filesystem2 #1267
Conversation
|
[moving this comment over from the other PR, I'll delete it from there] OK, I need help from @tas50 or someone. I sorta figured out what's up with the windows crap... It seems like sometimes The lack of my debug output in the failing tests made me think it was failing to create a Mash, but after running down that rabbit hole for a while, that doesn't seem to be the case. On my windows VM I cannot repro it using the exact same data: But as you can see from the appveyer output: and: But then LATER: So it works here. WUT? |
jaymzh
force-pushed
the
windows_fs2
branch
2 times, most recently
from
31ba78b
to
bc9ba5c
Compare
|
@jaymzh You can rebase this now |
|
rebased. |
|
CC @tas50 |
|
OK I think I found the problem. Fix coming that generates this: |
jaymzh
force-pushed
the
windows_fs2
branch
2 times, most recently
from
1b6ef24
to
9a3ffe6
Compare
|
OK @tas50 - so I hadn't looked at the backcompat data and it was broke, but is now fixed. However, I still ahve the same problem wiht the specs, I could use your help with. |
|
@jaymzh can you rebase this on master. We killed off appveyor. |
|
Yes, but it's going to be merge hell since the file was moved and I have to manually add changes that went in since then. So if I do the work, please review it in a timely manner this time around? :) |
|
OK that actually wasn't bad, only 2 minor changes from you. :) Unfortunately I no longer have easy access to windows VMs... but the code, as I showed, works (assuming rebase didn't break it), the tests, on the other hand I still don't know why they're being odd. |
|
@tas50 going on almost 4 months since I rebased so you could review this and 11 months since I updated this to work right on all windows boxes I could find. |
|
@jaymzh , I see a few things to failing these test cases. First and foremost those methods needs to be extracted out from collect(:windows) block. And we need to add "device" property in test cases. Additionally some code changes ("key" selection in different properties) are also required. Concern is for few devices, "volumename" is coming as empty ! And due to which we are getting some inappropriate results. Could you please re-check this logic once. Output: PS> bundle exec ohai filesystem
{
"": {
"kb_size": 105981550,
"kb_available": 76303601,
"kb_used": 29677949,
"percent_used": 28,
"fs_type": "ntfs",
"volume_name": "",
"encryption_status": "FullyDecrypted",
"mount": "C:"
},
"vbox_gas_6.0.14": {
"kb_size": 77195,
"kb_available": 0,
"kb_used": 77195,
"percent_used": 100,
"fs_type": "cdfs",
"volume_name": "vbox_gas_6.0.14",
"mount": "D:"
}
} |
|
@Nimesh-Msys - Thanks for looking at this. I... will need to track down a windows machine somewhere to develop this on again. I don't suppose you know of a windows shell service that's cheap or free? I don't quite understand why you moving those methods out might change the Also, for the example you give yeah, it's a typo, line 256 is: but should be: I'll fix that. |
|
Oh... also, is there a reason MsysTechnologiesllc@f93f3e5 isn't upstream? |
|
@Nimesh-Msys I pulled in your patch as well as fixed that bug you found, and also rebased |
jaymzh
force-pushed
the
windows_fs2
branch
8 times, most recently
from
2e2f58d
to
d8ae0b2
Compare
There was a problem hiding this comment.
Oh... also, is there a reason MsysTechnologiesllc@f93f3e5 isn't upstream?
It's a forked repo for internal purpose, Single direct upstream is chef/ohai.
I don't quite understand why you moving those methods out might change the logical_properties from sometimes inexplicably returning an array in tests to returning a Mash as expected.
In case of any failure, or cases where we are unable to retrieve some information(Exceptions), we have managed to continuing working with Mash. So that there won't be any dependency on multiple utilities that we are using to extract core information and then merging that in our results. When that method was within "collect_data(:windows)" block, it was actually not present for plugin's class of our test cases. This leads to an exception, and we were getting Mash as a result. One workaround for testing in this condition is by executing plugin.run.
Could you please cover some test cases. Few expected ones are mentioned below, and then we are good.
| expect(data[nil].symbolize_keys.keys).to eq(logical_props) | ||
| expect(data[nil]["kb_used"]).to eq(0) | ||
| expect(data[nil]["fs_type"]).to be_empty | ||
| expect(data[","].symbolize_keys.keys).to eq(logical_props) |
There was a problem hiding this comment.
If I'm right, these test cases I believe are for :filesystem2["by_pair"] ? The way its been done for other OS in PR#1266.
If yes, then could you please add the cases for both "filesystem" and "filesystem2" as the output which is not expected "by_pair" is not captured here.
There was a problem hiding this comment.
Sorta - we're not testing the output of the plugin here, but instead logical_properties... but yes I will the appropriate tests
| end | ||
|
|
||
| it "Refines required logical properties out of given instance" do | ||
| data = plugin.logical_properties(disks) | ||
| expect(data["C:"].symbolize_keys.keys).to eq(logical_props) | ||
| expect(data["D:"].symbolize_keys.keys).to eq(logical_props) | ||
| expect(data[",C:"].symbolize_keys.keys).to eq(logical_props) |
There was a problem hiding this comment.
And we can have multiple contexts, wherein when we have volume_name then That information is also present in this key, and this is the case when volume_name is not present.
| expect(plugin.merge_info([])).to be_empty | ||
| end | ||
| let(:logical_info2) { { ",drive1" => { "mount" => "drive1", "o" => 10, "p" => "test1" } } } | ||
| let(:encryption_info2) { { "drive2" => { "q" => 10, "r" => "test1" } } } |
There was a problem hiding this comment.
A small concern here, for filesystem, & filesystem2, the nature of keys for logical and encryption should be maintained. Like for fs: "drive1", "drive2" and for fs2: ",drive1", ",drive2", and so on. These methods are processing the core information and then we are merging this info to display the ohai mannered output. Symmetry maintenance would be helpful for further extension.
There was a problem hiding this comment.
I don't think so... that doesn't work anymore, which is why I broke that apart. AIUI, encryption_info only returns information on the mountpoint (i.e. drive letter), not volume. So imagine that you had:
MyCoolVolume mounted ad E:
EncryptionInfo doesn't know what MyCoolVolume is because the encryption commands don't return it, they only return the drive letter (E:)
So in order to safely build data we build by pair, so we build a key like MyCoolVolume,E:. However, if we were to try to pretend the encryption_info could be the same, then we'd build ,E:... and that doesn't match. Pretending it does is a lie. To "merge" these, we'd always ignore the first, part, look at the second part and then do exactly what I do above, look at anything that has a mount of E:.
Meanwhile if we don't have a VolumeName then logical_info can of course return ,E:, which has a specific meaning. It means a null-named volume is mounted at E:. But thats not what encryption_info has ever been able to tell you - all it can tell you is some encryption info about whatever happens to be mounted at E:.
Therefore trying to make the keys the same is impossible, and making them look the same is misleading and confusing, so I very specifically made it clear they were saying different things.
| property[:mount] = mount | ||
| property[:fs_type] = disk["filesystem"].to_s.downcase | ||
| property[:volume_name] = device | ||
| property[:device] = device |
There was a problem hiding this comment.
The property :device should be added in Yard formatted comments above the method.
jaymzh
force-pushed
the
windows_fs2
branch
2 times, most recently
from
28b861b
to
5d59e13
Compare
|
OK @Nimesh-Msys - yard docs updated, tons of tests added. |
|
@Nimesh-Msys Removed debug. Update commit message to remove comments about tests not passing and other silly stuff. Updated PR description accordingly. BTW - why are there the two buildkite tests that don't do anything and report failure? :( |
@tas50 might be able to help you out on this. |
|
@Nimesh-Msys - I'll bug @tas50 about it during the community meeting in the morning. I'm sure he'll be happy, I've been bugging him about this PR for 13 months.:) In the meantime, if you're happy with it, wanna approve it? |
|
According to today's Developer Meeting: So @tas50 is on it ... modulo some driving today. Hopefully we can get this merged this week and finally be done with this PR. |
Windows doesn't follow quite the same paradigms as Unix, but there's enough in common that we can still map most things. Signed-off-by: Phil Dibowitz <phil@ipom.com>
|
@Nimesh-Msys / @tas50 - anything left here? Would really like to get this wrapped up. |
|
@tas50 post-thanksgiving friendly i-think-everyone-is-happy-and-the-tests-pass-and-it's-been-14-months ping :) |
|
So this seems fine as an additional option in filesystem2, but I wouldn't support making this the default on Windows systems. It's going to require a lot of folks to rework their cookbooks and I'm not sure the benefit is enough for that. We've introduced a lot of artificial roadblocks to upgrades over the years that have resulted in many users just sticking with older releases like Chef 12 to avoid upgrade pains. I'd really like to avoid those sort of backwards compatibility breaks unless we need to. filesystem2 is there for anyone that wants drives by pair or label. |
|
I think it's far, far, far, far worse to have |
|
I'll echo what @jaymzh is saying. In many of our API cookbooks we reference We already had to do the conversion for Continuing to have a split brain definition for one remaining OS makes the API fragile and more prone to causing problems. We should be able to trust that the |
|
While it's trivial to use the new mountpoint logic now it's a hurdle to upgrades and historically changing logic in Ohai has prevented users from upgrading to the latest version of Chef Infra. A very large percentage of our userbase is still on Chef 12 due to the difficulty in upgrading Chef versions. A "trivial" change to ohai attributes means that a customer needs to build logic into their cookbooks that checks Chef versions and uses different attributes based on the client they're running. That becomes a non-trivial change for complex Chef installations. In the end, we're doing this to provide the ability to sort of labels, which is somewhat meaningless in Windows as anyone can change them to anything. I get standardizing attributes, but here we've bent Windows to match *nix in a way that provides limited utility at best. In doing so we're requiring a large number of users to untake an attribute migration. I don't see that as benefiting our userbase. Different operating systems are always going to present data in Ohai differently and that's by design. We aim to standardize attributes where it makes sense, but standardizing attributes on differing operating systems often results in a less than ideal API. |
|
I understand what you're saying. @NaomiReeves and I went through a LOT of work getting Facebook off of Chef 12 (and her way more than I). I sympathize, I do... but this is part of the reason I'm urging you to do Windows along with the others as you plan on doing in #1271. Because people WILL have to migrate and it becomes the difference between something like this to get to the intermediate version of chef: option 1 - when all things convertAnd then this once you're there: and eventually: option 2 - when they don'tORRRRR I have to crazytown like this: And then eventually: and then eventually: and never getting past that ugliness? I understanding that "hey $customer your code will never ever have to change" is really attractive, but you're making everyone's code (and your own uglier), you're increasing support costs significantly, you're making the documentation much harder to generate, you're making expectations much less obvious. Leaving them separate trades a small amount of short-term gain for a much much bigger long-term loss. |
Summary: Converting the `fb_systemd` cookbook over to the `ohai_fs_ver` node method. In chef 13, the filesystem ohai plugin was replaced with the filesystem2 plugin, and the `filesystem2` data is written to both the `filesystem2` and `filesystem` apis. All uses of the old `filesystem` api were converted to the `filesystem2` api before I upgraded the fleet to chef 13 (tracked in T28228267). In order to upgrade to Chef 14+, we need to remove the references to the `filesystem2` api. However, there are some outlier operating systems that missed the conversion and so we're going to use a node method to detect what api is available and use that one so we can continue to support this transition for those other platforms. official deprecation: https://docs.chef.io/deprecations_ohai_filesystem.html PR to covert BSD: chef/ohai#1181 PR to convert the rest of Unix (solaris, aix): chef/ohai#1266 PR to convert Windows: chef/ohai#1267 Reviewed By: davide125 Differential Revision: D19466401 fbshipit-source-id: 9c9a1e7838e8a9e969c00ef611dce03f3dd9249d
Summary: Well, until chef/ohai#1267 and I'm not backporting that just yet, so we'll actually test for the existence of `node['whatever_filesystem_ohai_version']['by_mountpoint']` and if it exists... continue Reviewed By: davide125 Differential Revision: D19467985 fbshipit-source-id: a6df657de74d3e2eae653624aa422550c4ef26b8
Summary: Converting the `fb_swap` cookbook over to the `filesystem_data` node method. In chef 13, the filesystem ohai plugin was replaced with the filesystem2 plugin, and the `filesystem2` data is written to both the `filesystem2` and `filesystem` apis. All uses of the old `filesystem` api were converted to the `filesystem2` api before I upgraded the fleet to chef 13 (tracked in T28228267). In order to upgrade to Chef 14+, we need to remove the references to the `filesystem2` api. However, there are some outlier operating systems that missed the conversion and so we're going to use a node method to detect what api is available and use that one so we can continue to support this transition for those other platforms. official deprecation: https://docs.chef.io/deprecations_ohai_filesystem.html PR to covert BSD: chef/ohai#1181 PR to convert the rest of Unix (solaris, aix): chef/ohai#1266 PR to convert Windows: chef/ohai#1267 Reviewed By: davide125 Differential Revision: D20610482 fbshipit-source-id: 01e9eb9073c226d18683ec0b0c806472f7e27c49
Summary: Converting the `fb_storage` cookbook over to the `filesystem_data` node method. In chef 13, the `filesystem` ohai plugin was replaced with the `filesystem2` plugin, and the `filesystem2` data is written to both the `filesystem2` and `filesystem` apis. All uses of the old `filesystem` api were converted to the `filesystem2` api before I upgraded the fleet to chef 13 (tracked in T28228267). In order to upgrade to Chef 14+, we need to remove the references to the `filesystem2` api. However, there are some outlier operating systems that missed the conversion and so we're going to use a node method to detect what api is available and use that one so we can continue to support this transition for those other platforms. official deprecation: https://docs.chef.io/deprecations_ohai_filesystem.html PR to covert BSD: chef/ohai#1181 PR to convert the rest of Unix (solaris, aix): chef/ohai#1266 PR to convert Windows: chef/ohai#1267 Reviewed By: davide125 Differential Revision: D20634208 fbshipit-source-id: 90d3d3a00763d4e74fbda648c57a0abd6b649656
Summary: Converting the `fb_helpers` cookbook over to the `filesystem_data` node method. In chef 13, the `filesystem` ohai plugin was replaced with the `filesystem2` plugin, and the `filesystem2` data is written to both the `filesystem2` and `filesystem` apis. All uses of the old `filesystem` api were converted to the `filesystem2` api before I upgraded the fleet to chef 13 (tracked in T28228267). In order to upgrade to Chef 14+, we need to remove the references to the `filesystem2` api. However, there are some outlier operating systems that missed the conversion and so we're going to use a node method to detect what api is available and use that one so we can continue to support this transition for those other platforms. I'm also adding in some checks to validate that the various keys we are using in some of these methods actually exist so we don't end up with the `#<NoMethodError: undefined method `[]' for nil:NilClass>` issues official deprecation: https://docs.chef.io/deprecations_ohai_filesystem.html PR to covert BSD: chef/ohai#1181 PR to convert the rest of Unix (solaris, aix): chef/ohai#1266 PR to convert Windows: chef/ohai#1267 Reviewed By: joshuamiller01 Differential Revision: D21030821 fbshipit-source-id: 5457a9cdc35c55a33f43413ad7f5cf20b8ea593d
Summary: Converting the `fb_helpers` cookbook over to the `filesystem_data` node method. In chef 13, the `filesystem` ohai plugin was replaced with the `filesystem2` plugin, and the `filesystem2` data is written to both the `filesystem2` and `filesystem` apis. All uses of the old `filesystem` api were converted to the `filesystem2` api before I upgraded the fleet to chef 13 (tracked in T28228267). In order to upgrade to Chef 14+, we need to remove the references to the `filesystem2` api. However, there are some outlier operating systems that missed the conversion and so we're going to use a node method to detect what api is available and use that one so we can continue to support this transition for those other platforms. I'm also adding in some checks to validate that the various keys we are using in some of these methods actually exist so we don't end up with the `#<NoMethodError: undefined method `[]' for nil:NilClass>` issues official deprecation: https://docs.chef.io/deprecations_ohai_filesystem.html PR to covert BSD: chef/ohai#1181 PR to convert the rest of Unix (solaris, aix): chef/ohai#1266 PR to convert Windows: chef/ohai#1267 Reviewed By: joshuamiller01 Differential Revision: D21030821 fbshipit-source-id: 5457a9cdc35c55a33f43413ad7f5cf20b8ea593d
Description
Windows doesn't follow quite the same paradigms as Unix, but there's
enough in common that we can still map most things.
Signed-off-by: Phil Dibowitz phil@ipom.com
Check List