Thanks for the review Christian! Sorry for the delayed answer, I'm just back from holidays :)
Thanks also for the details and summary. I think I have some resolved, some questions and some with no actions. I copy this back here so that we can track them. Let me know how this feels.


[Summary]

> - go issue of embedded libs, can we resolve reduce this?

I think there is no action needed here. Look at the details below for a (long) explanation.

> - I know it makes no sense in a container, but fix it so that it properly
>  installs by changing default config/postinst or whatever you see fit

See below, I need an example/more details of what is actually expected.

> - add watch file to your github

Done in master (https://github.com/ubuntu/zsys/commit/c390e46aa951c71b4154d769497a4a47e44aaae9). However this triggers a lintian warning: W: zsys source: debian-watch-file-in-native-package and I don't really like keeping lintian warnings.

> - please resolve the lintian copyright complains

Need feedback from you.

> - needs a security review

Pending

> - enable more isolation features for for zsys-commit.service please

Done, see below if this is good for you.

> - add some documentation

I guess not needed for eoan as there is nothing user visible (see more details below)

> - extend autopkgtests to cover some real use-cases

This is planned but not real for targetting eoan TBH. As you saw, we have a very extensive testsuite (similary to ubuntu-report) and are really hard on testing (running as part of building the package, but also in term of upstream CI).
However, running on a real zfs-on-root system in autopkgtests is quite hard to setup right now. This is why before zsys go "production ready", we really need this (so that I can sleep at night ;)), knowing though that it needs a lot of fundamental work to be setup on autopkgtests system. We already have some autopkgtests to ensure we are compatible with our libzfs shipped by ubuntu and that new zfs or grub doesn't regress us. But here, it means installing a real entire system, with zfs on root (so debootstrapping or copying current system, creating and accessing disks…), changing grub boot menu entry for next reboot and cross-reboot tests (I worked with pitti in the past to do this last item already for systemd autopkgtests, so it's not the most worrying part compared to initial setup). This is logged as https://github.com/orgs/ubuntu/projects/1#card-24794108. In a nutshell, I'm afraid that's not a realistic target for eoan (and part of the reason we keep it under the "experimental" flag).

> - this seems to be at a very early stage, every here and there more features
  are mentioned but not yet implemented. To review this it should sort of
  feature complete at least to the extend that are planned for the comi,ng
  release. Otherwise reviews cover what is there in 0.1 but e.g. 1.0 will
  look much much different (If the feature is too far out at least add the
  docs and specs to check if the design fits main inclusion)

Indeed. We had a plan that you can see on https://github.com/orgs/ubuntu/projects/1. However, we didn't get the time resources (even less than half of what expected) we budgeted on at the start of the cycle due to other customer work which got high priority. We thus reduced to a MVP allowing people installing a zfs system on root, with separated boot partition, enabling multiple (manual) snapshots for rollbacking system AND/OR user data attached to it. This experimental option will already help us a lot in term of exploring, testing and hardening zfs as a viable root option before turning it to stable.
The most complex logic (grub + zsys boot) is now implemented. This fundamental work is the most complex as it means datasets managements, grouping them logically in different machines,-  cloning system when booting on snapshot, handling parallel installations, handling partial "system only" revert.
The internal API for interacting with ZFS itself is already there and tested, even if not used (destroy, snapshot and other tagging…). It means that future improvement will be:
- implementing CLI options to expose/list what is already available on the backend (installed machines, available snapshots, takes a snapshot and more). However no "background work" should be needed and thus, this is just "cosmetic" features if you prefer.
- implementing a GUI, which will use the exact same API than the CLI.
- doing some background automated snapshots, which is just going to be a systemd unit triggering the same user command than manual snapshots. Tweaking those will be done via systemd timer units.
- separate (which is already separated in code: internal/ vs cmd/) CLI and backend, so that we have an on-demand zsys single daemon (avoiding having multiple parallel commands starting in parallel for integrity, a single daemon will know the state of the system and interact with it). This client/server local communication will mean that we need some polkit integration. This is the most invasive change in design between now and 1.0 that we have and this means that we, as the Desktop team, will request later on a security review again to ensure we did make it right security-wise.

I gave management the choice between keeping zsys to universe and call for testing and pushing this to main (as we are going to support it, even if experimental). The second option got the favor to get wider testing. It's just that we are going to warn to not use that on production as dataset layout changes (or even data loss?) may incur.

Note that the only "user visible" features are:
- the experimental entry in ubiquity (to come, done by jibel). mpt is giving design feedback on this.
- the grub menu multi-boot entry that is already in eoan for quite some time, enabling you to start your system in a previous snapshot status. Those enhancements are compatible with and without zsys as long as your system is "zfs on root". However, you get more options available if you are using zsys.

There is nothing else which is visible, the list of blog post we aim for publishing during september will highlight the behind the scene concepts and available grub options. This is the reason why no content for help is shown. Actually if you execute "zsys boot -h", you have more entries, but those are hidden from the general help as not intended for direct use, but only as a systemd service. However, those are no-op if executed again by the user once the system has booted. Some tests are there as well to ensure that the internal API called by those respects this condition (https://github.com/ubuntu/zsys/blob/master/internal/machines/machines_test.go#L123, https://github.com/ubuntu/zsys/blob/master/internal/machines/machines_test.go#L253, https://github.com/ubuntu/zsys/blob/master/internal/machines/machines_test.go#L287, https://github.com/ubuntu/zsys/blob/master/internal/machines/machines_test.go#L324, https://github.com/ubuntu/zsys/blob/master/internal/machines/machines_test.go#L450).

Once the CLI is there (postponed thus to next cycle), you will be able to expect the same standards than ubuntu-report:
- all user-interactive commands in -h, with its subcommands (this is done automatically by the use of cobra) and a detailed description for each of them.
- all non user-interactive (designed for the system) hidden by default, but if you specify it with -h, you see a description of it (already implemented).
- all user-interactive commands and options in shell completion, automatically generated at build-time (see the equivalent implementation in ubuntu-report: https://github.com/ubuntu/ubuntu-report/blob/master/cmd/ubuntu-report/generate_test.go#L49)
- all user-interactive commands and options listed in a generated manpage, at build-time: https://github.com/ubuntu/ubuntu-report/blob/master/cmd/ubuntu-report/generate_test.go#L27
- all user-interactive commands and options automatically updated in the project README: https://github.com/ubuntu/ubuntu-report/blob/master/cmd/ubuntu-report/generate_test.go#L71. Check the "Command line usage" section on https://github.com/ubuntu/ubuntu-report/.
- A more complete description of the project in README and what it can do.

Sorry for being long there, but I hope this gives the context, why we have those decisions, what our standards are (and I hope you can see that they are generally high in both testing and documentation-user interaction), what to expect for 1.0 and help inform better your own decision now :)


[Embedded sources and static linking]
> No static (C) linking, but this is golang and uses the classic approach of bundling a lot of vendor libs.
>
> Of your bundled libs I found almost all in the archive (package names):
> golang-gopkg-yaml.v2-dev golang-golang-x-xerrors-dev golang-golang-x-sys-dev golang-github-spf13-cobra-dev golang-github-spf13-pflag-dev golang-github-sirupsen-logrus-dev golang-github-mattn-go-colorable-dev golang-github-mattn-go-isatty-dev golang-github-konsorten-go-windows-terminal-sequences-dev golang-github-k0kubun-pp-dev golang-github-inconshreveable-mousetrap-dev golang-github-google-go-cmp-dev
>
> The only one I didn't find was vendor/github.com/bicomsystems/go-libzfs/
>
> Of course this would also mean you'd have to MIR all of those as well.
> But that is what the code review here has to do.
> So the amount of code to be verified/reviewed/maintained would not change
> And otherwise things have to maintained multiple times.
>
>  I know ABI instabilities are an issue, but you'd know which release you target.
>  Or are you planning to maintain one version of zsys for all future Ubuntu releases?
>  In that case I see why you bundle the versions, but be aware of the tracking and updating that you'll > have to do for all of them.
>
> Same for the security team, they need to track all those components for potentially many Go packages.

We faced the same issue and questionning for ubuntu-report ourselves. The first upload was relying on system version before going back to vendoring (as those build-deps were all in universe) when MIRing. Note that I restrained my dependencies to be the usual "well-known go libs". However, the very vast majority of them are in universe and as you stated, that would mean MIRing them. At the time of ubuntu-report MIR, Steve advised to rather vendor them (this was on IRC, I can't find the reference back on https://bugs.launchpad.net/ubuntu/+source/ubuntu-report/+bug/1759540) and I prefered as well this approach. This causes of course code duplication, but you should note that most of those libs are still in a 0.x version, and API and ABI breakages actually happens (from experience ;)). This is why the big set of tests is needed and implemented so that when I upgrade the list of deps, I'm sure that the code is still compatible with it.
The situation is better today with go modules: the project list the exact version of dependencies that are needed (commit id or tagged released), so we know what we are testing against as upstream and what downstream should depend on to ensure expected behavior.
Note that most of those deps are also used in snapd, which is using vendoring as well.

Furthermore, as long as we are still in "experimental" phase, we want to provide ppas/way for people to use the newer version (as noted, with the incoming CLI experience and such). Keeping a single version of deps is the only way to provide this kind of support IMHO with our current resources until we reach 1.0.
So, in a nutshell, code maintainability and checking/ensuring we are regularly updating deps is fine. The issue is code duplication for the security team, but this isn't really new as this is the case for snapd, juju, ubuntu-report and probably more (when I see that the yaml parser lib isn't in main for instance, I doubt a lot of go daemons in main aren't using vendoring).


> Currently the rule to keep them bundled is:
> "the requesting team must state their commitment to testing no-change-rebuilds triggered by a dependent > library/compiler and to fix any issues found for the lifetime of the release."
>
> Hence I'd like the Desktop and the Security Team state here on the bug that
> a) they are aware
> b) they are willing and able to do that tracking and updating

This is already what I'm doing for ubuntu-report, so I state explicitely that as part of the Desktop team, I'm aware and willing to do that work.


[Security]
>OK:
> - does not open a port (at least I think so)

Indeed, it does not.

> - runs a daemon as root - sort of ok
>  Just the one that commits the passed boot, but could we throw that one into all sorts of privTMP/priv*** and other systemd restrictions?

I just did this as commit https://github.com/ubuntu/zsys/commit/1bec99f4aa6a84c61f30cf12c83515d40ae578db, can you please have a look if this works for you?


> Security sensitive for:
> - parsing data formats (on disk)
>
> IMHO this is security sensitive as it needs to get access (and can prevent) to most of the disk data.
> If exploited it could pass data on.
> Therefore I request a security review of this as well.

Indeed, that makes sense

[Common blockers]
> OK:

> - build time internal tests seem excessive thanks for those

Yes, while we can run the ./internal/machines tests all in parallel with a mock zfs system, we don't want to do this for the underlying API playing with the real zfs system (./internal/zfs), and so, only one test is running at a time (with one test ~1s in this case: time to setup a chrooted zfs system on temporary files, populates and creating datasets, snapshots…, acting on the test scenario, and cleaning this all up)

> Not bad but improvable:
> - The autopkgtest seems a bit too light, in isolation-machine I'm sure quite a bunch of self-tests could be added right?

See my answer previously, the autopkgtests ensure that grub and zfs don't break us. However, as zsys is only active on zfs-on-root, this is complex in the current time-frame to prioritize the scenarios tests with the current autopkgtests infra.

> - I don't mind so early to not have translations, but do you have the infrastructure in place to do so later?

Yes! I did implement this on weeport experimental project a while ago, which has a po/ directory: https://gitlab.gnome.org/csoriano/weeport/po. It has multiple "modes": "take from po/ directory", "build on time po for testing" and "have all l10n embeeded in the binary". It has as well the "use C gettext rather than internal lib" (which I don't think we'll use for ubuntu).

[Packaging red flags]
> Please improve these:
>  - Install doesn't work by default (not everywhere at least)
>  - it currently doesn't install in containers
>    I know it isn't needed/working there anyway.
>    But please update the packaging in a way that the package installs with a disabled/detecting default config.

I would like some guidance there. As it's using systemd units, how can this will compatible with a docker container? Do you mean only shipping the binary itself in a docker container + deps? As it only use libzfs I'm unsure what you mean by "install doesn't work by default in containers"?

> - no watch file, less bad than usual as it is native, but add one to track your github please

Done, I don't think it will have a strong use, as told, adding it though trigger a lintian warning, let's get rid of it (easier to keep a package lintian-warning free IMHO than allowing one… which calls for another one "just for this time", and so on…)? (in https://github.com/ubuntu/zsys/commit/c390e46aa951c71b4154d769497a4a47e44aaae9)

> - there is a bunch of copyright lintian warnings - it would be great to resolve those on the initial inclusion

Hum, I'm not seeing those on an up to date eoan machine, even in pedantic mode:
$ lintian --pedantic zsys_0.1.dsc 
P: zsys source: package-uses-old-debhelper-compat-version 11
N: 0 tags overridden; 1 unused override
Even, debian fixed the false positive that was requiring me to add a debian/source/lintian-overrides:
# The NOTICE file is there (alongside a LICENSE)
zsys source: missing-notice-file-for-apache-license vendor/gopkg.in/yaml.v2/NOTICE

So, I can remove this now. How do you get those copyright lintian warnings?

> - documentation umm ... where?
>  I know this is WIP, but then we don't promote it at too early stages
>  The -h test is minimal, no man page, nothing in docs.
>  Even the frontpage on https://github.com/ubuntu/zsys still is a bit too light.
>  Before being acceptable there should be something users can start with.

I hope the previous content explains it for you. Zsys only allow but doesn't have itself user-visible features (only grub menu additional entries and ubiquity installer option). The blog post suite will be the start to explain those. I hope that's enough :)


[Upstream red flags]
> Not so good:
> - Embedded source copies (we talked about that already above)

Yep.

[ Notes ]
> This will eventually do much more than just setting the successful boot (zsys-commit.service).
> If I might ask how will this be later integrated into the system?
> Reading https://didrocks.fr/2019/08/06/ubuntu-zfs-support-in-19.10-introduction/ I'm not so sure if:
> a) this is mostly just the commit code
> b) this will grow a lot
> (BTW as mentioned before, more docs would have helped).

I think this was covered above, please shout if it's still not clear.