Make snap-discard-ns fail gracefully #151

Merged
merged 6 commits into from Sep 23, 2016

Conversation

Projects
None yet
2 participants
Collaborator

zyga commented Sep 22, 2016

This patch changes snap-discard-ns to gracefully fail (as in, return
successfully) when any of the following three conditions happen:

  • the /run/snapd/ns directory doesn't exist yet
  • the /run/snapd/ns/$SNAP_NAME.mnt file doesn't exist
  • the /run/snapd/ns/$SNAP_NAME.mnt file is not a mount point

This allows snapd to check for abnormal errors from snap-discard-ns.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

Make snap-discard-ns fail gracefully
This patch changes snap-discard-ns to gracefully fail (as in, return
successfully) when any of the following three conditions happen:

- the /run/snapd/ns directory doesn't exist yet
- the /run/snapd/ns/$SNAP_NAME.mnt file doesn't exist
- the /run/snapd/ns/$SNAP_NAME.mnt file is not a mount point

This allows snapd to check for abnormal errors from snap-discard-ns.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Please adjust sc_open_ns_group() to take an arg instead as per inline comments

src/ns-support.c
+ if (errno == ENOENT) {
+ free(group);
+ return NULL;
+ }
@jdstrand

jdstrand Sep 22, 2016

Contributor

This function is identical to sc_open_ns_group() except for this ENOENT check. It would be more maintainable to instead modify sc_open_ns_group() to take a flag and decide how to error based on the flag. Eg:

struct sc_ns_group *sc_open_ns_group(const char *group_name, const int graceful_enoent)
{
...
    if (group->dir_fd < 0) {
                if (graceful_enoent == 1 && errno == ENOENT) {
                        free(group);
                        return NULL;
                }
                die("cannot open directory for namespace group %s", group_name);
        }
...

Then sc-main.c uses:

group = sc_open_ns_group(group_name, 0);

and snap-discard-ns.c uses:

group = sc_open_ns_group(group_name, 1);

Obviously feel free to rename graceful_enoent and be sure to document this argument.

@zyga

zyga Sep 22, 2016

Collaborator

Will do, thank you for the review

zyga added some commits Sep 22, 2016

Add flags to sc_ns_open_group(), discard maybe_ variant
This patch discard the duplicated function and uses a new flags argument
to differentiate between the graceful return and immiediate exit error
handling.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Use unsigned integers as flags
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Collaborator

zyga commented Sep 22, 2016

Spread failure looks related to packaging. I will fix this separately and improve spread setup so that we see what actually failed.

@@ -155,7 +155,7 @@ static struct sc_ns_group *sc_test_open_ns_group(const char *group_name)
if (group_name == NULL) {
group_name = "test-group";
}
- group = sc_open_ns_group(group_name);
+ group = sc_open_ns_group(group_name, 0);
g_test_queue_destroy((GDestroyNotify) sc_close_ns_group, group);
// Check if the returned group data looks okay
g_assert_nonnull(group);
@jdstrand

jdstrand Sep 22, 2016

Contributor

+1 provided you add a test for sc_open_ns_group(group_name, SC_NS_FAIL_GRACEFULLY)

@zyga

zyga Sep 23, 2016

Collaborator

Will do, thank you! :)

zyga added some commits Sep 23, 2016

Discard hello-world ns after every test
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Fix typo
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Add test for graceful sc_open_ns_group failure
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga merged commit 2b0ae9e into master Sep 23, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Collaborator

zyga commented Sep 23, 2016

I'm merging this because the test failures are not really related to the change being made. Analysis indicates that ubuntu-core is being unmounted at some (random) point during the test process and depending on how fast or slow the VM is, tests just randomly fail when that happens. Snap-confine has no code that performs sanity checks on the presence of the core snap.

@zyga zyga deleted the resilience branch Sep 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment