Jake Watkins [:dividehex] Assignee
	Description • 10 years ago

Now that puppet 3.7.0 has landed OSX 10.10 can be happy.  Here are a few tweaks to get 10.10 working with toplevel::base

	Jake Watkins [:dividehex] Assignee
	Comment 1 • 10 years ago

As dustin pointed out, we could just merge 10.9 and 10.10

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 2 • 10 years ago

Jake, when do you think we can get this landed? Is any other testing necessary before?

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 3 • 10 years ago

I can land this now - risk to non-10.10 hosts is minimal.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 4 • 10 years ago

http://hg.mozilla.org/build/puppet/rev/984e6dd51412

Thanks Dustin. Is there anything else we have to do here? Just asking before I start merging the repo to QA.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 5 • 10 years ago

Not that I know of, no.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 6 • 10 years ago

I still see problems with screenresolution:

Error: Could not retrieve catalog from remote server: Error 400 on SERVER: no build of screenresolution known to work on this OS X version at /etc/puppet/production/modules/packages/manifests/mozilla/screenresolution.pp:31 on node mm-osx-107.qa.scl3.mozilla.com
Warning: Not using cache on failed catalog
Error: Could not retrieve catalog; skipping run

So we need another fix here:
http://hg.mozilla.org/build/puppet/file/8c28d2cece12/modules/packages/manifests/mozilla/screenresolution.pp#l24

I hope no other release of this tool. :)

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 7 • 10 years ago

Agreed - if you put a patch up dividehex should be able to r? it quickly.

	:kanban-engops
	Comment 8 • 10 years ago

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/80405684

	:kanban-engops
	Comment 9 • 10 years ago

Dustin J. Mitchell deleted the linked story in Pivotal Tracker

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 10 • 10 years ago

I filed https://tickets.puppetlabs.com/browse/FACT-724 for the warnings from facter (which appear to be harmless)

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 11 • 10 years ago

 * [ ] finish collectd (bug 1066744)
 * [ ] support user creation on 10.10 (and fail where it's not supported)
 * [ ] add empty support for talos on 10.10
 * [ ] disable the iCloud welcome screen (c.f. bug 1059438)

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 15 • 10 years ago

Comment on attachment 8508263 [details] [diff] [review]
bug1078882.patch

Review of attachment 8508263 [details] [diff] [review]:
-----------------------------------------------------------------

Please keep in mind that I cannot test those changes as done via an installation. So my review is based on those logical changes as I can see here. Please make sure to test those changes.

::: modules/disableservices/manifests/user.pp
@@ +7,4 @@
>      case $::operatingsystem {
>          Darwin : {
>              osxutils::defaults {
> +                '$username-disablescreensaver':

Hasn't this to be ${username}?

@@ +38,4 @@
>              # and to make double-sure, turn off quarantining:
>              # http://superuser.com/questions/38658/how-to-suppress-repetition-of-warnings-that-an-application-was-downloaded-from-t
>              osxutils::defaults {
> +                "$username-disable-quarantine":

Same here as above and for all the cases below.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 16 • 10 years ago

Comment on attachment 8508264 [details] [diff] [review]
bug1078882-p2.patch

Review of attachment 8508264 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/users/manifests/builder/account.pp
@@ +106,5 @@
>                      }
>                      $user_req = User[$username]
>                  }
> +                default: {
> +                    fail("cannot create user on OS X ${macosx_productversion_major}")

nit: I think we always started sentences with a capital letter.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 17 • 10 years ago

Comment on attachment 8508265 [details] [diff] [review]
bug1078882-p3.patch

Review of attachment 8508265 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/talos/manifests/init.pp
@@ +66,1 @@
>                      # nothing yet!

nit: Why not combining both in the same block until we really have to add some custom code?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 21 • 10 years ago

Comment on attachment 8508693 [details] [diff] [review]
bug1078882-p2-r2.patch

Review of attachment 8508693 [details] [diff] [review]:
-----------------------------------------------------------------

r=me code-wise. If it has been tested and it works, great!

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 22 • 10 years ago

Comment on attachment 8508691 [details] [diff] [review]
bug1078882-r2.patch

https://hg.mozilla.org/build/puppet/rev/8998840d4343

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 23 • 10 years ago

Comment on attachment 8508692 [details] [diff] [review]
bug1078882-p1-r2.patch

https://hg.mozilla.org/build/puppet/rev/c8fad62af8e0

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 24 • 10 years ago

Comment on attachment 8508693 [details] [diff] [review]
bug1078882-p2-r2.patch

https://hg.mozilla.org/build/puppet/rev/c450e43bf2b7

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 25 • 10 years ago

With this in place, Yosemite works as far as I know.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 26 • 10 years ago

I did a puppet agent run on our 10.10 box and now everything works as expected.

Thanks for the fixes Dustin!

	Chris Cooper [:coop] (he/him)
	Comment 27 • 10 years ago

There are multiple things still broken here. The screenshot reveals a few of them:

* xcode command line tools are not installed
* talos can't access the local files it needs
* updates and notifications are still enabled

Also, to even get to the point where I could run tests, I needed to create a symlink to hg in /usr/bin because the default PATH on Yosemite doesn't include /usr/local/bin where key tools like hg, wget, and screenresolution live. I tried mucking with /etc/paths, /etc/bashrc, and /etc/profile, but buildbot didn't pick up any of those changes.

	Chris Cooper [:coop] (he/him)
	Comment 28 • 10 years ago

Oops, here's the screenshot that also shows the update bubbles.

	Chris Cooper [:coop] (he/him)
	Comment 29 • 10 years ago

This might prove useful too for disabling the Yosemite bits that insist on phoning home:

https://fix-macosx.com/

	Chris Cooper [:coop] (he/him)
	Comment 30 • 10 years ago

I manually installed the xcode command line tools on t-yomsemite-r5-0002 and pressed on to see what else I could uncover. 

I found that adding a "Require all granted" directive to /etc/apache2/other/talos.conf allowed the talos tests to run.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 31 • 10 years ago

It seems like only some OS X versions require XCode for Talos - in particular, only 10.8 until now:

  https://github.com/mozilla/build-puppet/blob/master/modules/talos/manifests/init.pp#L51

so that's the place to make that change.

"Require all granted" is an Apache-2.4 thing, and won't work on Apache-2.2.  So that will need to be conditionalized, probably on the OS X version.

Buildbot explicitly sets the path in the startup plist, which is why none of the other methods of setting PATH helped.  Actually, it's now set by runner - https://github.com/mozilla/build-puppet/blob/master/modules/runner/templates/runner.plist.erb#L16 - and includes /usr/local/bin.  So that's still a mystery.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 32 • 10 years ago

Coop, I need some more info before I can write a puppet patch for all of this.

 * why is Xcode required for talos on 10.10?
 * what PATH are you seeing in Buildbot jobs run by runner?  Is it not the path supplied in runner's plist?
 * I don't see anything about updates at https://fix-macosx.com/.  If you can figure out which launchd services are involved, I'm sure we can just disable them.  I bet it's the new (in 10.9) notifications system, and we just never noticed it with 10.9.

	Chris Cooper [:coop] (he/him)
	Comment 33 • 10 years ago

Kim is going to takeover the 10.10 testing work so we'll have someone driving it from the releng side who isn't preoccupied with buildduty/mgmt.

(In reply to Dustin J. Mitchell [:dustin] from comment #32)
> Coop, I need some more info before I can write a puppet patch for all of
> this.
> 
>  * why is Xcode required for talos on 10.10?

Talos tried to build psutils using gcc as a first step.

>  * what PATH are you seeing in Buildbot jobs run by runner?  Is it not the
> path supplied in runner's plist?

I haven't had a chance to check since Dustin made comment #31. I couldn't find a direct reference to the limited PATH in puppet, but hadn't looked in the runner configs. The login shell contains /usr/local/bin and its in /etc/paths, so I guess runner isn't using either of those sources.

>  * I don't see anything about updates at https://fix-macosx.com/.  If you
> can figure out which launchd services are involved, I'm sure we can just
> disable them.  I bet it's the new (in 10.9) notifications system, and we
> just never noticed it with 10.9.

The phoning-home bits are new in Yosemite, so it's unlikely we've dealt with them before. Just wanted to call them out as something to look at.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 34 • 10 years ago

OK, installing Xcode is already done and easy enough via puppet.  Everything else requires more investigation, so I don't think it's time for a patch yet.

As long as our tests aren't typing PII into the Safari search bar, I think we're OK as far as phoning home :)

	Kim Moir [:kmoir] ET
	Comment 35 • 10 years ago

dustin regarding comment #32

Yesterday I imaged a 10.10 machine.  This doesn't have coop's links in /usr/bin to /usr/local/bin utilities. It looks like PATH just indicates PATH=/usr/bin:/bin:/usr/sbin:/sbin when running tests on it.

See
http://dev-master1.srv.releng.scl3.mozilla.com:8036/builders/Rev5%20MacOSX%20Yosemite%2010.10%20cedar%20talos%20tp5o/builds/1678/steps/clone_scripts/logs/stdio

despite the fact that /usr/local/bin is in the plist for runner

which compared to a machine running 10.8 which has /usr/local/bin in the path
http://buildbot-master108.srv.releng.scl3.mozilla.com:8201/builders/Rev5%20MacOSX%20Mountain%20Lion%2010.8%20mozilla-central%20talos%20chromez/builds/665/steps/clone_scripts/logs/stdio

I've been doing some reading and it looks like Apple is sanitizing paths in 10.10 which is causing problems.  New behaviour vs 10.9 and 10.8.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 36 • 10 years ago

Where is the sanitizing occurring?

	Kim Moir [:kmoir] ET
	Comment 37 • 9 years ago

I think this bug can probably be closed since now we are working on greening up 10.10 tests in bug 1121199 but I'll let Jake decide on the status.  If there additional puppet fixes, I think they are minor.

	Jake Watkins [:dividehex] Assignee
	Comment 38 • 9 years ago

I'm going to close this.  If we come across any problems on OS 10.10 that can be solved with puppet, we can open up new bugs for them.  The initial scope of work here is complete.