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

Update Travis configuration to test all node types. #97

Merged
merged 15 commits into from Oct 28, 2015

Conversation

@aneeshusa
Copy link
Member

aneeshusa commented Aug 21, 2015

For each node type in top.sls, check that highstate is successfully reached (no errors).
This includes Linux and Mac OS X machines.

Review on Reviewable

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 21, 2015

You're amazing, thank you.

@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 21, 2015

Sorry it took a while, spent yesterday installing Linux on my mom's laptop. Credits go to the Travis docs - I've never set it up before, but I was surprised that I got it on the first try.

Current status:

  • 'administrator' doesn't seem to work for the ssh keys on the Mac builds - I don't have a Mac, so no idea. To be honest, I don't really like the idea of ssh access to root/administrator; I always ssh as a regular user and sudo from there when needed.
  • No idea on what's causing Salt to silently fail with a 137 error on the Android builds. I just ran it successfully on my local machine, so something's probably wonky with Travis. I'll look into it a bit more, but if you can try it on an actual Ubuntu box that would be helpful.

Once those are fixed, this should be good to merge. Important improvement:

  • Add fake pillar data and add back the --retcode-passthrough to the final state.highstate. Don't use dry-run mode (test=True) - Salt doesn't take into account what each state would do so states with requisites often spuriously fail with test=True.
@metajack
Copy link
Contributor

metajack commented Aug 27, 2015

Has your key been propagated to the build slaves? I didn't see a PR go by for that. Maybe I'm misunderstanding the issue with the key.

Are you building the android servo with travis? Because Servo consumes too much memory during build to be built on Travis reliably. That might explain why you aren't having trouble locally.

@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 27, 2015

Sorry, I should have been clear that I was talking about the build failures on Travis.

  • All the ssh_auth.present states are failing on Mac, and I'm not really sure why. Travis reports the build succeeded because --retcode-passthrough is not enabled on the final state.highstate.
  • Yes, the PR is meant to test all the setups (including Android), so the failure makes sense, as it fails while downloading the Android NDK, which is a pretty large file.

Let me rebase and see if anything changes.

@aneeshusa aneeshusa force-pushed the aneeshusa:test-all-node-types branch 2 times, most recently from 5afe2a2 to 431afe6 Aug 27, 2015
@metajack
Copy link
Contributor

metajack commented Aug 27, 2015

Oh I see, it's not building Servo, but running into some other long running thing. I'm not sure what to do about that :(

@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 27, 2015

No changes from rebasing, but I made some progress on the Android front. Current status:

  • Salt is currently lying to Travis about return codes; you have to manually check the logs to see build status. (Just scroll to the end and Salt has a summary.)
  • ssh_auth.present is still broken on Macs; I don't know why.
  • buildbot and homu are broken; we'd need to supply fake pillar data for them to consume.
  • android-dependencies.sls went through just fine on servo-head, but failed on servo-linux-android1. The only difference I know of is that servo-linux-android1 should also be running the gonk states, but it's failing before it ever gets there. I'll dig a little more but we may have to ask Travis-CI about this one.

In my opinion, you could merge this now. The main goal of the PR is to get Travis to run states across all the different kinds of machines, which it's doing. Fixing the build failures can be done in other PRs. However, I'm not passing the retcode through Salt, so Travis won't know if the build failed or succeeded. You'll need to look at all the build logs individually to see if there are any problems. Once all the build failures get cleaned up, we can start passing the retcode again, and Travis will be able to report success/failure without a manual check through the logs.

However, if you want to clean up the build failures first, that works too. Let me know which way you want to go.

@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 27, 2015

Whoops, I should clarify - no building of Servo is being done. These states just set up all the dependencies and everything to get ready to build Servo: installing packages, grabbing the Android NDK and installing it, setting up buildbot/homu, adding ssh keys, etc. The current failure on servo-linux-android1 occurs when downloading the Android NDK, it seems. However, it seems to download and install just fine on servo-head.

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 28, 2015

I don't think any of us will notice regressions if they don't make travis fail, so for these tests to be useful, I think they need to tell travis about any failures.

@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 28, 2015

That's fair enough. I've turned the "don't lie to Travis" option on (--retcode-passthrough). Unfortunately, everything's currently broken. :(

@bors-servo
Copy link
Contributor

bors-servo commented Sep 22, 2015

The latest upstream changes (presumably #96) made this pull request unmergeable. Please resolve the merge conflicts.

@aneeshusa aneeshusa force-pushed the aneeshusa:test-all-node-types branch 2 times, most recently from e9e5605 to 4a23adb Sep 22, 2015
@aneeshusa
Copy link
Member Author

aneeshusa commented Sep 23, 2015

OK, I've fixed all the build failures aside from the Android-related ones. Android on Travis CI is known to be flaky, so I don't have any real answers. (For example, in the latest build it went through on servo-linux-android1 but not servo-head.) I think this is related to resource limits on Travis, so the one idea I have is turning off parallel builds and doing builds sequentially, so each build hopefully gets full use of the resources. I'm not sure if this would work consistently, and it would definitely slow everything down. Otherwise, another option is to test on everything but the android-related nodes. Which would be preferred? All the other tests are working now, so I think there would be value in merging at least those.

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 23, 2015

Can we skip or ignore the android parts somehow?

@aneeshusa
Copy link
Member Author

aneeshusa commented Sep 23, 2015

Updated to not run builds that involve Android. Specifically skipping Android states is harder and messier, but probably possible - I'll have to think about how to do it.

@aneeshusa aneeshusa force-pushed the aneeshusa:test-all-node-types branch from 051abad to dc2ecd9 Sep 23, 2015
@aneeshusa
Copy link
Member Author

aneeshusa commented Sep 23, 2015

OK, now it skips only Android states on Travis. I think this is ready to get reviewed.

@Manishearth
Copy link
Member

Manishearth commented Oct 27, 2015

@metajack
Copy link
Contributor

metajack commented Oct 27, 2015

@bors-servo r+


Reviewed 3 of 3 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 7 of 7 files at r8, 1 of 1 files at r9, 2 of 2 files at r10, 1 of 1 files at r11, 4 of 4 files at r12.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

📌 Commit dc2ecd9 has been approved by metajack

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

Testing commit dc2ecd9 with merge f72c7ca...

bors-servo added a commit that referenced this pull request Oct 27, 2015
Update Travis configuration to test all node types.

For each node type in top.sls, check that highstate is successfully reached (no errors).
This includes Linux and Mac OS X machines.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/97)
<!-- Reviewable:end -->
@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 27, 2015

Thanks for reviewing this! I'll look into the build failures from bors.

@aneeshusa aneeshusa force-pushed the aneeshusa:test-all-node-types branch 2 times, most recently from 2061d1f to 5575026 Oct 27, 2015
For each node type in top.sls, check that
highstate is successfully reached (no errors).
This includes Linux and Mac OS X machines.
aneeshusa added 2 commits Sep 23, 2015
Instead of skipping all node types that include Android, use the pillar
to skip Android states on Travis.

Providing 'travis: True' in the pillar for a node will enable skipping
Android states; the key can simply be omitted if not on Travis, and Salt
will fall back to running Android states by default.
2015.8.0 causes breakage on OS x and 2015.8.1, which fixes it, is not
yet available in homebrew.

Use 2015.5.6 for Ubuntu 12.04 and 2015.5.5 for OS X.
@aneeshusa aneeshusa force-pushed the aneeshusa:test-all-node-types branch from 5575026 to 7ef9de3 Oct 27, 2015
When installing autoconf213 from the homebrew/versions tap, salt
correct installs the package but fails to verify its installation
because it compares 'homebrew/versions/autoconf213', which is the
specified package name, with 'autoconf213', which is the installed
package name. Use the brew.install module function as a workaround
until this functionality is added to pkg.installed or pkgrepo.

See: saltstack/salt#26414
@aneeshusa aneeshusa force-pushed the aneeshusa:test-all-node-types branch from 7ef9de3 to f61b4d8 Oct 27, 2015
PR #132 added service_identity to a pip.installed state which was using
name instead of pkgs because it only had one package to install
(buildbot). Use pkgs instead everywhere and create a style guide.
@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 28, 2015

Alright, I've addressed the build failures, this is ready to be looked at again. @metajack

@metajack
Copy link
Contributor

metajack commented Oct 28, 2015

Looks good. I had one question that perhaps just needs some explanation.


Reviewed 1 of 1 files at r13, 1 of 1 files at r14, 3 of 3 files at r15.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


servo-dependencies.sls, line 22 [r14] (raw file):
Is this required by newer versions of homebrew? The old rule has been working so far.


Comments from the review on Reviewable.io

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 28, 2015

I left a detailed explanation in the commit message for f61b4d8, which I'll recap here. In the original code with 'homebrew/versions/autoconf213', salt passes this verbatim to homebrew and homebrew will correctly install that version of autoconf213. After running homebrew, salt queries the installed packages to confirm if the installation succeeded, and homebrew returns that 'autoconf213' is installed. Because 'homebrew/versions/autoconf213' is not the same as 'autoconf213', salt thinks the package did not install correctly. Salt's brew execution module does handle taps correctly, so the workaround is to call that module instead of installing autoconf213 through the pkg.installed state.

In summary, both styles work fine with homebrew, this change is to appease salt.

@metajack
Copy link
Contributor

metajack commented Oct 28, 2015

Can you squash the fixups easily? If not we can take this as is.

Once you squash, this is r=me.

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 28, 2015

Do you want me to squash all of the commits together or just the last three? I can squash them all together easily if you want, but I'm not sure what commit message I could use if I squashed the last three.

@metajack
Copy link
Contributor

metajack commented Oct 28, 2015

@bors-servo r+

Nevermind. I just relooked and these commits are already nice and clean. I was reading too much into "Fix" in the titles and reviewable doesn't really make it easy to look over commit messages.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 28, 2015

📌 Commit 032a7cb has been approved by metajack

@bors-servo
Copy link
Contributor

bors-servo commented Oct 28, 2015

Testing commit 032a7cb with merge f71f9aa...

bors-servo added a commit that referenced this pull request Oct 28, 2015
Update Travis configuration to test all node types.

For each node type in top.sls, check that highstate is successfully reached (no errors).
This includes Linux and Mac OS X machines.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/97)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 28, 2015

💔 Test failed - travis

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 28, 2015

Looks like the build failure is due to a commit being removed from @Manishearth's homu repository on Github. Details here: https://travis-ci.org/servo/saltfs/jobs/87915718

@Manishearth
Copy link
Member

Manishearth commented Oct 28, 2015

Feel free to revert my latest commit. It's not deployed right now anyway.

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 28, 2015

I'll go ahead and do that in a separate PR since it's not related to this. I'm a little curious as to why you seem to have removed that commit though?

bors-servo added a commit that referenced this pull request Oct 28, 2015
Revert "Add try privileges"

This reverts commit e65d05e.

Related: see #97.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/139)
<!-- Reviewable:end -->
@Manishearth
Copy link
Member

Manishearth commented Oct 28, 2015

Oh, I force pushed new commits onto that, which means the old commits will disappear on a vanilla git fetch.

@Manishearth
Copy link
Member

Manishearth commented Oct 28, 2015

I had updated homu on the server too, and was going to send a salt patch, but Ms2ger reverted it since he needed our broken CI to be working again to land things 😄

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 28, 2015

Oh haha, I agree with Ms2ger that working CI is good :) Can you tell bors to try again now that the revert has landed?

@Manishearth
Copy link
Member

Manishearth commented Oct 28, 2015

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Oct 28, 2015

Testing commit 032a7cb with merge 4bb437a...

bors-servo added a commit that referenced this pull request Oct 28, 2015
Update Travis configuration to test all node types.

For each node type in top.sls, check that highstate is successfully reached (no errors).
This includes Linux and Mac OS X machines.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/97)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 28, 2015

☀️ Test successful - travis

@bors-servo bors-servo merged commit 032a7cb into servo:master Oct 28, 2015
1 of 2 checks passed
1 of 2 checks passed
homu Testing commit 032a7cb with merge 4bb437a...
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.