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.

Sign up for GitHub

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

Jump to bottom

NAS-130523 / 25.04 / Container support using Incus #14592

Merged
merged 45 commits into from
Oct 21, 2024
Merged

NAS-130523 / 25.04 / Container support using Incus #14592

merged 45 commits into from
Oct 21, 2024
+1,962 −10

Conversation

william-gr
Copy link
Member

@william-gr william-gr commented Sep 27, 2024 •
edited
Loading

This PR should cover the basics for having LXC containers.

The device handling is incomplete (especially GPU) but I believe that can be completed in another set of PRs.

It should be enough for development of UI for the majority of functionality.

@bugclerk
Copy link
Contributor

Jira URL: https://ixsystems.atlassian.net/browse/NAS-130523

@bugclerk bugclerk changed the title Prototype implementation for Container support using Incus (WIP) NAS-130523 / 25.04 / Prototype implementation for Container support using Incus (WIP) Sep 27, 2024
@william-gr william-gr force-pushed the NAS-130523 branch 2 times, most recently from e03876c to bf66dfa Compare September 30, 2024 17:43
anodos325
anodos325 reviewed Oct 1, 2024
View reviewed changes
src/freenas/etc/subgid Outdated Show resolved Hide resolved
@william-gr william-gr marked this pull request as ready for review October 4, 2024 17:58
@william-gr william-gr changed the title NAS-130523 / 25.04 / Prototype implementation for Container support using Incus (WIP) NAS-130523 / 25.04 / Prototype implementation for Container support using Incus Oct 4, 2024
@william-gr william-gr changed the title NAS-130523 / 25.04 / Prototype implementation for Container support using Incus NAS-130523 / 25.04 / Container support using Incus Oct 4, 2024
@william-gr
Copy link
Member Author

william-gr commented Oct 4, 2024 •
edited
Loading

@anodos325 @yocalebo I know this is not 100% yet but maybe this is acceptable for landing without major changes and the rest can be done through subsequent PRs. UI team is in need of work and perhaps this is enough for them to get started.

Qubad786
Qubad786 reviewed Oct 15, 2024
View reviewed changes
Copy link
Contributor

@Qubad786 Qubad786 left a comment

Choose a reason for hiding this comment

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

We should also add some changes so that relevant datasets being used are reported as such by pool.dataset.details

src/middlewared/middlewared/main.py Outdated Show resolved Hide resolved
src/middlewared/debian/control Show resolved Hide resolved
src/middlewared/middlewared/plugins/service_/services/pseudo/misc.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/service_/services/pseudo/misc.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/service_/services/pseudo/misc.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/instance.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/instance.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/stats.py Outdated Show resolved Hide resolved
tests/api2/test_virt_002_instance.py Show resolved Hide resolved
tests/api2/test_virt_002_instance.py Show resolved Hide resolved
@william-gr
Copy link
Member Author

william-gr commented Oct 16, 2024 •
edited
Loading

@Qubad786 thanks for the thorough review. I think I have handled most of the points raised with a reply so far, please let me know if I have forgotten anything.

Qubad786
Qubad786 reviewed Oct 18, 2024
View reviewed changes
Copy link
Contributor

@Qubad786 Qubad786 left a comment

Choose a reason for hiding this comment

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

Just some more nits

src/middlewared/middlewared/plugins/pool_/dataset_details.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/pool_/dataset_details.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/pool_/dataset_details.py
@@ -336,6 +339,22 @@ def build_details(self, mntinfo):
'mount_info': self.get_mount_info(path_config['source'], mntinfo),
})

# virt instance
for instance in self.middleware.call_sync('virt.instance.query'):
Copy link
Contributor

@Qubad786 Qubad786 Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this expensive btw?
We usually want this endpoint to be very fast and in that case we might want to be as selective as we can be in terms of what we are retrieving if getting everything is expensive comparatively

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the alternative?

src/middlewared/middlewared/plugins/service_/services/incus.py Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/device.py Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/global.py Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/instance_device.py Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/instance_device.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/instance.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/plugins/virt/instance.py Outdated Show resolved Hide resolved
@william-gr
Copy link
Member Author

@Qubad786 @yocalebo reviews addressed, please let me know what else is missing.

yocalebo
yocalebo approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@yocalebo yocalebo left a comment

Choose a reason for hiding this comment

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

Okay, this looks good from my viewpoint. I'm going to approve but I'd like @Qubad786's final approval before it's merged. (Also make sure the database migration ID's are still correct since we've had some migrations since this PR was opened up).

@william-gr
Copy link
Member Author

retest this please

@william-gr william-gr merged commit 3e5b1de into master Oct 21, 2024
3 checks passed
@william-gr william-gr deleted the NAS-130523 branch October 21, 2024 19:13
@bugclerk
Copy link
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@anodos325 anodos325 anodos325 left review comments

@yocalebo yocalebo yocalebo approved these changes

@Qubad786 Qubad786 Qubad786 approved these changes

@creatorcary creatorcary Awaiting requested review from creatorcary

@aiden3c aiden3c Awaiting requested review from aiden3c

Assignees
No one assigned
Labels
no-time-tracked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@william-gr @bugclerk @Qubad786 @anodos325 @yocalebo