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

fix: reconfigure vm with multiple pci passthrough devices #2236

Merged
merged 2 commits into from
Jul 5, 2024

Conversation

spacegospod
Copy link
Collaborator

@spacegospod spacegospod commented Jul 5, 2024

Description

Usually when we reconfigure VMs we set negative integers as keys for new devices. vCenter assigns actual keys after the reconfiguration task completes which we then read and reassign to our resource.

This was not the case with PCI passthrough devices. No key was being assigned which meant that every such device got a zero as its key (the default integer value). This is fine if you're adding a single device but if you try with more than one the API fails.

The fix is simple - we simply need to assign a unique negative integer as the device key just like we do for other device types.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

I had to manually test this one since it requires dedicated devices on the underlying hardware.

There is an important disclaimer to be made here - adding PCI passthrough devices during VM creation does not work. It is does not work out of the box and this is not related to this change.

I am not making an attempt to resolve this problem with this PR as it requires a significant refactoring effort.

Release Note

resource/virtual_machine: Fixed virtual machine reconfiguration with multiple PCI passthrough devices.

References

Closes #1688

@spacegospod spacegospod self-assigned this Jul 5, 2024
@spacegospod spacegospod requested a review from a team as a code owner July 5, 2024 15:43
@github-actions github-actions bot added provider Type: Provider needs-review Status: Pull Request Needs Review labels Jul 5, 2024
@tenthirtyam tenthirtyam added the bug Type: Bug label Jul 5, 2024
@tenthirtyam tenthirtyam added this to the v2.8.3 milestone Jul 5, 2024
@tenthirtyam tenthirtyam changed the title fix: Reconfigure VM with multpiple PCI passthrough devices fix: reconfigure vm with multiple pci passthrough devices Jul 5, 2024
@tenthirtyam
Copy link
Collaborator

I am not making an attempt to resolve this problem with this PR as it requires a significant refactoring effort.

Is there an issue that can track the need to refactor this area?

Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

LGTM - nice detective work! 🕵️ 🔍

@tenthirtyam tenthirtyam merged commit 0101e81 into main Jul 5, 2024
11 checks passed
@tenthirtyam tenthirtyam deleted the bugfix/pci-devices branch July 5, 2024 16:32
@spacegospod
Copy link
Collaborator Author

I am not making an attempt to resolve this problem with this PR as it requires a significant refactoring effort.

Is there an issue that can track the need to refactor this area?

good idea, I just logged one
#2237

Copy link

github-actions bot commented Aug 8, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2024
@tenthirtyam tenthirtyam removed the needs-review Status: Pull Request Needs Review label Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Type: Bug provider Type: Provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating r/virtual_machine with multiple PCI passthrough devices fails
3 participants