Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

tests: add smoke test for app sandbox #3371

Merged
merged 2 commits into from Nov 28, 2016

Conversation

lucab
Copy link
Member

@lucab lucab commented Nov 14, 2016

Part of #3349

@lucab lucab added this to the v1.20.0 milestone Nov 14, 2016
@lucab lucab force-pushed the to-upstream/app-smoke-test branch 3 times, most recently from b234bd9 to c6be790 Compare November 15, 2016 13:28
@lucab
Copy link
Member Author

lucab commented Nov 15, 2016

First iteration of this uncovered a bug on kvm, where app-add was trying to remount host cgroups from inside the vm.

@lucab
Copy link
Member Author

lucab commented Nov 15, 2016

Second iteration of this uncovered a bug on non-overlayfs pod, where app-rm was trying to read the non-existing TreeStoreID file.

@lucab
Copy link
Member Author

lucab commented Nov 15, 2016

Third iteration of this uncovered an issue on environment handling, the entrypoints are not receiving a proper $PATH. kvm currently fails for this reason.

@lucab
Copy link
Member Author

lucab commented Nov 17, 2016

Fourth iteration of this hit a race in KVM, due a too low thread limit. Tracked as #3382. This unfortunately happens from time to time at daemon-reload time, causing some non-deterministic test failures. I'm disabling the test on KVM for the moment and will re-enable once the limit issue is fixed.

@lucab lucab force-pushed the to-upstream/app-smoke-test branch 2 times, most recently from 0d6f833 to 64af74f Compare November 17, 2016 14:27
@lucab
Copy link
Member Author

lucab commented Nov 22, 2016

Fifth iteration of this hit a problem on rm, as the unmounting tasks are still TODO. As the mounts are done in stage1, this unfortunately means that systemd+journald in stage1 still hold references to the application mounts (custom volumes and ancillary mounts like procfs, sysfs, devs, etc.). Coupled with #1922 (semaphore is running on a hold 3.13 kernel), it means that stage1 pod processes are holding those rootfs paths busy as bind-mounts targets and thus stage0 can't remove rootfs directory.

This requires proper cleaning of all mountpoints under app rootfs by some stage1 helper before proceeding.

@lucab
Copy link
Member Author

lucab commented Nov 23, 2016

Sixth iteration hit a misbehavior on mounting/unmounting, due to the lack of /etc/mtab in stage1 rootfs.

@s-urbaniak s-urbaniak modified the milestones: v1.21.0, v1.20.0 Nov 24, 2016
@s-urbaniak
Copy link
Contributor

still WIP, hence bumping to the next release

@lucab
Copy link
Member Author

lucab commented Nov 24, 2016

I split the first batch of fixes from here into #3405. I'm happy enough that most of the things here are addressed for rkt 1.20.0. The main remaining blocker at this point is #3411, which is unfortunately big enough to require its own task.

@lucab lucab force-pushed the to-upstream/app-smoke-test branch 3 times, most recently from e88667f to eb7b760 Compare November 25, 2016 13:02
@lucab lucab force-pushed the to-upstream/app-smoke-test branch 2 times, most recently from b23d941 to c50a873 Compare November 25, 2016 16:49
@lucab
Copy link
Member Author

lucab commented Nov 25, 2016

This now uses #3415 to fix #3411.

Test should be finally green 🎉, PTAL.

args = enterCmd
args = append(args, "/usr/bin/systemctl")
args = append(args, "daemon-reload")
// TODO(sur): find all RW cgroups exposed for this app and clean them up
Copy link
Member Author

Choose a reason for hiding this comment

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

@s-urbaniak: this TODO is in place here as a reminder to re-visit the rm steps once #3389 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack 👍

Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

just a few nits

)

func init() {
flag.StringVar(&flagApp, "app", "", "Application name")
flag.BoolVar(&debug, "debug", false, "Run in debug mode")

// `--phase` is not part of stage1 contract
flag.IntVar(&flagPhase, "phase", 0, "Removal phase, defaults to 0 when called from the outside")
Copy link
Contributor

Choose a reason for hiding this comment

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

"phase 0" and "phase 1" are very abstract. We have established and documented terms for stages, where "stage 0" happens on the host, and "stage 1" happens in the context of the pod.

Can we either make the context of phases more explicit in the documentation, or even call this flag stage?

Copy link
Member Author

@lucab lucab Nov 28, 2016

Choose a reason for hiding this comment

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

I kind of agree with this comment, but I also the fear that some degree of confusion will remain anyway: app-rm is a stage1 entrypoint, however half of it (phase0/stage0) is run on the host and half of it (phase1/stage1) in the pod context.

I'll perform a s/phase/stage/ because it helps reducing the number of concepts and reflecting where this is being run.

case 1:
// phase1: app-rm:phase0 -> app-rm:phase1
err = cleanupPhase1(appName, enterCmd)
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

just a nit, but I suggest to make the set of supported cleanup phases (or cleanup in stages) more explicit and fail for an unsupported phase:

switch flagPhase {
  case 0:
     // cleanup things in stage0
  case 1:
    // cleanup things in stage1
  default:
    log.FatalF("unsupported phase %d", flagPhase)
}

}

// cleanupPhase0 is default phase for rm entrypoint, performing
// initial cleaning steps which don't custom logic in pod context.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's document here, that all of this explicitly happens in stage0, i.e.:

This cleanup phase happens in stage 0.
It removes the app service files and calls:
1. `systemctl daemon-reload` in stage1
2. itself in stage1 for phase 1 cleanups

args = enterCmd
args = append(args, "/usr/bin/systemctl")
args = append(args, "daemon-reload")
// TODO(sur): find all RW cgroups exposed for this app and clean them up
Copy link
Contributor

Choose a reason for hiding this comment

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

ack 👍


// TODO unmount all the volumes
// cleanupPhase1 inspects pod systemd-pid1 mountinfo to find all remaining
Copy link
Contributor

Choose a reason for hiding this comment

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

// cleanupPhase1 is executed in stage1 and inspects the pod's systemd-pid1 moutinfo ....

@lucab lucab force-pushed the to-upstream/app-smoke-test branch 2 times, most recently from 82a21ef to 2ad36c1 Compare November 28, 2016 10:34
@lucab
Copy link
Member Author

lucab commented Nov 28, 2016

@s-urbaniak Thanks for the review, much appreciated! Rebased to address your comments.

Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

LGTM 👍

}

// cleanupStage0 is the default initial step for rm entrypoint, which takes
// cares of cleaning up resources in stage0 and calling into stage1 by:
Copy link
Contributor

Choose a reason for hiding this comment

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

// takes care

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good catch.

@@ -59,7 +69,31 @@ func main() {
}

enterCmd := stage1common.PrepareEnterCmd(false)
switch flagStage {
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling this stage0 vs stage1 is a bit confusing - it only refers to the mount namespace (stage0 vs. stage1) - even the "stage 0" cleanup code is removing things created by the mass of code we call "the stage1".

Perhaps a better naming scheme is in order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Those were called phase0/phase1, then renamed to stage0/stage1 to reflect the context they are running in. See #3371 (comment). Another option would be --context=parent and --context=pod to also make explicit that part of this are running in a different context. @s-urbaniak @jonboulle opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find stage0 less confusing than phase0 ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've left this as is for the moment. This an internal implementation anyway, perhaps we can make this clearer down the road if we have other similar cases to document/classify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems reasonable to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

@casey LGTY?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, NBD.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants