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

Switch to session libvirt #35

Closed
wants to merge 1 commit into from
Closed

Switch to session libvirt #35

wants to merge 1 commit into from

Conversation

zeenix
Copy link

@zeenix zeenix commented Oct 11, 2019

The crc network can live on the system libvirt while we do the rest on the session connection.

This requires crc-org/crc#709.

Fixes #20.

libvirt.go Outdated Show resolved Hide resolved
constants.go Show resolved Hide resolved
libvirt.go Show resolved Hide resolved
libvirt.go Outdated Show resolved Hide resolved
constants.go Outdated Show resolved Hide resolved
@@ -496,7 +511,7 @@ func (d *Driver) getMAC() (string, error) {
}

func (d *Driver) getIPByMacFromSettings(mac string) (string, error) {
conn, err := d.getConn()
conn, err := d.getSystemConn()

Choose a reason for hiding this comment

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

Does this method needs more changes? It seems to expect a libvirt network, but we switched to bridges? With qemu:///session, can we get the vm ip address without using qemu:///system?

Copy link
Author

@zeenix zeenix Oct 28, 2019

Choose a reason for hiding this comment

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

We switched to using the network in the VM as a bridge, we still know that it's a libvirt network existing on the system connection. So I don't think so.

@zeenix zeenix force-pushed the session branch 2 times, most recently from 0da5269 to 18aa92d Compare October 29, 2019 16:00
Copy link

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

With this commit, machine-driver-libvirt only needs qemu:///session for read-only access to the 'crc' network. It uses it to check everything is in order before starting a VM, and to get the bridge name which will be used in getIPByMacFromSettings, and when creating a new VM.
Overall looks good, needs careful testing, and maybe further adjustments on the crc side (for example, create a polkit ACL only allowing read access to the 'crc' network rather than adding the user to the libvirt group).

libvirt.go Show resolved Hide resolved
libvirt.go Show resolved Hide resolved
libvirt.go Outdated Show resolved Hide resolved
libvirt.go Show resolved Hide resolved
@zeenix
Copy link
Author

zeenix commented Nov 1, 2019

With this commit, machine-driver-libvirt only needs qemu:///session for read-only access

Correct but I think you meant qemu:///system. :)

The `crc` network can live on the system libvirt while we do the rest on
the session connection.

Fixes crc-org#20.
@guillaumerose
Copy link

We will reopen this PR if needed. #20 is still opened.

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.

libvirt: move to user session, instead of system
5 participants