many: derive implicit slots from interface meta-data #3370

zyga · May 22, 2017

This patch moves the declaration that a given interface should have an
implicit slot on the core snap from snap/implicit.go into each
particular interface definition file. This brings us closer to having
all the relevant code for a given interface be entirely defined in the
interface file itself.

The fuse-support interface had some extra complexity that was now merged
into the interface definition (it is not implicitly added on ubuntu
14.04). The interface code had to be changed from commonInterface to a
new dedicated type.

The code that adds interfaces is now moved to overlord/ifacecstate and
made private (it is only relevant there and it avoids cyclic imports).

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

pedronis · May 22, 2017

pedronis reviewed May 22, 2017

View changes

can't we try to address the problem that is still brittle given that we are changing things ?

Yes but this is not something I wish to do in this branch. The goal right now is to reduce the number of files that need to be patched to create a new interface. Addressing the implicit slot mechanism can and should be done separately IMO. I think this branch does improve the situation slightly as it moves the code closer to the interface manager where ultimately all the interface decisions are being taken.

codecov-io · Jun 5, 2017

Codecov Report

Merging #3370 into master will decrease coverage by 0.34%.
The diff coverage is 65.62%.

@@            Coverage Diff             @@
##           master    #3370      +/-   ##
==========================================
- Coverage   77.56%   77.21%   -0.35%     
==========================================
  Files         371      372       +1     
  Lines       25519    25630     +111     
==========================================
- Hits        19793    19791       -2     
- Misses       3976     4089     +113     
  Partials     1750     1750

Impacted Files	Coverage Δ
snap/implicit.go	`81.81% <ø> (-3.19%)`	⬇️
interfaces/builtin/pulseaudio.go	`50% <0%> (-9.26%)`	⬇️
interfaces/builtin/docker_support.go	`64.86% <0%> (-9.43%)`	⬇️
interfaces/builtin/lxd_support.go	`76.19% <0%> (-8.03%)`	⬇️
interfaces/builtin/io_ports_control.go	`80% <0%> (-10.91%)`	⬇️
interfaces/builtin/upower_observe.go	`79.06% <0%> (-6.65%)`	⬇️
interfaces/builtin/physical_memory_observe.go	`78.12% <0%> (-11.88%)`	⬇️
interfaces/builtin/time_control.go	`78.12% <0%> (-11.88%)`	⬇️
interfaces/builtin/uhid.go	`63.33% <0%> (-4.53%)`	⬇️
interfaces/builtin/joystick.go	`73.07% <0%> (-14.43%)`	⬇️
... and 106 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d455a61...f4a686a. Read the comment docs.

mvo5 · Jun 7, 2017

mvo5 reviewed Jun 7, 2017

View changes

Looks good, I welcome the change to only have to touch a single file instead having to touch things all over the map when implementing a new interfaces. Some ideas/questions, but the code looks good, maybe worthwhile to get a quick look from @niemeyer about the design of this.

Is there/will there ever be a case where we have implicitOnCore: false, implicitOnClassic: true ? The old code had implicit and implicitOnlyOnClassic only. Mostly wondering. Also wondering if: implicit: All, implicit: CoreOnly, implicit: ClassicOnly might look nicer (i.e. using bits).

There are things that are implicit on classic, yes. I changed this from the previous code to make it less magic and more obvious as to what is added. I don't mind a single implicit bit-flag but I'd rather do that in a separate branch to allow this one to land and iterate.

Why is this not just adding something like:

registerIface(&commonInterface{
...
ImplicitOnClassic: !(release.ReleaseInfo.ID == "ubuntu" && release.ReleaseInfo.VersionID == "14.04"),
...
}

i.e. why did it have to be converted to a new type?

Indeed, I think it could be a common interface.

Done

It seems this file is pretty empty now, why do we still have it?

There are some things that we could test, I think. I can remove it or fill in the tests.

jdstrand · Jun 7, 2017

jdstrand approved these changes Jun 7, 2017

View changes

+1 to land as is. The interfaces are all in the right place. I did request an additional test. I also agree with others that need to fill in two variables for something that is implicit everywhere is slightly awkward. Please consider making this easier either as part of this PR or a (soonish) upcoming PR.

This looks like an unrelated change. I'm not saying it is wrong.... It seems that this sort of thing should be captured elsewhere. Eg, I can imagine Fedora or SUSE might have different interfaces. I mention this only because if going that direction, maybe it's best to not include this change in this PR.

This is not an unrelated change. It used to be defined exactly this way in implicit.go before.

Can we test that something only on classic is not in core?

Yes, sure :-)

zyga deleted the zyga:feature/metadata-defines-implicitness branch Jun 8, 2017

-        -	// XXX: AddImplicitSlots is really a brittle interface
-        -	snap.AddImplicitSlots(snapInfo)
+        +	// XXX: addImplicitSlots is really a brittle interface
+        +	addImplicitSlots(snapInfo)

@@ -72,6 +72,8 @@ func init() {
          		name:                  "account-control",
          		summary:               accountControlSummary,
          		description:           accountControlDescription,
+        +		implicitOnCore:        true,

-        -		summary:               fuseSupportSummary,
-        -		connectedPlugAppArmor: fuseSupportConnectedPlugAppArmor,
-        -		connectedPlugSecComp:  fuseSupportConnectedPlugSecComp,
-        -		reservedForOS:         true,

-        -	// Ensure that we have *some* implicit slots
-        -	c.Assert(len(info.Slots) > 10, Equals, true)
-        -}
+        +type implicitSuite struct{}

          	registerIface(&commonInterface{
          		name:                  "fuse-support",
          		summary:               fuseSupportSummary,
+        +		implicitOnCore:        true,
+        +		implicitOnClassic:     !(release.ReleaseInfo.ID == "ubuntu" && release.ReleaseInfo.VersionID == "14.04"),

snapcore/snapd

many: derive implicit slots from interface meta-data #3370

zyga commented May 22, 2017 •

Edited 1 time

zyga
May 22, 2017

pedronis reviewed May 22, 2017

View changes

codecov-io commented Jun 5, 2017 •

Edited 1 time

codecov-io
Jun 8, 2017

mvo5 reviewed Jun 7, 2017

View changes

jdstrand approved these changes Jun 7, 2017

View changes

zyga merged commit `8390a6c` into snapcore:master Jun 8, 2017
4 of 7 checks passed

4 of 7 checks passed

zyga deleted the zyga:feature/metadata-defines-implicitness branch Jun 8, 2017

snapcore/snapd

Join GitHub today

many: derive implicit slots from interface meta-data #3370

Conversation

zyga commented May 22, 2017 • edited Edited 1 time zyga May 22, 2017

All checks have failed

pedronis reviewed May 22, 2017 View changes

All checks have passed

codecov-io commented Jun 5, 2017 • edited Edited 1 time codecov-io Jun 8, 2017

Codecov Report

mvo5 reviewed Jun 7, 2017 View changes

zyga Jun 7, 2017 • edited Edited 1 time zyga Jun 7, 2017

Some checks were not successful

jdstrand approved these changes Jun 7, 2017 View changes

Some checks were not successful

Hide details View details zyga merged commit 8390a6c into snapcore:master Jun 8, 2017 4 of 7 checks passed

4 of 7 checks passed

zyga deleted the zyga:feature/metadata-defines-implicitness branch Jun 8, 2017

zyga commented May 22, 2017 •

Edited 1 time

zyga
May 22, 2017

pedronis reviewed May 22, 2017

View changes

codecov-io commented Jun 5, 2017 •

Edited 1 time

codecov-io
Jun 8, 2017

mvo5 reviewed Jun 7, 2017

View changes

zyga Jun 7, 2017 •

Edited 1 time

zyga
Jun 7, 2017

jdstrand approved these changes Jun 7, 2017

View changes

zyga merged commit `8390a6c` into snapcore:master Jun 8, 2017
4 of 7 checks passed