Skip to content

/ mantle

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

*: add unprivileged-qemu platform #968

Merged
merged 2 commits into from May 13, 2019
Merged

*: add unprivileged-qemu platform #968

merged 2 commits into from May 13, 2019

Conversation

@arithx
Copy link
Contributor

arithx commented Jan 31, 2019

Adds a new platform unprivileged-qemu which is meant to be ran without
root access. It has a restricted set of functionality, such as a lack of
the Local cluster and no networking between machines.
@arithx arithx force-pushed the arithx:qemu_2_electric_boogaloo branch from dbecf55 to a08948a Jan 31, 2019
@arithx arithx force-pushed the arithx:qemu_2_electric_boogaloo branch 2 times, most recently from 9ad6e46 to 46933f2 Feb 19, 2019
@arithx arithx changed the title WIP: *: add unprivileged-qemu platform *: add unprivileged-qemu platform Feb 19, 2019
@arithx arithx force-pushed the arithx:qemu_2_electric_boogaloo branch from 46933f2 to 6712ac9 Feb 19, 2019
@arithx

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

arithx commented Feb 19, 2019

Removing WIP, ready for review.

From offline discussions, once this is merged we should open up an issue for refactoring the QEMU based platforms. A refactor would likely include a new Command interface that supports both namespaced & non-namespaced command sets as well as some form of a base QEMU cluster struct / interface that has methods which can be overwritten to support both clusters requirements (Local cluster, networking differences, etc.)
@arithx arithx force-pushed the arithx:qemu_2_electric_boogaloo branch from 6712ac9 to 03e954c Feb 21, 2019
@cgwalters
cgwalters reviewed Mar 4, 2019
View changes
Copy link
Member

cgwalters left a comment

And we're intending to support unprivileged-qemu for CL as well?

I only skimmed this...it seems sane. My biggest concern is the long term overlap between this and cosa's qemu bits but we'll figure that out somehow.
platform/machine/unprivqemu/cluster.go Outdated
return nil, err
}

// parse /proc/net/tcp to determine the port selected by QEMU

This comment has been minimized.

Sign in to view
Copy link
@cgwalters

cgwalters Mar 4, 2019

Member

Eeek. Ideally there'd be a way for us to bind the socket and pass it to qemu...may require a qemu patch though.

Or maybe the port is exposed over QMP?

This comment has been minimized.

Sign in to view
Copy link
@arithx

arithx Mar 4, 2019

Author Contributor

A specific port can be selected and given in the hostfwd parameter, but it seemed easier to allow QEMU's auto-selection code to be used and then determine what they were using.

As for using QMP I delved into it for a bit and while it is probably possible I didn't find an easy/clean way of doing it that way.
@arithx

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

arithx commented Mar 4, 2019

Yes, unprivileged-qemu works on all supported distributions.
@arithx arithx force-pushed the arithx:qemu_2_electric_boogaloo branch from 03e954c to 09eecaa Mar 15, 2019
@cgwalters

This comment has been minimized.

Sign in to view
Copy link
Member

cgwalters commented Mar 29, 2019

I gave this a try and it seems to work for me. Let's ship it?
@cgwalters

This comment has been minimized.

Sign in to view
Copy link
Member

cgwalters commented Apr 5, 2019

Hey so...what's blocking this?
@cgwalters

This comment has been minimized.

Sign in to view
Copy link
Member

cgwalters commented Apr 5, 2019

I don't have merge rights to mantle it looks like. Any objections to adding me?

From my PoV @arithx this is also good to merge.
@bgilbert

This comment has been minimized.

Sign in to view
Copy link
Member

bgilbert commented Apr 5, 2019

@cgwalters It's blocked on a full code review. 😄
@ajeddeloh
ajeddeloh reviewed Apr 11, 2019
View changes
Copy link
Contributor

ajeddeloh left a comment

Couple questions, overall LGTM.
platform/machine/unprivqemu/cluster.go Outdated Show resolved Hide resolved
cmd/kola/spawn.go
@@ -174,7 +174,7 @@ func doSpawn(cmd *cobra.Command, args []string) error {
return fmt.Errorf("Could not read machine options: %v", err)
}

var machineOpts qemu.MachineOptions
var machineOpts platform.MachineOptions

This comment has been minimized.

Sign in to view
Copy link
@ajeddeloh

ajeddeloh Apr 11, 2019

Contributor

I think we should find a better home for this than platform, though not sure if that's worth blocking this PR.

This comment has been minimized.

Sign in to view
Copy link
@arithx

arithx Apr 11, 2019

Author Contributor

If I recall correctly I originally had it in a platform/qemu sub-package but I don't remember why I changed it back to just being in platform. I wouldn't personally be opposed to either having it in a sub-package or leaving it inside of platform.

@bgilbert did you have a particularly strong opinion either way?

This comment has been minimized.

Sign in to view
Copy link
@bgilbert

bgilbert Apr 29, 2019

Member

platform seems like the obvious place for it. I don't think it's important to start a subpackage just for these structs. I'm okay with other approaches though.

This comment has been minimized.

Sign in to view
Copy link
@arithx

arithx Apr 30, 2019

Author Contributor

I'll leave it in it's current place for this PR and we can decide later if we want to move it.
kola/harness.go Outdated
checkPlatforms := []string{pltfrm}

// unprivileged-qemu has the same restrictions as QEMU but might also want additional restrictions due to the lack of a Local cluster
if pltfrm == "unprivileged-qemu" {

This comment has been minimized.

Sign in to view
Copy link
@ajeddeloh

ajeddeloh Apr 11, 2019

Contributor

nit for later, not this PR, we should define these as const strings in each platform.
kola/tests/misc/raid.go Outdated Show resolved Hide resolved
@bgilbert
bgilbert reviewed Apr 29, 2019
View changes
cmd/kola/options.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cmd/kola/spawn.go
@@ -174,7 +174,7 @@ func doSpawn(cmd *cobra.Command, args []string) error {
return fmt.Errorf("Could not read machine options: %v", err)
}

var machineOpts qemu.MachineOptions
var machineOpts platform.MachineOptions

This comment has been minimized.

Sign in to view
Copy link
@bgilbert

bgilbert Apr 29, 2019

Member

platform seems like the obvious place for it. I don't think it's important to start a subpackage just for these structs. I'm okay with other approaches though.
@arithx arithx force-pushed the arithx:qemu_2_electric_boogaloo branch 2 times, most recently from 4997e43 to 849fcf3 Apr 30, 2019
@arithx

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

arithx commented Apr 30, 2019

Update pushed addressing comments
@jlebon
jlebon reviewed May 8, 2019
View changes
Copy link
Member

jlebon left a comment

Not super familiar with the codebase, though looks sane to me at a high-level! Just some minor notes.

Would be cool to get this in and hooked up into the FCOS pipeline.
platform/qemu.go Show resolved Hide resolved
kola/harness.go Show resolved Hide resolved
kola/tests/docker/docker.go Outdated Show resolved Hide resolved
@arithx
platform/machine/qemu: centralize qemu code
48c77e8 
Prepares to add the unprivileged-qemu platform by moving the qemu code
to a centralized location.
@arithx arithx force-pushed the arithx:qemu_2_electric_boogaloo branch from 849fcf3 to b4fbb63 May 8, 2019
@arithx
*: add unprivileged-qemu platform
Loading status checks…
67b9c0d 
Adds a new platform `unprivileged-qemu` which is meant to be ran without
root access. It has a restricted set of functionality, such as a lack of
the Local cluster and no networking between machines.
@arithx arithx force-pushed the arithx:qemu_2_electric_boogaloo branch from b4fbb63 to 67b9c0d May 8, 2019
@arithx

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

arithx commented May 8, 2019

Rebased, updated per comments, and pushed
@ajeddeloh
ajeddeloh approved these changes May 8, 2019
View changes
Copy link
Contributor

ajeddeloh left a comment

LGTM
@arithx arithx merged commit fedb5c5 into coreos:master May 13, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arithx arithx deleted the arithx:qemu_2_electric_boogaloo branch May 13, 2019
@arithx arithx mentioned this pull request May 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters

@bgilbert bgilbert

@jlebon jlebon

@ajeddeloh ajeddeloh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@arithx @cgwalters @bgilbert @ajeddeloh @jlebon
You can’t perform that action at this time.