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

ds-identify/CloudStack: return $DS_MAYBE if vm is running on vmware/xen #4281

Conversation

weizhouapache
Copy link
Contributor

@weizhouapache weizhouapache commented Jul 24, 2023

Proposed Commit Message

summary: ds-identify/CloudStack: return $DS_MAYBE if vm is running on vmware/xen

Currently, ds-identify determines if CloudStack is supported by checking dmi product name. It works fine on KVM, as the VM has the following system information

System Information
	Manufacturer: Apache Software Foundation
	Product Name: CloudStack KVM Hypervisor

However, it does not work on VMware or XenServer/XCP-ng. For example, on VMware, the system information is

System Information
        Manufacturer: VMware, Inc.
        Product Name: VMware Virtual Platform
        Version: None

Therefore, return $DS_MAYBE instead of $DS_NOT_FOUND if vm is running on vmware/xen.

Additional Context

Test Steps

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Currently, ds-identify determines if CloudStack is supported by checking dmi product name. It works fine on KVM, as the VM has the following system information
```
System Information
	Manufacturer: Apache Software Foundation
	Product Name: CloudStack KVM Hypervisor
```

However, it does not work on VMware or XenServer/XCP-ng. For example, on VMware, the system information is
```
System Information
        Manufacturer: VMware, Inc.
        Product Name: VMware Virtual Platform
        Version: None
```
@weizhouapache weizhouapache force-pushed the dscheck-cloudstack-return-maybe-if-vmware-or-xen branch from 8f1ecba to 35caf66 Compare July 25, 2023 02:17
@weizhouapache
Copy link
Contributor Author

@TheRealFalcon
can you please review this PR ? thanks

@TheRealFalcon TheRealFalcon self-assigned this Aug 1, 2023
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here look good, but the unit tests need to be updated accordingly. See the failing Github Actions job for which tests to fix.

FAILED tests/unittests/test_ds_identify.py::TestDsIdentify::test_ibmcloud_template_no_userdata
FAILED tests/unittests/test_ds_identify.py::TestDsIdentify::test_ibmcloud_template_no_userdata_in_provisioning
FAILED tests/unittests/test_ds_identify.py::TestDsIdentify::test_ibmcloud_template_userdata_in_provisioning
FAILED tests/unittests/test_ds_identify.py::TestDsIdentify::test_vmware_envvar_no_data
FAILED tests/unittests/test_ds_identify.py::TestDsIdentify::test_vmware_guestinfo_no_data
FAILED tests/unittests/test_ds_identify.py::TestDsIdentify::test_vmware_no_valid_transports
@weizhouapache
Copy link
Contributor Author

The changes here look good, but the unit tests need to be updated accordingly. See the failing Github Actions job for which tests to fix.

thanks @TheRealFalcon
I am working on the fix

@weizhouapache
Copy link
Contributor Author

The changes here look good, but the unit tests need to be updated accordingly. See the failing Github Actions job for which tests to fix.

thanks @TheRealFalcon I am working on the fix

@TheRealFalcon
I have updated the PR

run the test locally, it looks ok

$ tox -e py3
...

------------------------------------------------------------------------------------------------
TOTAL                                                          29809   6045  11789   1470    77%

================================ slowest 10 durations ================================
2.01s call     tests/unittests/sources/test_scaleway.py::TestDataSourceScaleway::test_metadata_connection_errors_legacy_ipv4_url
1.81s call     tests/unittests/sources/test_ec2.py::TestEc2::test_aws_inaccessible_imds_service_fails_with_retries
1.21s call     tests/unittests/test_url_helper.py::TestUrlHelper::test_timeout
1.01s call     tests/unittests/sources/test_ec2.py::TestEc2::test_network_config_cached_property_refreshed_on_upgrade
1.01s call     tests/unittests/sources/test_azure.py::TestProvisioning::test_source_pps_fails_initial_dhcp[Unknown]
1.01s call     tests/unittests/sources/test_azure.py::TestProvisioning::test_source_pps_fails_initial_dhcp[Savable]
1.00s call     tests/unittests/sources/test_azure.py::TestProvisioning::test_source_pps_fails_initial_dhcp[Running]
1.00s call     tests/unittests/sources/test_vmware.py::TestDataSourceVMware::test_wait_on_network
0.94s call     tests/unittests/sources/test_ec2.py::TestEc2::test_valid_platform_with_strict_true
0.87s call     tests/unittests/sources/test_ec2.py::TestEc2::test_get_instance_tags
= 4780 passed, 4 skipped, 1 deselected, 1 xfailed, 271 warnings in 70.05s (0:01:10) ==
______________________________________ summary _______________________________________
  py3: commands succeeded

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@TheRealFalcon TheRealFalcon merged commit 7949bb3 into canonical:main Aug 14, 2023
25 checks passed
@rohityadavcloud
Copy link

Thanks @weizhouapache - good work!

weizhouapache added a commit to weizhouapache/image-builder that referenced this pull request Aug 15, 2023
…k images

This reverts commit d2956b3.

We'v found that the new images for CAPC do not work on VMware and Xen, which is caused by an issue fixed by canonical/cloud-init#4281
(it works fine on KVM)

We need to revert the changes and reapply it after intensive testing.
@TheRealFalcon
Copy link
Member

@weizhouapache , unfortunately, we're going to have to revert this commit. We're seeing some serious regressions caused by this commit (see #4501 ) due to cases where DataSourceNone was being used before on VMWare and Xen, but now is being detected as a possible CloudStack. This means cloud-init goes from being disabled to waking up and running through all of the first-boot activities including resetting host keys and creating new users.

We'd still love to accommodate your use cases, but I think that requires finding a stronger way to identify CloudStack.

TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request Oct 16, 2023
TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request Oct 16, 2023
TheRealFalcon added a commit that referenced this pull request Oct 16, 2023
…#4281)" (#4511)

This reverts commit 7949bb3
but keeps the CLA signature.

Fixes GH-4501
LP: #2039453
TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants