-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add systemd integ tests to run with docker #17308
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Rajat Gupta <[email protected]>
Signed-off-by: Rajat Gupta <[email protected]>
Signed-off-by: Rajat Gupta <[email protected]>
❌ Gradle check result for e9c7531: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
qa/systemd-test/docker-compose.yml
Outdated
container_name: opensearch-systemd-test-container | ||
build: | ||
dockerfile_inline: | | ||
FROM amazonlinux:2023 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use cloud vendor neutral image here? We have a large number of distributions here under qa\os
folder (many are outdated sadly, may be a good opportunity to fix that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, does this image quay.io/centos/centos:stream9
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the Docker images we use are stuck with centos:8
: https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/DockerBase.java#L39
Signed-off-by: Rajat Gupta <[email protected]>
❌ Gradle check result for 56c519d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
# install systemd | ||
RUN dnf -y install systemd && dnf clean all | ||
# in practice, you'd COPY in the RPM you want to test right here | ||
RUN dnf -y install https://artifacts.opensearch.org/releases/bundle/opensearch/2.18.0/opensearch-2.18.0-linux-x64.rpm && dnf clean all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem with this approach is that we are always testing a fixed & stale version of opensearch and its systemd unit. Unfortunately, I don't have a solution for it. @reta you could something better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We build RPMs as part of the distributions [1], shouldn't we use those instead?
[1] https://github.com/opensearch-project/OpenSearch/tree/main/distribution/packages/rpm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. that solution looks perfect.
question: the RPM is built once for every major/minor version release cycle? or it gets updated as we merge code in main? or a nightly build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: the RPM is built once for every major/minor version release cycle? or it gets updated as we merge code in main? or a nightly build?
It is part of the build: ./gradlew assemble
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterzhuamazon Could you please suggest a way we can resolve this (i.e. taking the latest distribution and not hardcoding it every time a new version is released?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RajatGupta02 so we build the RPM as part the Gradle build itself, there should be no need to look for the distribution RPMs anywhere outside the local build itself. These tests could be run as part of distribution builds. Take a look on [1] as an inspiration, adds some tests for archive distribution. We could have a test module for packages as well that will use Docker Compose against RPMs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I had a discussion regarding the same with @peterzhuamazon, since the rpm distribution being locally built with ./gradlew assemble
gave errors while installing, to which he explained that there are certain scripts that are added on top of it to make it runnable, and that is done in the opensearch-build repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this scripts? If yes, we have to move these test(s) to opensearch-build and make it part of their workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RajatGupta02 yeah, moving it to opensearch-build might make more sense, because in the other PR @peterzhuamazon suggests that the systemd configs are eventually moved into opensearch-build, so ideally we should test against that copy of systemd unit.
but let's evaluate how much is the effort for the move, I know you have limited time and you have already spent a significant time on getting this up and ready in core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to sync up with @peterzhuamazon to understand how should we integrate new tests within the opensearch-build repo
} | ||
|
||
@Test | ||
public void testProcessExit() throws IOException, InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we name it as testOpensearchProcessCannotExit
?
Description
This PR adds integration tests for the systemd unit file that ships with the opensearch distribution. The tests run on the systemd service that runs within a docker container. The tests have been written to run with the systemd unit file that is expected to release with version 3.0 .
Supporting References
Related PR: #17107
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.