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

End to end tests: hab-svc-load and hup-does-not-abandon-services #6849

Merged
merged 7 commits into from Aug 16, 2019

Conversation

@baumanj
Copy link
Contributor

commented Aug 14, 2019

Add/fix these two tests for our end-to-end CI. This will merge back into #6780.

See also #6816

@chef-expeditor

This comment has been minimized.

Copy link

commented Aug 14, 2019

Hello baumanj! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@baumanj baumanj force-pushed the end-to-end/hup-does-not-abandon-services branch 2 times, most recently from 99229bf to 1911efa Aug 14, 2019
@baumanj baumanj changed the base branch from master to cm/end-to-end Aug 14, 2019
@baumanj baumanj force-pushed the end-to-end/hup-does-not-abandon-services branch from 9b38e38 to eab5307 Aug 14, 2019
@baumanj baumanj self-assigned this Aug 14, 2019
@baumanj baumanj changed the title End to end/hup does not abandon services End to end tests: hab-svc-load and hup-does-not-abandon-services Aug 14, 2019
@baumanj baumanj marked this pull request as ready for review Aug 14, 2019
@christophermaier christophermaier referenced this pull request Aug 14, 2019
8 of 8 tasks complete
Copy link
Contributor

left a comment

Looks good, but it would be good to address the race condition.

- label: "[:linux: hup-does-not-abandon-services]"
command:
- .expeditor/scripts/setup_environment.sh DEV
- hab pkg install --binlink core/expect/5.45.4/20190115014137

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 15, 2019

Contributor

The reason the fully-qualified ident is needed here is because the environment contains HAB_BLDR_CHANNEL=DEV (see line 13 above), and there isn't a core/expect in that channel.

Since the exact release of expect isn't really important here, we could simplify this to

hab pkg install --binlink --channel=stable core/expect/5.45.4

or, simpler still, to

hab pkg install --binlink --channel=stable core/expect

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 15, 2019

Author Contributor

What's the advantage? I don't think expect is likely to change in a way that's relevant to this test. Pinning it has the advantage to eliminating a potential difference when running this in different environments.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 15, 2019

Contributor

It's just that when I see a pin, I immediately wonder what the significance of that specific release is.

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 15, 2019

Author Contributor

Fair enough. I'll try changing it.

@@ -99,7 +99,7 @@ expect {
error "Redis not ready to accept connections!"
}
}
set redis_pid [exec pgrep redis-server]
set redis_pid [exec cat /hab/svc/redis/PID]

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 15, 2019

Contributor

When I run this test locally, it will sometimes fail at this part; it looks like there might be a race between when the Supervisor writes this PID file and when the test tries to read it.

If I add a sleep 1 immediately before this check, for instance, I see no failures.

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 15, 2019

Author Contributor

0d5487c should address this

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 15, 2019

Contributor

That works.

Out of curiosity, what prompted you to move away from the pgrep approach to the PID file? Not having to worry about whether pgrep would be there?

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 15, 2019

Author Contributor

As I mentioned in c53bfa1:

there may be other redis-servers running locally.

To wit:

jbauman@ubuntu:~/habitat$ pgrep -afl redis-server
77398 /opt/opscode/embedded/bin/redis-server 127.0.0.1:16379                         
sleep 1

# Load up Redis on that Supervisor
spawn hab svc load core/redis

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 15, 2019

Contributor

Initially, I was surprised to see that this package installation was coming from stable and not DEV, but then I discovered that the HAB_BLDR_CHANNEL environment variable is ignored in hab svc load.

@christophermaier

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Incidentally, I think you can rebase your work directly on master and merge from there. I don't think any of your changes are dependent on the ones from my PR.

@baumanj baumanj force-pushed the end-to-end/hup-does-not-abandon-services branch from eab5307 to 0d5487c Aug 15, 2019
@baumanj baumanj requested review from chefsalim, eeyun and raskchanky as code owners Aug 15, 2019
@baumanj

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Incidentally, I think you can rebase your work directly on master and merge from there. I don't think any of your changes are dependent on the ones from my PR.

Any reason not to merge back into your PR? All the e2e work is captured in the same issue, and there are some things in my changes that may be useful to the other e2e tests. Also, it'll just be less work for me to merge it as is since my branch was based on yours from the beginning.

@christophermaier

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Incidentally, I think you can rebase your work directly on master and merge from there. I don't think any of your changes are dependent on the ones from my PR.

Any reason not to merge back into your PR? All the e2e work is captured in the same issue, and there are some things in my changes that may be useful to the other e2e tests. Also, it'll just be less work for me to merge it as is since my branch was based on yours from the beginning.

If I were to rework and force-push my branch then that could mess yours up. They're not inherently coupled. If your PR has something I (or anyone else) can use, then I'll rebase mine when yours merges.

@baumanj baumanj force-pushed the end-to-end/hup-does-not-abandon-services branch from 0d5487c to 91bc0d2 Aug 15, 2019
@baumanj baumanj changed the base branch from cm/end-to-end to master Aug 15, 2019
baumanj added 4 commits Aug 14, 2019
Some minor tweaks were necessary to the expect file. Notably, replace
`pgrep redis-server` with accessing /hab/svc/redis/PID, since there may
be other `redis-server`s running locally.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
We discovered #6852 as part
of hup-does-not-abandon-services.exp, but that test is more complex.
It's preferable to have a simpler, more targeted test as well which
makes the nature of the failure (and potential regression) clearer.

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
@baumanj baumanj force-pushed the end-to-end/hup-does-not-abandon-services branch from 91bc0d2 to aaac068 Aug 15, 2019
Signed-off-by: Christopher Maier <cmaier@chef.io>
baumanj added 2 commits Aug 15, 2019
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
@baumanj baumanj force-pushed the end-to-end/hup-does-not-abandon-services branch from 51919f1 to b399808 Aug 16, 2019
@baumanj baumanj requested a review from christophermaier Aug 16, 2019
@baumanj

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

I think I've addressed all the feedback now.

@baumanj baumanj merged commit 003b835 into master Aug 16, 2019
5 of 6 checks passed
5 of 6 checks passed
buildkite/habitat-sh-habitat-master-end-to-end Build #64 failed (3 minutes, 6 seconds)
Details
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3142 passed (32 minutes, 3 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #258 passed (34 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@baumanj baumanj deleted the end-to-end/hup-does-not-abandon-services branch Aug 16, 2019
@scotthain

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

This is drive-by post-merge commentary that may have been resolved somewhere else, but it looks like ACCEPTANCE_HAB_BLDR_URL was changed to HAB_BLDR_URL and not all instances were change later in the pipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.