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

Self-Test: Run nginx #699

Merged
merged 1 commit into from
Mar 15, 2019
Merged

Self-Test: Run nginx #699

merged 1 commit into from
Mar 15, 2019

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 11, 2019

Adds a self-test that runs nginx and doesn't basic sanity checks on the
files it serves. This is nice to make sure that --open works but
also it should serve as another line of defence against making invalid
nginx config files. This is important as those nginx config files will
be used on the web site and we'd really prefer not to bring that down.

To get this to work consistently I have to drop the "don't run twice"
behavior of build_docs.pl. That behavior is implemented by checking
for a pidfile on startup. If the file is there it checks if ps for
that pid comes back with a process that looks like the running one. When
the perl process terminates naturally it destroys the file. This is
known to cause trouble when a server loses power and can't delete the
file and the process is started on startup. Startup is fairly
predictable and it is common that the process would end up with the same
pid so the ps check will think that the new process collides with
itself.

Pulling a server's power from the wall is fairly similar to what
happens when the docker process that runs the --self-test comes to the
end and destroys that docker image. This hasn't come up before because
build_docs.pl always terminates fairly quickly. It doesn't when you do
--open. I tried to get something that consistently removed the pidfile
but such a thing is complex in make, I think. So I just removed it.

"Don't run twice" wasn't really buying us much anyway. At this point we
run build_docs on immutable infrastructure in CI and Jenkins manages
whether or not to run the build concurrently. We have much more precise
control there which is what we want anyway. It might have some value
during --self-test to make sure that we don't leave build_docs.pl
running between tests. At this point we shoot it if the test is
successful and let it die if the test fails. Which isn't great but it
should work.

Adds a self-test that runs nginx and doesn't basic sanity checks on the
files it serves. This is nice to make sure that `--open` works but
*also* it should serve as another line of defence against making invalid
nginx config files. This is important as those nginx config files will
be used on the web site and we'd really prefer not to bring that down.

To get this to work consistently I have to drop the "don't run twice"
behavior of `build_docs.pl`. That behavior is implemented by checking
for a pidfile on startup. If the file is there it checks if `ps` for
that pid comes back with a process that looks like the running one. When
the perl process terminates naturally it destroys the file. This is
known to cause trouble when a server loses power and can't delete the
file and the process is started on startup. Startup is fairly
predictable and it is common that the process would end up with the same
pid so the `ps` check will *think* that the new process collides with
itself.

Pulling a server's power from the wall is *fairly* similar to what
happens when the docker process that runs the `--self-test` comes to the
end and destroys that docker image. This hasn't come up before because
`build_docs.pl` always terminates fairly quickly. It doesn't when you do
`--open`. I tried to get something that consistently removed the pidfile
but such a thing is complex in make, I think. So I just removed it.

"Don't run twice" wasn't really buying us much anyway. At this point we
run `build_docs` on immutable infrastructure in CI and Jenkins manages
whether or not to run the build concurrently. We have much more precise
control there which is what we want anyway. It might have some value
during `--self-test` to make sure that we don't leave `build_docs.pl`
running between tests. At this point we shoot it if the test is
successful and let it die if the test fails. Which isn't *great* but it
should work.
@nik9000 nik9000 requested a review from a user March 11, 2019 21:12
@@ -24,9 +24,6 @@ BEGIN
}

use lib 'lib';
use Proc::PID::File;
die "$0 already running\n"
if Proc::PID::File->running( dir => '.run' );

Copy link

@ghost ghost Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PID files are lock files, and lock files are trouble. Good riddance.

I usually try to do some tricky integration interrogation of the process list to avoid PID files, and thus stale locks. The output of ps -eo comm and ps -eo command are favorites of mine.

@nik9000 nik9000 merged commit 8139b96 into elastic:master Mar 15, 2019
@nik9000
Copy link
Member Author

nik9000 commented Mar 15, 2019

Thanks for reviewing @Jarpy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant