-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(lxd): add s390x virtio-ports detection for LXD #6597
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
base: main
Are you sure you want to change the base?
Conversation
|
Additionally, we should add resolute support to pycloudlib per canonical/pycloudlib#494 |
fe83d28 to
413e95b
Compare
holmanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @blackboxsw, this is a good start. I left some comments inline, also note that the commit message:
Drop the now unnecessary DMI board name check from ds-identify and
DataSourceLXD because virtio-ports is sufficient in bot KVM/QEMU and
s390x.
The following mechinisms for LXD detection are retained:
1. /dev/lxd/sock socket file (primary method)
2. virtio-ports serial device (KVM/QEMU and new for s390x)
This reads as if s390x is a new platform alternative to QEMU, when in fact it is a different cpu instruction set (like x86_64 or amd64).
Support LXD detection on IBM's s390x architecture using virtio-ports serial device detection. Add a new detection method that checks for LXD serial devices in /sys/class/virtio-ports/ for both current (com.canonical.lxd) and legacy (org.linuxcontainers.lxd) names. Update public LXD datasource documentation for this detection method. Drop the now unnecessary DMI board name check from ds-identify and DataSourceLXD because virtio-ports is sufficient in bot KVM/QEMU and non-x86_64 platforms without DMI information. The following mechinisms for LXD detection are retained: 1. /dev/lxd/sock socket file (primary method) 2. virtio-ports serial device presence if socket file isn't yet present Implements: PL073
blackboxsw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review @holmanb. I think I have resolved all comments.
413e95b to
cff1ba6
Compare
holmanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we still have a failing ruff check.
holmanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this @blackboxsw. See my comments.
holmanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @blackboxsw, I think this is getting closer. See my latest round of comments.
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging blackboxsw, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag blackboxsw to reopen it.) |
67190b4 to
df409f7
Compare
- comment fixes in tests and code paths regarding lxd socket file. - fix integration test behavior to validate ds-identify discovery of lxd despite security.devlxd set False - fix unit tests to drop trailing newline
df409f7 to
35c274a
Compare
blackboxsw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Brett. I believe I have addressed all review comments now.
35c274a to
042e234
Compare
holmanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @blackboxsw. Besides the linter failures it looks like there were also a couple of comments from before that weren't addressed.
| # Expect fallback to NoCloud because python DataSourceLXD cannot | ||
| # get any information from /dev/lxd/sock due to security.devlxd above. | ||
| cloud_id = client.execute("cloud-id").stdout | ||
| if "nocloud" != cloud_id: | ||
| raise AssertionError( | ||
| "cloud-init did not discover 'nocloud' datasource." | ||
| f" Found '{cloud_id}'" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests a specific failure mode that occurs when the socket is disabled, which is more of a test of the platform than of cloud-init, isn't it? This doesn't actually "Test DataSourceLXD on KVM detected by ds-identify using virtio-ports."
If the LXD project stopped providing the nocloud device then this test would start to fail unexpectedly due to changes in an external project. Checking for nocloud here could provide a "canary signal" to warn that something is broken on the platform, but the signal getting to the right place would mean work for the developers to look at failing integration tests and doing something about the failure. But if this is known and expected by LXD developers then what is the point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment was maybe misleading here or didn't provide enough relevant context.
I've updated the comment to:
+ # Expect NoCloud because python DataSourceLXD cannot read metadata from
+ # /dev/lxd/sock due to security.devlxd above and falls back to
+ # the nocloud seed files written by _customize_environment.
cloud_id = client.execute("cloud-id").stdout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what is happening. I'm saying I don't necessarily think that this is important behavior for us to test. Is the fact that NoCloud is detected when the socket is not available a core feature of cloud-init? Or is this just a failure mode that happens to still resemble success simply because that is how LXD originally exposed the IMDS?
My comment earlier was about the test maintenance burden this kind of test will have introduced if the LXD project later stops providing 2 duplicate datasources. I don't want to have to come back later and rewrite this test when the LXD project decides that they will no longer provide duplicate datasources.
tests/unittests/test_ds_identify.py
Outdated
| }, | ||
| # /dev/lxd/sock does not exist and KVM virt-type | ||
| "mocks": [{"name": "is_socket_file", "ret": 1}, MOCK_VIRT_IS_KVM], | ||
| "mocks": [{"name": "is_socket_file", "ret": 1}, MOCK_VIRT_IS_KVM_QEMU], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forget to push?
|
Thank you for this additional round @holmanb. Getting nearer to the finish line here and avoiding stray test changes that are unrelated to this PR. |
tests/unittests/cloudinit/example.pyshould be tested bytests/unittests/test_example.pytox -e py3tox -e doc.Proposed Commit Message
Additional Context
Test Steps
Merge type