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

[Build][Test] OS and OSD improvements plus support for Playground #1756

Closed
wants to merge 3 commits into from

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Mar 15, 2022

Description

While using a child folder 'Playground' in Jenkins I had to do a number of
changes to support a successful test run. I also included the ability to
skip publishing the notification not to spam channels from builds from
for example Playground. Also ability to skip building docker because
from what I could tell it was publishing docker images from Playground
pipelines as well.

Finally, some cleanup to integ tests to use the input manifest to container image.
Also, make integ tests run in docker container for OpenSearch. Use libtester for
detectTestDockerAgent

Signed-off-by: Kawika Avilla kavilla414@gmail.com

Issues Resolved

#1688
#1687
#1686
#1758

Check List

  • Commits are signed per the DCO using --signoff

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.

@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2022

Codecov Report

Merging #1756 (bffdc7a) into main (4a537b7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #1756   +/-   ##
=========================================
  Coverage     94.68%   94.68%           
  Complexity       18       18           
=========================================
  Files           171      172    +1     
  Lines          3554     3555    +1     
  Branches         27       27           
=========================================
+ Hits           3365     3366    +1     
  Misses          185      185           
  Partials          4        4           
Impacted Files Coverage Δ
...sts/jenkins/jobs/DetectTestDockerAgent_Jenkinsfile 100.00% <ø> (ø)
.../jenkins/jobs/CreateTestResultsMessage_Jenkinsfile 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a537b7...bffdc7a. Read the comment docs.

@peterzhuamazon
Copy link
Member

#1394
Ever consider making this change so playground runs the same?

@kavilla
Copy link
Member Author

kavilla commented Mar 15, 2022

#1394 Ever consider making this change so playground runs the same?

Couple things about that. We merged that change for copying artifacts for OSD builds which helps with building in Playground.

However, since we build in Playground it publishes our artifacts to S3 under Playground\${JOB_NAME}. For integration tests and soon to be added BWC tests, it will need that full project name to download the correct build if it is under a nested folder.

If the suggestion is to regardless of the job always build and publish ${$JOB_BASE_NAME} then nested pipelines like Playground will also flood the some base folder as the production ready releases. Which I wouldn't recommend but we can go that route if it's preferred @peterzhuamazon

@tianleh
Copy link
Member

tianleh commented Mar 15, 2022

Great improvement to avoid spamming the notification! We could consider sending such notification to another channel, e.g 'opensearch-distribution-builds-staging' as a future improvement.

@kavilla kavilla force-pushed the avillk/playground branch 3 times, most recently from 1664118 to 142b541 Compare March 17, 2022 05:25
While using a child folder 'Playground' in Jenkins I had to do a number of
changes to support a successful test run. I also included the ability to
skip publishing the notification not to spam channels from builds from
for example `Playground`. Also ability to skip building docker because
from what I could tell it was publishing docker images from `Playground`
pipelines as well.

Finally, some cleanup to integ tests to use the input manifest to container image.
Also, make integ tests run in docker container for OpenSearch.

Issues resolved:
opensearch-project#1688
opensearch-project#1687
opensearch-project#1686

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@kavilla kavilla force-pushed the avillk/playground branch from 142b541 to 446eac5 Compare March 17, 2022 06:01
@kavilla kavilla mentioned this pull request Mar 17, 2022
1 task
Issue Resolved:
opensearch-project#1758

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@dblock dblock requested a review from abhinavGupta16 March 18, 2022 14:57
@kavilla
Copy link
Member Author

kavilla commented Mar 18, 2022

Just to showcase how it looks,

This is how it looks when manually triggering a build:

Screen Shot 2022-03-18 at 12 55 32 PM

Build is triggered skipping the build docker step:

Screen Shot 2022-03-18 at 12 56 11 PM

Finally showcasing the integ tests (please note this was me passing an non existing test yaml file thus it was caught in the verify params stage and aborted the workflow and threw an error)

Screen Shot 2022-03-18 at 12 57 05 PM

The cron trigger will by default pass only the input manifest if we don't automatically add the test manifest, but it will fail the build but it will show a failure in the integ tests. Perhaps we can introduced a skipped icon if no test manifest was found. All the other values will be defaulted so it will default publish notifications and default build docker if no value is passed for those params.

stage('verify-parameters') {
steps {
script {
env.INTEG_TEST_JOB_NAME = INTEG_TEST_JOB_NAME != '' ?
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the check here since INTEG_TEST_JOB_NAME always has the default value which is the same as the DEFAULT_INTEG_TEST_JOB_NAME if not assigned?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably me being too protective but technically someone can kick off a build and empty out the string for INTEG_TEST_JOB_NAME. I can remove this if we do not want to handle those situations.

Copy link
Member

Choose a reason for hiding this comment

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

Had some offline discussion with @kavilla, this makes sense to me that we could leave the parameter empty with this stage of check. These situation could happen for example when we pass the params from cron jobs. This shouldn't be a blocker for me.

@@ -65,22 +90,19 @@ pipeline {
echo "artifactUrl (x64): ${artifactUrl}"

def integTestResults =
build job: INTEG_TEST_JOB_NAME,
build job: env.INTEG_TEST_JOB_NAME,
Copy link
Member

Choose a reason for hiding this comment

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

I think according to @abhinavGupta16, use env prefix may cause some issues with our tester. Could you help confirm it? @abhinavGupta16

Copy link
Member Author

Choose a reason for hiding this comment

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

Here I wanted to ensure the right variable is used. If we go the route not worrying about the empty string case (which would require someone manually triggering the build and clearing it) then I will remove theDEFAULT and only use the param and remove the env prefix.

}
stages {
stage('verify-parameters') {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. I don't think this stage is necessary.

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Copy link
Member

@zelinh zelinh left a comment

Choose a reason for hiding this comment

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

I really like the features of checking if we want to build docker or publish notifications so that I don't have to comment them out anymore for testing.

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