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

overlord,tests: expose enabled features to /var/lib/snapd/features #6170

Closed
wants to merge 14 commits into from

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Nov 19, 2018

This is a re-iteration of the earlier facts patch-set. Instead of a facts file
we now have flag files. The files are empty and are stored in
/var/lib/snapd/features. I also added the feature flag for per-user mount
namespaces. This will be handy in upcoming tests.

I chose not to change packaging to reduce the impact since the directory is
dynamically created. I will %ghost the directory later in RPM-specific
packages, before the next release.

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

zyga and others added 8 commits November 19, 2018 13:38
The features directory is designed to store marker files for optional
features that are activated in snapd that need to be checked by
snap-update-ns or other programs that cannot read the state file
directly.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This patch moves the list of known experimental flags to the new file
where we will also soon handle exporting them to C parts of snapd.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Experimental flags are available in snapd but, before this patch, not in
the C world of snap-confine and state-agnostic world of snap-update-ns.

This patch fixes this by exporting core.experimental.* flags to
/var/lib/snapd/features/$name. Features are simply empty files that are
read by compatible C and Go code.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This feature flag toggles persistence and updates of the per-user
mount namespaces. Until this feature is finished, tested and deemed
stable it will remain off.


Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, some tiny nitpicks/questions but otherwise good to go.

overlord/configstate/configcore/experimental.go Outdated Show resolved Hide resolved
}

var _ = Suite(&experimentalSuite{
features: []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just get this from "experimental.go" via export_test.go and the experimentalFlags that are defined there? As it is right now it misses the new per-user-mount-namespace as part of th test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to do a follow-up and use the features/ package as a canonical defining set, does that sound sensible?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah

test -f /var/lib/snapd/features/layouts

snap set core experimental.layouts=false
test ! -f /var/lib/snapd/features/layouts
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick^2) most other tests seem to use ! at the start of the line for a negative test like this. Is there any particular advantage of one vs the other?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This way test handles the negation but I only really do it because it reads more natural (to me) when written left-to-right. "test that it is not the case that there is a file named /var/lib/snapd/features/layouts". I don't have a strong preference, it's just how I wrote it.

overlord/configstate/configcore/corecfg.go Outdated Show resolved Hide resolved
cmd/snap-mgmt/snap-mgmt.sh.in Show resolved Hide resolved
overlord/configstate/configcore/corecfg.go Outdated Show resolved Hide resolved
return err
}
if value == "true" {
content[flag] = &osutil.FileState{Mode: 0644}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it'd be useful to write something like true to the file too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, because that changes how we work with the file.

Stat doesn't need any permissions and doesn't do any file IO. If we want to do IO I will kindly recover the original facts PR which handled more useful things...

overlord/configstate/configcore/experimental_test.go Outdated Show resolved Hide resolved
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for extending the tests.

@zyga
Copy link
Collaborator Author

zyga commented Nov 22, 2018

Closing to integrate with #6181

@zyga
Copy link
Collaborator Author

zyga commented Nov 22, 2018

This is now proposed, unified, as #6190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants