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

rm workspace/aqa-tests/TKG if cleanWs() fails #4426

Merged
merged 1 commit into from
Mar 21, 2023
Merged

Conversation

sophia-guo
Copy link
Contributor

@sophia-guo sophia-guo commented Mar 15, 2023

Related #4417 (comment)

Related adoptium/infrastructure#2630

Signed-off-by: Sophia Guo [email protected]

@sophia-guo sophia-guo requested review from llxia and smlambert March 15, 2023 18:09
@smlambert
Copy link
Contributor

Looking at this code, it feels like there is a big chunk of duplicate code in the zos and non-zos case:

retry(5) {
if (retry_count > 0) {
sleep(sleep_time)
}
retry_count++
timeout(time: 1, unit: 'HOURS') {
try {
cleanWs disableDeferredWipeout: true, deleteDirs: true
} catch (Exception e) {
echo 'Exception: ' + e.toString()
//cleanWs has issue to delete workspace that contains non-ASCII filename in TKG output https://issues.jenkins.io/browse/JENKINS-33478
//cannot delete workspace directly. Otherwise, Jenkins job will abort due to missing workspace
sh "rm -rf ${env.WORKSPACE}/aqa-tests/TKG"
// call cleanWs() again
cleanWs disableDeferredWipeout: true, deleteDirs: true
}
}

and
retry(5) {
if (retry_count > 0) {
sleep(sleep_time)
}
retry_count++
timeout(time: 1, unit: 'HOURS') {
try {
cleanWs disableDeferredWipeout: true, deleteDirs: true
} catch (Exception e) {
echo 'Exception: ' + e.toString()
//cleanWs has issue to delete workspace that contains non-ASCII filename in TKG output https://issues.jenkins.io/browse/JENKINS-33478
//cannot delete workspace directly. Otherwise, Jenkins job will abort due to missing workspace
sh "rm -rf ${env.WORKSPACE}/aqa-tests/TKG"
// call cleanWs() again
cleanWs disableDeferredWipeout: true, deleteDirs: true
}
}

Is there an opportunity to also reduce duplication within runTest()?

@llxia
Copy link
Contributor

llxia commented Mar 16, 2023

Also, the cleanWs code in this PR is duplicated from

try {
// cleanWs() does not work in some cases, so set opts below
cleanWs disableDeferredWipeout: true, deleteDirs: true
} catch (Exception e) {
echo 'Exception: ' + e.toString()
//cleanWs has issue to delete workspace that contains non-ASCII filename in TKG output https://issues.jenkins.io/browse/JENKINS-33478
//cannot delete workspace directly. Otherwise, Jenkins job will abort due to missing workspace
sh "rm -rf ${env.WORKSPACE}/aqa-tests/TKG"
// call cleanWs() again
cleanWs disableDeferredWipeout: true, deleteDirs: true
}
.

We should create a function for it as it is used in 3 places.

@sophia-guo
Copy link
Contributor Author

@sophia-guo
Copy link
Contributor Author

Defined the function forceCleanWS() twice. One is in jenkinsfileBase ( for post stage of jenkins job) and one is in openjdk_tests ( Initial stage of the jenkins job). Otherwise extra the git clone ( an extra git clone for this specific case) or curl the file jenkinsfileBase file would be needed. I'm fine with two definitions, but open to other suggestions.

try {
cleanWs disableDeferredWipeout: true, deleteDirs: true
} catch (Exception e) {
echo 'Exception: ' + e.toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace?

@@ -295,7 +295,7 @@ def runTest() {
}
retry_count++
timeout(time: 1, unit: 'HOURS') {
cleanWs disableDeferredWipeout: true, deleteDirs: true
forceCleanWS()
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace?

try {
cleanWs disableDeferredWipeout: true, deleteDirs: true
} catch (Exception e) {
echo 'Exception: ' + e.toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace again

@sophia-guo sophia-guo force-pushed the rm branch 2 times, most recently from 804a776 to 9d059c8 Compare March 21, 2023 13:21
@@ -1246,7 +1235,7 @@ def run_parallel_tests() {
def childJobs = parallel parallel_tests
node {
// cleanWs() does not work in some cases, so set opts below
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need the comment? If so, it should be moved before line1298?

buildenv/jenkins/JenkinsfileBase Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Show resolved Hide resolved
buildenv/jenkins/openjdk_tests Show resolved Hide resolved
buildenv/jenkins/openjdk_tests Show resolved Hide resolved
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

LGTM

@llxia llxia merged commit 5ff302a into adoptium:master Mar 21, 2023
@sophia-guo sophia-guo self-assigned this Apr 17, 2023
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.

4 participants