Merge ~msaikia/cloud-init:topic-msaikia-vmware-custom-script into cloud-init:master
| Status: | Merged |
|---|---|
| Approved by: | Chad Smith on 2017-12-08 |
| Approved revision: | bde436d2775fdc1882a6f08c69aa528c0f638be5 |
| Merged at revision: | ce33e423cde806a0590fec635778d62836e1bd37 |
| Proposed branch: | ~msaikia/cloud-init:topic-msaikia-vmware-custom-script |
| Merge into: | cloud-init:master |
| Diff against target: |
650 lines (+459/-42) 8 files modified
cloudinit/sources/DataSourceOVF.py (+88/-37) cloudinit/sources/helpers/vmware/imc/config.py (+4/-0) cloudinit/sources/helpers/vmware/imc/config_custom_script.py (+153/-0) cloudinit/sources/helpers/vmware/imc/config_nic.py (+1/-1) tests/unittests/test_datasource/test_ovf.py (+107/-4) tests/unittests/test_vmware/__init__.py (+0/-0) tests/unittests/test_vmware/test_custom_script.py (+99/-0) tests/unittests/test_vmware_config_file.py (+7/-0) |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Chad Smith | 2017-09-01 | Approve on 2017-12-08 | |
| Server Team CI bot | continuous-integration | 2017-09-01 | Approve on 2017-12-05 |
|
Review via email:
|
|||
This proposal supersedes a proposal from 2017-08-23.
Commit Message
VMware: Support for user provided pre and post-customization scripts
In the VMware customization workflow, we have some options for the user
to upload scripts for additional customization. Based on user request,
those custom scripts can be either run before regular customization or
after. For post customization scripts, we decide whether to run the scripts
just after customization or post system reboot.
Description of the Change
Support for users to upload custom scripts in VMware customization workflow.
In the VMware workflow, we have some options for the user to upload custom scripts for additional customization. Based on user request those custom scripts can be either run before regular customization or after. For post customization scripts, we decide whether to run the scripts just after customization or post reboot.
PASSED: Continuous integration, rev:a339c450b53
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
| Chad Smith (chad.smith) wrote : | # |
Thanks for this contribution, I have a couple of notes to get us started here. Will test more on this and respond Monday.
| Chad Smith (chad.smith) wrote : | # |
Leaving as needs fixing for the review comments mentioned. Looks good again, thanks for the work here. Please mark this back to "needs review" when comments have been addressed.
| Maitreyee Saikia (msaikia) wrote : | # |
Addressed review comments.
Thanks!
PASSED: Continuous integration, rev:3b626e16069
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:3b626e16069
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
- b39eb0a... by Joshua Powers on 2017-09-01
- 3316786... by Joshua Powers on 2017-09-01
- 75d806c... by Chad Smith on 2017-09-02
- 0ea19ba... by Chad Smith on 2017-09-02
- f759ce5... by Chad Smith on 2017-09-02
- 9f80a27... by Lars Kellogg-Stedman on 2017-09-05
- 33f9546... by Lars Kellogg-Stedman on 2017-09-05
- 7f54e3a... by Scott Moser on 2017-09-05
- eeb754d... by Scott Moser on 2017-09-05
- 83777af... by Chad Smith on 2017-09-06
- 78b02e3... by Chad Smith on 2017-09-06
- 1a738fa... by Scott Moser on 2017-09-07
- f08b473... by Scott Moser on 2017-09-07
- 8336907... by Sankar Tanguturi on 2017-09-08
- 190dbc7... by Sankar Tanguturi on 2017-09-08
- 5708cb2... by Sankar Tanguturi on 2017-09-08
- 55e8a71... by Joshua Powers on 2017-09-11
- 78de454... by Joshua Powers on 2017-09-11
- 37a3d71... by Joshua Powers on 2017-09-11
- 4ed7511... by Joshua Powers on 2017-09-11
- 45194dd... by Maitreyee Saikia on 2017-09-12
- 157a632... by Maitreyee Saikia on 2017-09-12
- ae1683b... by Ethan Apodaca on 2017-09-14
- b57a5d7... by Ethan Apodaca on 2017-09-14
- cef363b... by Ethan Apodaca on 2017-09-14
- f4a2806... by Maitreyee Saikia on 2017-09-14
PASSED: Continuous integration, rev:f4a2806ff2f
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
- 8179b05... by Maitreyee Saikia on 2017-09-15
PASSED: Continuous integration, rev:8179b05c540
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
| Chad Smith (chad.smith) wrote : | # |
Thanks again for your patience here. A couple more review comments and then I *think* we are there.
I'm still seeing merge conflicts with this branch that will likely require a git rebase to sort.
- c664a44... by Maitreyee Saikia on 2017-09-22
FAILED: Continuous integration, rev:c664a448219
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
- d58e6af... by David Mulford on 2017-10-09
- 92e9944... by Maitreyee Saikia on 2017-10-24
- 7711408... by Maitreyee Saikia on 2017-10-25
- a5d82c6... by Maitreyee Saikia on 2017-10-26
- 8785b65... by Maitreyee Saikia on 2017-10-26
PASSED: Continuous integration, rev:8785b65633b
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
| Maitreyee Saikia (msaikia) wrote : | # |
Hi Chad, thanks for taking a look at the review request. I have published a new review request after addressing your concerns. Please also find response to some other comments.
Thanks
| Maitreyee Saikia (msaikia) wrote : | # |
Thanks Chad. Please find the responses below. Will address the rest.
| Maitreyee Saikia (msaikia) wrote : | # |
Thanks Chad for the review. I think the diff comments did not get updated in my previous reply. Re-posting comment.
- 3f27def... by Scott Moser on 2017-11-08
- f530c36... by Robert Schweikert on 2017-11-08
- 4694ebc... by Robert Schweikert on 2017-11-08
- 7189c59... by Maitreyee Saikia on 2017-11-09
- c773da4... by Maitreyee Saikia on 2017-11-09
- adf16c7... by Maitreyee Saikia on 2017-11-09
PASSED: Continuous integration, rev:adf16c77ee2
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
- de5d6e9... by Scott Moser on 2017-11-09
| Maitreyee Saikia (msaikia) wrote : | # |
Addressed review comments.
| Chad Smith (chad.smith) wrote : | # |
I'm +1 on this now. I know we have cleanup we need to address in the future, but for vmware's pre/post script flow, this current iteration makes sense.
| Chad Smith (chad.smith) wrote : | # |
Maitreyee, one last pass on this from me. Could you please add a couple unit tests which exercise the OVF get_data execution path with customizationscript present (and erroring) to ensure that behavior is properly handled. (probably looking for tests in tests/unittests
| Maitreyee Saikia (msaikia) wrote : | # |
> Maitreyee, one last pass on this from me. Could you please add a couple unit
> tests which exercise the OVF get_data execution path with customizationscript
> present (and erroring) to ensure that behavior is properly handled. (probably
> looking for tests in tests/unittests
Hi Chad,
We have some tests in tests/unittests
Do you want me to test the calling function? The calling function does not error out if the variable "customscript" is not set. The if condition here is only to determine whether to go to that workflow or not? If not, we just move to the next configuration. All the error conditions are tested in the actual function, as to whether the correct error is raised or not.
Do you still want me to add tests on the calling function?
| Chad Smith (chad.smith) wrote : | # |
Thanks Maritreyee for those tests specific to the custom script. And yes please to add a couple tests special_
We are trying to get cloud-init up out of the 60% range for unit test coverage so that we can have more certainty that we aren't breaking consumers out there when we add new features or refactor some code.
This is hard I realize as it doesn't seem like there are any get_data tests for OVF datasource currently. Here's a patch reference with a couple things in it for your branch:
1. Some tests for new functions you've added
2. Sample test for OVF.get_data()
3. Shuffle your marker files under /var/lib/cloud as that's where other cloud-init specific semaphores also live. It's better not to leak files into root '/'.
Here's a paste of the start http://
| Maitreyee Saikia (msaikia) wrote : | # |
Thanks for the patch Chad.
Working on the tests now.
But I have a little concern about putting the marker file in /var/lib/cloud.
What if someone cleans up the entire /var/lib/cloud?
In that case, we will lose the marker file for the next run, and as such will
go through the entire workflow, which is not what we expect. Is there any way to handle that?
- 679b393... by Maitreyee Saikia on 2017-11-15
| Chad Smith (chad.smith) wrote : | # |
Maitreyee, I believe that is exactly the behavior we want to be common across all of cloud-init. Right now, if /var/lib/cloud is cleaned up and the machine is rebooted, we expect cloud-init to re-perform all configuration steps as if it is run on against clean system. In fact we are adding a cloud-init clean commandline function to do just that:
https:/
Also, per filesystem heirarchy standard, we also shouldn't be leaking files to other directories beyond what the cloud-init package 'owns'. It's okay if we haven't yet sorted the details of leaving around vmware marker files (or cleaning up former config changes due to those customizations. This lack of 'cleanup' handling is something that cloud-init will have to tackle for ssh keys or users created which may have been left on the system by the first cloud-init run, and may need removing before reboot.
- 50a025c... by Maitreyee Saikia on 2017-11-22
- 0cb4226... by Maitreyee Saikia on 2017-11-22
- 582c798... by Maitreyee Saikia on 2017-11-22
FAILED: Continuous integration, rev:582c798a3af
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
- da14017... by Maitreyee Saikia on 2017-11-23
PASSED: Continuous integration, rev:da14017dec1
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
| Maitreyee Saikia (msaikia) wrote : | # |
Thanks Chad,
I have addressed your comments.
Please take a look at the review request.
Regards,
> Maitreyee, I believe that is exactly the behavior we want to be common across
> all of cloud-init. Right now, if /var/lib/cloud is cleaned up and the machine
> is rebooted, we expect cloud-init to re-perform all configuration steps as if
> it is run on against clean system. In fact we are adding a cloud-init clean
> commandline function to do just that:
>
> https:/
> init/+merge/333513
>
> Also, per filesystem heirarchy standard, we also shouldn't be leaking files to
> other directories beyond what the cloud-init package 'owns'. It's okay if we
> haven't yet sorted the details of leaving around vmware marker files (or
> cleaning up former config changes due to those customizations. This lack of
> 'cleanup' handling is something that cloud-init will have to tackle for ssh
> keys or users created which may have been left on the system by the first
> cloud-init run, and may need removing before reboot.
| Chad Smith (chad.smith) wrote : | # |
just a couple of tiny comments, thanks for adding the unit tests for datasourceovf.
- 2f2070f... by Maitreyee Saikia on 2017-11-29
PASSED: Continuous integration, rev:2f2070fdafe
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
| Maitreyee Saikia (msaikia) wrote : | # |
> just a couple of tiny comments, thanks for adding the unit tests for
> datasourceovf.
Thanks. Updated new diff.
- 028d945... by Maitreyee Saikia on 2017-12-02
PASSED: Continuous integration, rev:028d945d24b
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
| Chad Smith (chad.smith) wrote : | # |
Thank you for all the work here Maitreyee. One minor comment above about where to store the marker files after discussion with the team.
I've tested your content on non-vmware kvm instances and validated that it doesn't break other consumer paths as well. Thank you again for the good work. If you agree to the above final change suggestion, I'll merge this proposal.
| Maitreyee Saikia (msaikia) wrote : | # |
> Thank you for all the work here Maitreyee. One minor comment above about where
> to store the marker files after discussion with the team.
>
> I've tested your content on non-vmware kvm instances and validated that it
> doesn't break other consumer paths as well. Thank you again for the good work.
> If you agree to the above final change suggestion, I'll merge this proposal.
Thank you Chad.
I am ok with the suggestion.
Should I push a new diff?
- 06624c9... by Maitreyee Saikia on 2017-12-05
- bde436d... by Maitreyee Saikia on 2017-12-05
PASSED: Continuous integration, rev:bde436d2775
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
| Chad Smith (chad.smith) wrote : | # |
Validated this branch against a non-vmware OVF kvm instance and the logic didn't adversely affect non-vmware paths. We should land this.


PASSED: Continuous integration, rev:dc7653be740 c682a325f31162a 59dbaf344b8505 /jenkins. ubuntu. com/server/ job/cloud- init-ci/ 188/
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild: /jenkins. ubuntu. com/server/ job/cloud- init-ci/ 188/rebuild
https:/