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

QEMU orchestrator: Add support for data disks. #1775

Merged
merged 2 commits into from
Mar 4, 2022

Conversation

cwize1
Copy link
Contributor

@cwize1 cwize1 commented Mar 3, 2022

No description provided.

@cwize1 cwize1 requested a review from squirrelsc as a code owner March 3, 2022 23:10
@@ -478,6 +478,10 @@ def _generate_min_capability(self, capability: Any) -> Any:
min_value.data_disk_count = search_space.generate_min_capability_countspace(
self.data_disk_count, capability.data_disk_count
)
if self.data_disk_size or capability.data_disk_size:
Copy link
Member

Choose a reason for hiding this comment

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

@LiliDeng please check if this change will impact Azure data disk tests or not.

nodes_requirement.append(node_capabilities)
# Check that the general node capabilities are compatible with this node's
# specific requirements.
if not node_space.check(nodes_capabilities):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I finally figured out how node capabilities/requirements are supposed to be handled. Does this look correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's right. And the capability is extendable, if there are special things in qemu to enforce like vm_size in Azure.

# Configure data disks.
if node_space.disk:
assert isinstance(
node_space.disk.data_disk_count, int
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally tried to use node.capability here. But the data_disk_count was an IntRange instead of an int. Is this expected behavior?

Copy link
Member

Choose a reason for hiding this comment

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

The capability should be set by platform. Its default value is unlimited capacity from 0 to max.

The capability type can be rewritten by platform, like AzureDiskOptionSettings. But it needs to replace the default one by platform like _convert_to_azure_node_space. It's pretty complicated, and undocumented. Feel free to let me know, if there are more questions.

I'm planning to write some resource planner, it may be helpful for lab resource management. But don't rely on it, I don't have time allocated...

@squirrelsc squirrelsc merged commit bc4bea0 into microsoft:main Mar 4, 2022
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.

2 participants