Add support for sysprep customization templates #17293
Conversation
|
@gmcculloug Please review |
|
@gmcculloug I'm basically mimicking the cloud init code. I was hoping to use the cloud init specs to base my own specs off of, but there don't appear to be any. I've got a couple basic specs, though. In any case, it appears to work. I was able to select the sysprep script I had created on the infra side, and the machine provisioned successfully. |
djberg96
changed the title
|
@djberg96 The way this is currently coded you are enabling sysprep support for all cloud providers. You mention testing but can you clarify what providers you tested this on. I'm concerned about enabling on all providers, including any potential future providers. Definitely would be good to add tests around |
|
@gmcculloug the individual providers can disable it in their |
|
I understand what you are saying, but we should, to the best of our abilities, only provide viable options to the end user. For example, we know the OS of the image from either the provider inventory data or from Smate-State data. Therefore we should only show Sysprep items to Windows Images. The CloudInit logic you built this from did not have to filtering them out since it is available on both Linux and Windows. Also, still curious where the testing was performed? If you want to enabled Sysprep at this level will you be making followup PRs to the providers were we do not currently support passing sysprep? It would be good to link those PRs to this one. cc @bronaghs |
|
@gmcculloug I would be surprised if cloud-init worked on Windows. It would require 3rd party software wouldn't it? Perhaps it would depend on the provider. Maybe I'm missing something. I definitely need help with the testing because I'm just not sure how other than to successfully provision an instance from a sysprep'ed image. Once you sysprep an image, it's essentially "generalized" so you can't log into it any more to check it. Not as far as I know anyway. As for the other providers, I wasn't planning on going through them all explicitly, as I guess I figured for most of them it would "just work" for Windows VM's, and we could disable on an as-needed basis. I'll look at filtering to see if we can restrict based on image type. |
|
We can definitely help with specs for this, I can work with @syncrou on getting something in place for the existing CloudInit that you can work off of. For this code change we would want to validate the workflow code, no need to go to the depth of validating a provisioned instance. That is way out of scope of these changes. Think unit testing, not integration testing.
This gets to the core of my concern. @blomquisg Who from the providers team is best to address the question about what providers even support passing Sysprep data? Then we would need to evaluate if we have actually enabled passing it to the provider. My guess is that it is no just going to be functional without additional code changes to the provider repos. Based on that feedback we should be able to make an informed decision on this being enabled by default at the code workflow or per-provider. |
| def allowed_sysprep_customization_templates(_options = {}) | ||
| result = [] | ||
| return result if (source = load_ar_obj(get_source_vm)).blank? | ||
|
|
There was a problem hiding this comment.
@djberg96 You can check the platform of the source VM here and return an empty array if it is not windows.
Example call to the platform method:
https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_provision/options_helper.rb#L59
There was a problem hiding this comment.
@gmcculloug Ok, done. I tested it out, sure enough, the sysprep templates aren't visible for Linux images.
|
This approach feels very widespread. I would prefer we only enable Sysprep support in the Azure provisioning workflow and only for Windows VMs, I would not assume everyone has the expertise to know only to run Sysprep on Windows VMs. Once working (with specs) for Azure, we can apply to the other cloud provider one by one. |
|
@bronaghs Well, based on https://access.redhat.com/documentation/en-us/red_hat_cloudforms/4.6/ it's supposed to be supported for Amazon, Azure, VMWare, RHOS and RHV. |
|
This change is widespread across cloud providers, so that is the scope my comment pertains to, so my suggestion is to add support fo Azure first and then Amazon as a separate effort. |
|
@gmcculloug @syncrou Well, I tried a fairly straightforward copy/pasta approach (and fixed one minor typo), but obviously I'm doing something wrong. Any suggestions? |
|
@djberg96 Looks like the only travis failure now is due to the use of |
|
@gmcculloug Yep, forgot about the windows check originally, then forgot that casecmp? isn't supported in Ruby 2.3.x. |
| it "should retrieve sysprep templates when cloning" do | ||
| options = {'key' => 'value' } | ||
| allow(sysprep_workflow).to receive(:supports_native_clone?).and_return(true) | ||
| allow_any_instance_of(MiqTemplate).to receive(:platform).and_return('windows') |
There was a problem hiding this comment.
Can this allow_any_instance_of be replaced with allow(template) to avoid one of the rubocop warnings?
There was a problem hiding this comment.
Tried that, doesn't work, though I'm not sure why as it definitely looks like it should work.
There was a problem hiding this comment.
That means that the miq_template instance is getting loaded through a different path instead of using the in-memory copy created by the let block. I cannot do it today, but I'll find a little time to see if I can find a where we can stub and return the in-memory version to avoid this any_instance call.
There was a problem hiding this comment.
@djberg96 I was able to work around this by stubbing the load_ar_obj method that gets called from the new mixin method:
allow(sysprep_workflow).to receive(:load_ar_obj).and_return(template)
allow(template).to receive(:platform).and_return('windows')There was a problem hiding this comment.
@gmcculloug Ok, pushed up that change, thanks!
Added basic spec for the supports_sysprep? method. Only allow sysprep on Windows images. Initial stab at specs for sysprep templates. Force windows platform for sysprep test.
There was a problem hiding this comment.
I'm expecting the one allow_any_instance_of warning which I think we need to keep for now.
@djberg96 @bronaghs We still need to resolve the issue around how widely this is enabled.
#17293 (comment)
I am going to mark as needing changes until we come to an agreement so this does not get merged by mistake.
…plates to check for support first.
|
@gmcculloug @bronaghs I've modified the code so that sysprep support is disabled by default. It would be up to the individual provider subclasses to enable it. So, that will be a separate, small PR for Azure. |
|
Some comments on commits https://github.com/djberg96/manageiq/compare/bf541dd90ae2565077059283be4cebd8e49085fe~...4155045082c9e2112e0b1fb9b9deab6f7b6be48a spec/models/manageiq/providers/cloud_manager/provision_workflow_spec.rb
|
|
Checked commits https://github.com/djberg96/manageiq/compare/bf541dd90ae2565077059283be4cebd8e49085fe~...4155045082c9e2112e0b1fb9b9deab6f7b6be48a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@gmcculloug @Fryguy Do we want this backported at all or not? |
|
As an enhancement it typically would not be back-ported. |
This adds support for sysprep customization templates.
Attempts to address #17288
This is a WIP for now because I'm not sure if we want to support this by default, or if individual providers should have to opt-in. And also because it needs more specs and UAT.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1565791