Skip to content
This repository was archived by the owner on Apr 3, 2018. It is now read-only.

9p: Do not pass the rootfs through 9pfs#499

Merged
sboeuf merged 1 commit into
containers:masterfrom
amshinde:do-not-pass-rootfs-9p
Dec 1, 2017
Merged

9p: Do not pass the rootfs through 9pfs#499
sboeuf merged 1 commit into
containers:masterfrom
amshinde:do-not-pass-rootfs-9p

Conversation

@amshinde
Copy link
Copy Markdown
Collaborator

We pass the shared directory through 9p in hyperstart.go and
handle mounting the rootfs or the underlying block device in the agent.
We do not really need this extra 9p share and should avoid it.

Signed-off-by: Archana Shinde archana.m.shinde@intel.com

@amshinde amshinde requested a review from sboeuf November 30, 2017 01:12
@grahamwhaley
Copy link
Copy Markdown
Contributor

CIs failed - looks like an unrelated code mismatch - maybe something needs a rebase or something else is in flight. Here is the error:

go build -i -o cc-runtime .
  # github.com/clearcontainers/runtime
  ./cc-env.go:205: proxyConfig.URL undefined (type virtcontainers.CCProxyConfig has no field or method URL)
  ./config.go:306: unknown field 'URL' in struct literal of type virtcontainers.CCProxyConfig
  Makefile:268: recipe for target 'cc-runtime' failed

maybe @jodh-intel or @sboeuf know what might have caused that?

Other than that - just to check, does this change presume that we always pass the root dir as a block device, and is that true for all cases?

@jodh-intel
Copy link
Copy Markdown
Collaborator

Can you provide a bit more detail on this? How did we discover the duplication? Should this strictly have been removed in a previous PR (oversight)?

Also, the tests still reference ctr-rootfs and ctr-9p so will need to be updated I think.

@jodh-intel
Copy link
Copy Markdown
Collaborator

@grahamwhaley - the CI issue is related to clearcontainers/runtime#835 not being merged (and thus affecting all recent vc PRs). That PR is now merged, so I've kicked off a rebuild for this PR...

@jodh-intel
Copy link
Copy Markdown
Collaborator

@grahamwhaley, @sboeuf - ... which reminds me, are we missing a bit of Jenkins config? The 3 CI builds I've re-triggered for this PR are currently queued, but I'd still expect the red crosses on this PR to show the mustard-coloured "in-progress" icons pretty much immediately?

@grahamwhaley
Copy link
Copy Markdown
Contributor

@jodh-intel I think you are probably right - that is most likely a non-configured 'trigger' entry for the github pull request builder plugin I suspect - but .... oh, and as I am typing this they have changed.... so, maybe it is OK after all (it could have been Azure spinup time maybe...)

@jodh-intel
Copy link
Copy Markdown
Collaborator

@grahamwhaley - the 3 builds are now executing so it does seem that if it's queued, you'll still see the red fail cross". I have hit rebuild on a PR due to this only to find that there are one or more identical rebuild requests in the queue so if we could resolve this issue, we'll be saving some resources ;)

@sboeuf
Copy link
Copy Markdown
Collaborator

sboeuf commented Nov 30, 2017

@jodh-intel when you trigger a re-build from Jenkins, Github sees it only when the Azure VM is up and running and the build script is running. The whole part where Jenkins starts a new VM and waits for it to be online does not change the Github status.

@jodh-intel
Copy link
Copy Markdown
Collaborator

That's pretty much what I guessed. So presumably there is no way to trigger a github update as soon as the rebuild button is pressed to show it's "queued"?

@sboeuf
Copy link
Copy Markdown
Collaborator

sboeuf commented Nov 30, 2017

@jodh-intel well I didn't find a way, but this does not mean it's not feasible :)

@grahamwhaley
Copy link
Copy Markdown
Contributor

I suspect not @jodh-intel - those status items are controlled/updated by the specific jenkins plugin, and it probably doesn't get prodded until the 'slave' machine has been chosen and run up, and hence the potential delay waiting for Azure. You'll just have to be patient :-)

We pass the shared directory through 9p in hyperstart.go and
handle mounting the rootfs or the underlying block device in the agent.
We do not really need this extra 9p share and should avoid it.
The 9p share was being passed twice prior to this change.

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde force-pushed the do-not-pass-rootfs-9p branch from 1a6cc2b to b0dd4c6 Compare November 30, 2017 20:35
@amshinde
Copy link
Copy Markdown
Collaborator Author

@jodh-intel This 9p share not have been there in the first place. We were passing rootfs through qemu as well as hyperstart with different 9p tags. The 9p share passed in qemu.go ("ctrl-9p") was essentially unused. I dont know when this change was introduced, but I have seen it right from the time I started taking a look at 3.0, just got around to getting rid of it :)

@jodh-intel
Copy link
Copy Markdown
Collaborator

jodh-intel commented Dec 1, 2017

Thanks @amshinde. Great to see code being removed, particularly 9p-related code!! 😄

lgtm

Approved with PullApprove Approved with PullApprove

Copy link
Copy Markdown
Collaborator

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

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

I agree this function appendFSDevices() should only add potential volumes, but there is no reason for the rootfs to be passed through this (additionally to what the agent implementation could do).

@sboeuf
Copy link
Copy Markdown
Collaborator

sboeuf commented Dec 1, 2017

LGTM

@sboeuf sboeuf merged commit fe2699b into containers:master Dec 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants