-
Notifications
You must be signed in to change notification settings - Fork 277
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
Automate integration tests jenkins job #1429
Automate integration tests jenkins job #1429
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1429 +/- ##
============================================
- Coverage 94.45% 94.40% -0.06%
Complexity 12 12
============================================
Files 141 144 +3
Lines 3084 3090 +6
Branches 8 8
============================================
+ Hits 2913 2917 +4
- Misses 164 166 +2
Partials 7 7
Continue to review full report at Codecov.
|
21e64ac
to
996452d
Compare
// script { | ||
// def stashed = lib.jenkins.Messages.new(this).get(['build-x64', 'build-arm64']) | ||
|
||
// publishNotification( |
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.
You can comment out the body of publishNotification
which may be easier than commenting out the steps everywhere ;)
d5bf1c7
to
4337856
Compare
FYI. I have a task to trigger OpenSearch Dashboards integ test post bundle build #1372 We can sync up to see if we can leverage each other's 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.
Let's have test cases for all the jenkinsfile
that are added.
distribution-build.jenkinsfile
integ-test.jenkinsfile
Tests have been added for I'm not aware of any other ways to test these in an automated way, do you have any suggestion? |
041e4d4
to
dcdaa2f
Compare
We can add tests for the intire job ina very similar way to what we do for our libraries. You can refer to this as reference. I hope this helps |
Link seems to be broken. I'm still unsure what you mean |
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.
Looking really close, could you remove all the commented out code at this point?
This file tests the opensearch-build/tests/jenkins/TestSignArtifacts.groovy Lines 43 to 60 in 2ca4c8d
|
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.
Are we using/will we ever use jenkins/test/orchestrator/Jenkinsfile
?
I'm good with this putting back notifications and removing any Tyler-references ;)
I think the framework is worth leaving for now, to help with future work at getting the entire test orchestrator running. I imagine we will want the distribution build and/or other test pipelines to be able to kick off all integ/perf/bwc tests cc @peternied |
my vote is to nuke anything unused (and doesn't work), and move it to a link to a specific commit for anyone who wants to resurrect it when it's ready to be used |
From my understanding this project was close to complete and was somewhat working at one time, but has fallen behind regarding parameters being passed, etc. There's mentions in the READMEs (e.g. here) about this orchestrator project and tracking github issues for them. Do you think it's worth removing all of this, or leaving as-is and just making sure the tracking issues in the READMEs are up-to-date? |
+1; however, I am comfortable with making this a new PR focused on removing these components and fixing the documentation |
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 did a deep dive today, and that resulted in some things coming out. I'd recommend doing some smaller PRs that would make this more manageable.
src/jenkins/BuildManifest.groovy
Outdated
@@ -49,6 +51,23 @@ class BuildManifest implements Serializable { | |||
this.getArtifactRoot(jobName, buildNumber) | |||
].join('/') | |||
} | |||
|
|||
public String getDistUrl(String publicArtifactUrl = 'https://ci.opensearch.org/ci/dbc', String jobName, String buildNumber) { |
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.
This function is used to reverse engineer the url so we can pass in the root path to the test system. Instead could we update the python code to accept the full path to the build manifest? It reduces the number of parameters we pass around greatly
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.
After looking further, it looks like the root path is used, because both the build manifest (ending in /builds/opensearch/manifest.yml
) and the bundle manifest (ending in /dist/opensearch/manifest.yml
) are both used for a few things in the dependency installer - the build manifest is used for downloading maven dependencies, and the bundle manifest is used for downloading the distribution (via location
param stored in the bundle manifest).
I prefer to keep the root url rather than add parameters here to keep the interface simpler. It's already a bit complex when running Dashboards tests since multiple root urls are needed already. Lmk what you think.
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.
Also note that getDistUrl()
in particular is only used in the distribution-build workflow to generate the build manifest url to pass to the integ-test workflow. The existing getArtifactRootUrl()
is the one used for reverse engineering currently, and is called in runIntegTestScript.groovy
.
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 should consider taking build output such as maven and promoting it to the distribution manifest so we can use a single manifest as input.
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.
Wouldn't this be blending the purposes of these manifests too much? From my understanding, the build manifest is for listing out the artifacts and related dependencies per component that are included in the build. The bundle/distribution manifest is a simplified list of built components, with references to their build locations.
Agreed there is quite a bit of overlap between the two though, where maybe it's worth considering merging these into a single manifest? I may not have the most context for why it was designed this way to begin with, but I feel that only one may be needed.
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.
Agreed. It's beyond the scope of this issue, but the idea was that the same build is being used to assemble a different distribution, e.g. cross-platform.
vars/runIntegTests.groovy
Outdated
parameters: [ | ||
string(name: 'TEST_MANIFEST', value: "${testManifest}"), | ||
string(name: 'BUILD_MANIFEST_URL', value: "${buildManifest}"), | ||
] |
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 should add the parameter wait: false
so that the build workflow isn't blocked
We should also add the parameter propagate: false
so if the tests pass/fail it doesn't impact the distribution-build job.
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.
Similar to my question here, how do we want to go about test failures/successes impacting the jenkins jobs?
I see 2 main paths forward:
- Fail the step if the tests failed, which would cause the entire job to fail
- distribution-build Jenkins job should always succeed (assuming no syntax / other workflow-related bugs), and test success/failure is handled via notifications and/or other jenkins jobs that will fail if the tests fail (maybe integ-test itself?)
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 put a more detail reply on that first question, for now only job failure == workflow failure is more useful because we are using as we are building so we are still focused on stability of the workflow
-
As you've written it I think so. Note; I think we can revisit this when we have an automated action like 'deploy to production' that has the required pre-reqs run. I think we would want to make sure things like 8+ hour long build jobs are still correcting rolling up state across different workflows.
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.
Right, I think that makes a lot of sense. We can follow this fail-fast strategy for now.
Got it, makes sense. I'll update #1447 with some details and resolve in a separate PR |
To break this up easier, I'm thinking of splitting this up into 3 parts / PRs:
|
2654e7d
to
fa6873f
Compare
9a43de4
to
83580f3
Compare
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
…tegTests() Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
…rious cleanup Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
…eters comments Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
83580f3
to
386794e
Compare
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
…ad of bundle Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
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.
This looks great. I have strong feelings about the utility library, WDYT?
vars/getBuildManifestInfo.groovy
Outdated
String buildId = buildManifest.getArtifactBuildId() | ||
|
||
return [buildManifestPath, buildId] | ||
} |
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.
Returning a tuple limits the utility of this shared function. As suggested bellow, maybe something like downloadBuildManifest
that downloads the remote manifest and returns the buildManifest
object, then all the future uses are available to the caller. I can see another workflow needing to download the manifest but use some other manifest property like platform.
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.
Agreed this is a better approach, changed to downloadBuildManifest()
in c60435a
steps { | ||
script { | ||
// Fetching the build-specific values and assigning to their env vars | ||
(buildManifestPath, buildId) = getBuildManifestInfo() |
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.
This API is strange, I'd prefer it'd download a manifest and return that.
def buildManifest = downloadBuildManifest(manifest: "${BUILD_MANIFEST_URL}")
...
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 much prefer this approach. Functions with no side effects are my jam
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 you mean you prefer the version as implemented now in the PR?
Note that shared functions are global. This one does not "get build manifest info", it "get some very specific build manifest info used in integ-test workflow", are we going to be adding another function like this when we need some other value from a remote manifest that will be an identical copy but return different fields?
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've updated this, see #1429 (comment)
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.
Great work!
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Description
This PR updates the existing integ test jenkins workflow, which currently doesn't work and is using outdated parameters.
More specifically:
jenkins/test/testsuite/Jenkinsfile
) tojenkins/opensearch/integ-test.jenkinsfile
. Params were modified to try to prevent link-building logic which has caused broken builds in the past.runIntegTestScript
groovy script to execute the actualtest
shell scriptgetBuildManifestInfo
groovy script to use the passed-in build manifest URL to pull out build-related informationThere will be follow up PRs to finish automating integration tests:
jenkins/test
) and associated READMEsNOTE: this PR is a WIP for now. Things to wrap up before it is ready:
test.sh
instead of root URLScreenshot - working integ test job
Issues Resolved
Closes #1114
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.