vgauth needs to start before vmtoolsd

Hi Oliver,

Thank for reporting this bug. Can you put together a repro case for this issue so that we can verify any fixes? In a VM or container is best so that we can be sure of a clean environment.

Hi Oliver,
I can see the case that you are outlining.

I would consider this getting a part of my work on 10.3.5 which I start now and eventually will be backported back to the latest LTS as you know.

But we've had enough issues with these dependencies in the past, so once I have a test version I'd need:
- vmware to check the solution for the fix here
- vmware to do a full regression check (as usual, I'll send the mails)
- our cloud-init people to review and ack and test this as well as it will in turn push cloud-init-local.service to be much later and I want to be sure this is no problem.

I'm especially concerned about currently having intentionally (to be super early)

open-vm-tools.service:
  DefaultDependencies=no
  Before=cloud-init-local.service

Which means as early as possible (no other dependencies) and in front of cloud init.
Which due to the suggested:
  After=vgauth.service
would effectively make open-vm-tools.service to be after DefaultDependencies again.

@Rharper / @Smoser - will that be a problem?
If so any suggestions how to resolve while still getting the request filed here considered?

Not sure if vgauth.service can work with "DefaultDependencies=no" if that is the outcome to be needed.

Oliver - please share what you think as well for this discussion.

Also let me know when you have a Debian bug, I want to outline my concerns there as well.
If you don't mind, copy my thoughts here to the Debian bug when opening it.

I'm waiting for comments on my concerns for the dependencies.
If anybody wants to take a more practical approach there are test builds in [1].

[1]: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3530

After discussing with smoser it seems we agree that we expect some potential issues.
Eventually it comes down to almost "chaning-any-dependency-anywhere-in-systemd has issues" :-)

In this particular case we are concerned that vgauth.service might have issues as it has to start much earlier than usual.

That is different to the "Fedora has the same" argument, as they don't have open-vm-tools.service not that early for cloud-init. Due to that "pulling vgauth.service" before "open-vm-tools.service" is a bigger change for Ubuntu one should be aware of.

As I said in comment #5 I have combined the requested change in PPA [1] and it might come down to VMWare to verify that not only it fixes this bug here, but also has no unwanted side effects.

Call for action:
- please verify the proposed package in [1] as part of our agreement to verify new versions
- while doing so please focus additionally on the services changed here open-vm-tools.service and vgauth.service to be correct
- please also verify that it really fixes the issue you opened this bug for

In addition I'll send a mail about that.

[1]: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3530

Subscribed John, Oliver and Yuhua who usually work with us to coordinate and verify open-vm-tools

Thanks for addressing this issue.

The packages are built for Ubuntu/disco (19.04) - any chance these can be built with bionic?

Well, due to this being a bit unclear yet I didn't prep the PPA for all versions.
But if it helps in depth testing I can certainly do so.

I built the same open-vm-tools code for 18.04/18.10 in the same PPA that was referenced.
So any version >18.04 can be tested against the proposed changes including those Dependency changes.

Thanks Christian!

Can you tell what versions of the package are affected? I'd expect all older versions have this issue, but I am not sure since it hasn't been reported before.

2:10.2.0-3~ubuntu0.16.04.1 in Xenial might be affected as well, Trusty is just very different in terms of startup constraints. But lets focus if it is even working fine before going further.
I'm still afraid making vgauth so early might case rare but severe issues - really try to triple check that on the verification.

@Oliver,

I have 2 questions about adding 'After=vgauth.service' to open-vm-tools.service
and adding 'DefaultDependencies=no' to vgauth.service.

a.) if open-vm-tools.service requires vgauth.service, then shouldn't we also
add a 'Requires=vgauth.service' ?
b.) Your MP adds RequiresMountsFor=/tmp to vgauth.service. That would seem to
imply that vgauth.service will *use* /tmp. As we found in bug 1707222,
use of /tmp during boot is not safe. some systemd jobs might delete the
files while you're using them. So.. the question is then how is
vgauth.service using /tmp and/or how is it avoiding a race condition.

I forwarded those questions to the developer for vgauth.

For a) my take is that vmtoolsd runs fine without vgauth, only when special authentication is required is it needed. So I am not sure if Requires=vgauth.service' is a good idea.

Any update on deep-testing and verifying this one?

Alternative - complete the merge of 10.3.5 without this change for now to un-couple it - opinions?

Yeah - I went a bit back and forth on it, but let us split this discussion from the current merge for Ubuntu 19.04. There can always be a follow on upload once this discussion is fully complete and verified.

I pinged the team again to test these packages. Overall, the packages seem fine, as tested by
Yuhua Zou on our email thread, quoted below:

It works well when do sanity check for open-vm-tools 10.3.5 from ppa: ci-train-ppa-service/3530

1. Check in the following VMs

disco-server-cloudimg-amd64.ova daily build
ubuntu-18.10-Desktop-amd64
ubuntu-18.10-server-amd64
ubuntu-18.10-live-server-amd64
ubuntu-18.04-Desktop-amd64
ubuntu-18.04-server-amd64
ubuntu-18.04-live-server-amd64

2. Check items:

install / upgrade open-vm-tools from ppa: ci-train-ppa-service/3530
tools service and VGAuth service are running when install / upgrade open-vm-tools
tools service and VGAuth service are running after install / upgrade and reboot guestOS
uninstall open-vm-tools 10.3.5

3. Note:

        1) Not check with daily build disco-server-amd64.iso because I can’t install guestOS with this ISO successfully.

 It can’t pass the stage “Configure the package manager” during the installing process.

        2) I didn’t check the boot sequence between vmtoold and VGauth services. So the issue https://bugs.launchpad.net/ubuntu/+source/open-vm-tools/+bug/1804287

still need Oliver’s verification.

Okay, the packages have been tested by the SRM team. It works as expected now.

Thanks for fixing this!

Also, updates on Scott's questions:
a) vgauth isn't always required, but to avoid issues when it is it's better to have it running always. However, it is already enabled by default. What if a user wants to disable it because it's not needed? With a "Requires" it would be started anyway.

b) /tmp isn't needed for vgauth, so the line "RequiresMountsFor=/tmp" can be removed. I guess it has been added to vgauth.service when it was added to open-vm-tools.service. It's needed for the latter, but not the former.

To help Debian just as much and since we are currently in sync (Thanks Bzed for taking all my changes) I submitted that - after our discussion and verification here - to Debian at: https://github.com/bzed/pkg-open-vm-tools/pull/15

There was no Debian bug (that I'd know of) filed yet, but the description says "I will also file a bug with Debian". If there is any let me know to link the PR with it and this bug.

This bug was fixed in the package open-vm-tools - 2:10.3.5-3

---------------
open-vm-tools (2:10.3.5-3) unstable; urgency=medium

  [ Christian Ehrhardt ]
  * [d3d4703] Start vgauth before vmtoolsd.
    VGAuthService needs to be ready when vmtoolsd runs. Certain cases - e.g.
    Site Recovery Manager failover - will need vgauth to be up.
    Therefore add an After=vgauth.service dependency to open-vm-tools.service
    To have vgauth be able start early - and not pull cloud-init back late - it
    is also required to drop default dependencies which according to VMware is
    fine to do so.
    (LP: #1804287)

 -- Bernd Zeimetz <email address hidden> Fri, 14 Dec 2018 09:55:04 +0100

Thanks to Bernd my MP was accepted and is in latest Debian and synced to Ubuntu-Disco now.
While I'm sort of confident on the change, I'd say we'll keep the new 10.3.5 in Disco until after the Christmas break and then take a look at the backports to 18.04 and 18.10.

FYI: The backport of this will be part of bug 1813944

This is a packaging change in an SRU, and therefore requires a separate SRU template with test case, independent of the exception granted to the new upstream version of this package.

Also the vgauth service is now setting DefaultDependencies=no which is evidently related to this change but not described by this bug. DefaultDependencies=no requires a great deal of care and at first glance this change looks INCORRECT because there are no manual dependencies added in place of the default dependencies - it's almost certainly wrong to say that vgauth has NO dependencies (at minimum: local-fs.target?)

More due diligence is definitely required here to demonstrate the correctness of the changes.

An upload of open-vm-tools to cosmic-proposed has been rejected from the upload queue for the following reason: "needs a separate SRU bug for the debian/desktop.conf addition. Also, upload fails to comply with https://wiki.ubuntu.com/DebianMaintainerField".

An upload of open-vm-tools to bionic-proposed has been rejected from the upload queue for the following reason: "needs a separate SRU bug for the debian/desktop.conf addition. Also, upload fails to comply with https://wiki.ubuntu.com/DebianMaintainerField".

Hi Steve, thanks for the review - let me try to clarify:
IMHO it is mentioned in the changelog:
  To have vgauth be able start early - and not pull cloud-init back late - it
  is also required to drop default dependencies which according to VMware is
  fine to do so.
But I agree that it isn't properly SRU described in this bug here was it was a change that was pulled in by the "-sa -v<lastversion>" and from further down the changelog - that was my fault, sorry - prepping it better for a re-upload.

Disambiguation of services in comments discussion on this bug:
vgauth - open-vm-tools.vgauth.service
vmtoolsd - open-vm-tools.service
fs - systemd-remount-fs.service
tmp - systemd-tmpfiles-setup.service

Summarizing the dependencies (and wishes for them) once more:
- open-vm-tools.vgauth.service is supposed to be before before open-vm-tools.service (for the issue outlined here)
- open-vm-tools.service needs to be before cloud-init-local.service (to get datasources)

As discussed on the bug between comment #3 and #18 we expressed our concerns about DefaultDependencies=no but were somewhat convinced by testing that it might work, but still felt unsure.

After all even cloud-init-local has at least
  After=systemd-remount-fs.service

But then almost exactly as we were afraid of in general issues were found for open-vm-tools.service and this was added:
  After=systemd-remount-fs.service systemd-tmpfiles-setup.service
(one for issues we had with /tmp just as we assumed above in e.g. comment #12, and one for systemd-remount-fs just as cloud-init-local)
That was for Debian bug #914910 and is in https://github.com/bzed/pkg-open-vm-tools/commit/e4697c7

Now IMHO this same safety net dependencies should be added to open-vm-tools.vgauth.service as well to not find an issue there.

Eventually that should be no issue as - summarizing again with the above in mind:
- open-vm-tools.vgauth.service is supposed to be before before open-vm-tools.service (for the issue outlined here)
- open-vm-tools.service needs to be before cloud-init-local.service (to get datasources)
- cloud-init-local.service has DefaultDependencies=no but After=systemd-remount-fs.service
- open-vm-tools.service got this against races
  After=systemd-remount-fs.service systemd-tmpfiles-setup.service

That already makes it:
fs / tmp -> vmtoolsd -> cloud-init
Now throwing in the requirement to have vgauth to be before vmtoolsd means we can add fs / tmp dependencies there to be on the safe side without changing the current dependency chain.

It would then be
fs / tmp -> vgauth -> vmtoolsd -> cloud-init

I agree that this will make it feel more correct and as outlined will not thwart the intention of the "get vmtools before vmtoolsd" change in the consideration of cloud-init-local needing to be early but also after open-vm-tools.

Let me suggest that to Bernd to make it right in Buster as well ...

Actually we made it "PartOf" maybe it inherits all we need already ...

No it does not, so we need the change that I envisioned - prepping that as mentioned ...

Subscribing Bernd here as well to be aware of the re-started discussion on better dependencies.
I have filed a PR to the Debian packaging at [1] and will continue once the discussion on that concluded.

[1]: https://github.com/bzed/pkg-open-vm-tools/pull/18

Accepted in Debian, I'll make it part of Disco and afterwards also part of the SRU respin.

Uploaded with 10.3.5-7

Re-fixed with the sync of 10.3.5-7, rebasing the SRU onto that ...

After improving on what we have found triggered by the first SRU review this is ready again.
Changes:
- Despite being an MRE in general all associated individual bugs have full SRU templates now.
- maintainers correctly updated
- The change to the vgauth Dependencies is now safer

That said, this is uploaded to Bionic/Cosmic unapproved again and waiting for SRU Team re-review.

Hello Oliver, or anyone else affected,

Accepted open-vm-tools into cosmic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/open-vm-tools/2:10.3.5-7~ubuntu0.18.10.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-cosmic to verification-done-cosmic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-cosmic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Oliver, or anyone else affected,

Accepted open-vm-tools into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/open-vm-tools/2:10.3.5-7~ubuntu0.18.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Thanks Steve for accepting.

@Oliver - verifying the SRM case on Bionic and Cosmic is up to you since you have the setup to do so.

Thanks all for the update!

I asked our QE team to verify the SRM fix, I will let you know. I just did a sanity check for the quiesced snapshot fix, it looks good.

@Oliver are there any other fixes that we should add? There are some more branches in the github repository for 10.3.5 - is there anything that needs to go into Debian buster?

@Bernd, just the fixes in the stable-10.3.5-quiesced-snapshot branch. The other branch from 10.3.5 , stable-10.3.5-appmonitorlib was created on a request by another OS vendor, and is not needed in other distributions.

I got this from Oliver (per Mail) after asking for a verification:
"the tests for vgauth (related to SRU) have passed. It's good to go."

Marking verified

This bug was fixed in the package open-vm-tools - 2:10.3.5-7~ubuntu0.18.10.1

---------------
open-vm-tools (2:10.3.5-7~ubuntu0.18.10.1) cosmic; urgency=medium

  * Backport recent open-vm-tools (LP: #1813944)
    - also adresses handling of quiesced snapshot failures (LP: #1814832)
    - also adresses issues with resolutionKMS plugins sometimes fails to
      load at boot (LP: #1818473)

open-vm-tools (2:10.3.5-7) unstable; urgency=medium

  [ Christian Ehrhardt ]
  * [71b468f] make vgauth service execution more reliable.
    Since d3d47039 "Start vgauth before vmtoolsd" there is a potential race
    of starting vgauth so early that it might have issues. This was
    discussed back in the day in [1] to [2], but confirmed to be ok by
    VMWare.
    We were all somewhat convinced by this, but a bad feeling remained not
    only with me but also with Bernd [4].
    A recent SRU review denial made me rethink all of it and I think we can
    make it safer without thwarting the purpose of the original change.
    Note: Disambiguation of service names used below:
    vgauth - open-vm-tools.vgauth.service
    vmtoolsd - open-vm-tools.service
    fs - systemd-remount-fs.service
    tmp - systemd-tmpfiles-setup.service
    cloud-init - cloud-init-local.service
    Currently we have these dependency requirements:
    - vgauth should be before vmtoolsd
    - cloud init should be before vmtoolsd
    - cloud init has to be really early in general
      - therefore this is using DefaultDependencies=No
    That lead to this graph:
     fs / tmp -> vmtoolsd -> cloud-init
    And d3d47039 added it to be like:
     fs / tmp -> vmtoolsd -> cloud-init
                   ^
     vgauth --|
    But there is no need to have vgauth without any pre-dependencies at all.
    It is only needed to be "before" vmtoolsd, therefore we can make it:
     fs / tmp -> vgauth -> vmtoolsd -> cloud-init
    That will make execution of vgauth much less error-prone (even though I
    have no hard issue to report) while at the same time holding up all
    known required ordering constraints.
    [1]: https://bugs.launchpad.net/ubuntu/+source/open-vm-tools/+bug/1804287/comments/3
    [2]: https://bugs.launchpad.net/ubuntu/+source/open-vm-tools/+bug/1804287/comments/12
    [3]: https://bugs.launchpad.net/ubuntu/+source/open-vm-tools/+bug/1804287/comments/25
    [4]: https://github.com/bzed/pkg-open-vm-tools/pull/15#issuecomment-447237910
    Signed-off-by: Christian Ehrhardt <email address hidden>

open-vm-tools (2:10.3.5-6) unstable; urgency=medium

  * [43ec618] Correct and/or improve handling of certain quiesced
    snapshot failures.
    Thanks to Oliver Kurth (Closes: #921470)

open-vm-tools (2:10.3.5-5) unstable; urgency=medium

  * [54cce3e] Start vmtoolsd after apparmor.service.
    Github issue #17

open-vm-tools (2:10.3.5-4) unstable; urgency=medium

  [ Alf Gaida ]
  * [e13792d] udevadm trigger should not fail (Closes: #917642)

open-vm-tools (2:10.3.5-3) unstable; urgency=medium

  [ Christian Ehrhardt ]
  * [d3d4703] Start vgauth before vmtoolsd.
    VGAuthService needs to be ready when vmtoolsd runs. Certain cases - e.g.
    Site Reco...

The verification of the Stable Release Update for open-vm-tools has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package open-vm-tools - 2:10.3.5-7~ubuntu0.18.04.1

---------------
open-vm-tools (2:10.3.5-7~ubuntu0.18.04.1) bionic; urgency=medium

  * Backport recent open-vm-tools (LP: #1813944)
    - also adresses handling of quiesced snapshot failures (LP: #1814832)
    - also adresses issues with resolutionKMS plugins sometimes fails to
      load at boot (LP: #1818473)

open-vm-tools (2:10.3.5-7) unstable; urgency=medium

  [ Christian Ehrhardt ]
  * [71b468f] make vgauth service execution more reliable.
    Since d3d47039 "Start vgauth before vmtoolsd" there is a potential race
    of starting vgauth so early that it might have issues. This was
    discussed back in the day in [1] to [2], but confirmed to be ok by
    VMWare.
    We were all somewhat convinced by this, but a bad feeling remained not
    only with me but also with Bernd [4].
    A recent SRU review denial made me rethink all of it and I think we can
    make it safer without thwarting the purpose of the original change.
    Note: Disambiguation of service names used below:
    vgauth - open-vm-tools.vgauth.service
    vmtoolsd - open-vm-tools.service
    fs - systemd-remount-fs.service
    tmp - systemd-tmpfiles-setup.service
    cloud-init - cloud-init-local.service
    Currently we have these dependency requirements:
    - vgauth should be before vmtoolsd
    - cloud init should be before vmtoolsd
    - cloud init has to be really early in general
      - therefore this is using DefaultDependencies=No
    That lead to this graph:
     fs / tmp -> vmtoolsd -> cloud-init
    And d3d47039 added it to be like:
     fs / tmp -> vmtoolsd -> cloud-init
                   ^
     vgauth --|
    But there is no need to have vgauth without any pre-dependencies at all.
    It is only needed to be "before" vmtoolsd, therefore we can make it:
     fs / tmp -> vgauth -> vmtoolsd -> cloud-init
    That will make execution of vgauth much less error-prone (even though I
    have no hard issue to report) while at the same time holding up all
    known required ordering constraints.
    [1]: https://bugs.launchpad.net/ubuntu/+source/open-vm-tools/+bug/1804287/comments/3
    [2]: https://bugs.launchpad.net/ubuntu/+source/open-vm-tools/+bug/1804287/comments/12
    [3]: https://bugs.launchpad.net/ubuntu/+source/open-vm-tools/+bug/1804287/comments/25
    [4]: https://github.com/bzed/pkg-open-vm-tools/pull/15#issuecomment-447237910
    Signed-off-by: Christian Ehrhardt <email address hidden>

open-vm-tools (2:10.3.5-6) unstable; urgency=medium

  * [43ec618] Correct and/or improve handling of certain quiesced
    snapshot failures.
    Thanks to Oliver Kurth (Closes: #921470)

open-vm-tools (2:10.3.5-5) unstable; urgency=medium

  * [54cce3e] Start vmtoolsd after apparmor.service.
    Github issue #17

open-vm-tools (2:10.3.5-4) unstable; urgency=medium

  [ Alf Gaida ]
  * [e13792d] udevadm trigger should not fail (Closes: #917642)

open-vm-tools (2:10.3.5-3) unstable; urgency=medium

  [ Christian Ehrhardt ]
  * [d3d4703] Start vgauth before vmtoolsd.
    VGAuthService needs to be ready when vmtoolsd runs. Certain cases - e.g.
    Site Reco...