From 57fe8575e3f6e2562ad8950000ac4395d0d5fa8a Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 08:59:27 +0530 Subject: [PATCH 01/10] Addressed the comments received from Devops team --- private-cicd/Jenkinsfile | 405 +++++++++++++++--- private-cicd/README.md | 17 +- private-cicd/docs/CREATE-PRESUBMIT-JOB.md | 22 +- private-cicd/docs/PRIVATE-CICD-GUIDE.md | 26 +- private-cicd/scripts/resolve-pull-request.py | 133 ++++++ .../scripts/test_resolve_pull_request.py | 82 ++++ 6 files changed, 583 insertions(+), 102 deletions(-) create mode 100644 private-cicd/scripts/resolve-pull-request.py create mode 100644 private-cicd/scripts/test_resolve_pull_request.py diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 8d1dead65701..396154123ba6 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -146,6 +146,93 @@ boolean hasIntegrationStarted(build) { return environment?.get('PRESUBMIT_INTEGRATION_STARTED') == 'true' } +@NonCPS +String revisionHeadSha(revision) { + if (revision == null) { + return '' + } + try { + def hash = revision.pullHash?.toString() ?: '' + if (hash ==~ /^[0-9a-fA-F]{40}$/) { + return hash + } + } catch (Exception ignored) { + return '' + } + return '' +} + +@NonCPS +String indexedHeadSha(build) { + return revisionHeadSha(build?.getAction(jenkins.scm.api.SCMRevisionAction)?.revision) +} + +// A multibranch child only contains builds for one PR. Stop an older run of +// that child before it holds the VM. A queued duplicate of the same indexed +// SHA is cancelled; a queued item with no SHA is left alone. +@NonCPS +def supersedeMultibranchBuilds(String jobName, int buildNumber, String sha) { + def jenkins = jenkins.model.Jenkins.get() + def job = jenkins.getItemByFullName(jobName) + if (job == null) { + throw new IllegalStateException("Cannot find Jenkins job '${jobName}'.") + } + + def results = [] + def latest = revisionHeadSha(job.getAction(jenkins.scm.api.SCMRevisionAction)?.revision) + if (latest && sha && !latest.equalsIgnoreCase(sha)) { + def self = job.getBuildByNumber(buildNumber) + if (self != null && self.isBuilding() && !hasIntegrationStarted(self)) { + self.doStop() + results << "Stopping build #${buildNumber}: a newer head ${latest.take(12)} is indexed." + } else if (self != null && hasIntegrationStarted(self)) { + results << "Not stopping build #${buildNumber}: ONTAP integration has started." + } + return results + } + jenkins.queue.items.findAll { item -> + item.task?.fullName == job.fullName + }.each { item -> + def queued = revisionHeadSha(item.getAction(jenkins.scm.api.SCMRevisionAction)?.revision) + if (queued && sha && queued.equalsIgnoreCase(sha)) { + jenkins.queue.cancel(item) + results << "Cancelled a queued build for ${sha.take(12)}." + } + } + job.getBuilds().findAll { build -> + build.isBuilding() && build.number < buildNumber + }.each { build -> + if (hasIntegrationStarted(build)) { + results << "Not aborting build #${build.number}: ONTAP integration has started." + } else { + build.doStop() + results << "Aborted superseded build #${build.number}." + } + } + return results +} + +@NonCPS +int diffNumberForResolvedSha(String jobName, int buildNumber, String sha) { + def job = jenkins.model.Jenkins.get().getItemByFullName(jobName) + if (job == null || !sha) { + return 1 + } + def previousShas = [] as Set + job.getBuilds().findAll { + it.number < buildNumber + }.each { build -> + def previousSha = build.getAction( + org.jenkinsci.plugins.workflow.cps.EnvActionImpl + )?.getOverriddenEnvironment()?.get('RESOLVED_SOURCE_SHA') + if (previousSha) { + previousShas << previousSha + } + } + previousShas.remove(sha) + return previousShas.size() + 1 +} + @NonCPS def abortOlderBuildsForSource(String jobName, int buildNumber, String sourceKey) { def job = jenkins.model.Jenkins.get().getItemByFullName(jobName) @@ -198,6 +285,44 @@ boolean isValidBranchName(String branchName) { } } +boolean isMultibranchPullRequest() { + return env.CHANGE_ID?.trim() +} + +// Branch Source does not fill Declarative parameters. A multibranch run stores +// one resolved pull-request context in the environment and reads it here. +def sourceValue(String name) { + if (env.PRESUBMIT_MULTIBRANCH_READY == 'true') { + switch (name) { + case 'SOURCE_MODE': + return env.EFFECTIVE_SOURCE_MODE + case 'PR_ID': + return env.EFFECTIVE_PR_ID + case 'PR_ACTION': + return env.EFFECTIVE_PR_ACTION + case 'PR_DRAFT': + return env.EFFECTIVE_PR_DRAFT + case 'PR_REPOSITORY': + return env.EFFECTIVE_PR_REPOSITORY + case 'PR_BASE_BRANCH': + return env.EFFECTIVE_PR_BASE_BRANCH + case 'PR_HEAD_BRANCH': + return env.EFFECTIVE_PR_HEAD_BRANCH + case 'PR_HEAD_SHA': + return env.EFFECTIVE_PR_HEAD_SHA + case 'PR_AUTHOR_LOGIN': + return env.EFFECTIVE_PR_AUTHOR_LOGIN + case 'PR_AUTHOR_EMAIL': + return env.EFFECTIVE_PR_AUTHOR_EMAIL + case 'PR_URL': + return env.EFFECTIVE_PR_URL + case 'PR_TITLE': + return env.EFFECTIVE_PR_TITLE + } + } + return params.get(name) +} + def validateInventory(Map inventory) { if (containsPlaceholder(inventory)) { error('VM inventory still contains PLACEHOLDER- values.') @@ -321,13 +446,13 @@ void recordStageEvent(String stageName, String status) { } String presubmitSubjectBase() { - def recipientName = params.PR_AUTHOR_LOGIN?.trim() + def recipientName = sourceValue('PR_AUTHOR_LOGIN')?.trim() if (!recipientName && env.PRESUBMIT_MAIL_TO?.contains('@')) { recipientName = env.PRESUBMIT_MAIL_TO.substring(0, env.PRESUBMIT_MAIL_TO.indexOf('@')) } - def sourceName = params.SOURCE_MODE == 'branch' ? - "branch ${params.SOURCE_BRANCH}" : "PR-${params.PR_ID}" - def title = params.PR_TITLE?.replaceAll(/[\r\n]+/, ' ')?.trim() + def sourceName = sourceValue('SOURCE_MODE') == 'branch' ? + "branch ${params.SOURCE_BRANCH}" : "PR-${sourceValue('PR_ID')}" + def title = sourceValue('PR_TITLE')?.replaceAll(/[\r\n]+/, ' ')?.trim() if (title?.length() > 80) { title = title.take(77) + '...' } @@ -365,15 +490,15 @@ boolean hasGithubAppCredential() { List githubEnvironment() { return [ - "GITHUB_REPOSITORY=${params.PR_REPOSITORY}", - "GITHUB_HEAD_SHA=${params.PR_HEAD_SHA}", - "PR_ID=${params.PR_ID}", - "PR_TITLE=${params.PR_TITLE ?: ''}" + "GITHUB_REPOSITORY=${sourceValue('PR_REPOSITORY')}", + "GITHUB_HEAD_SHA=${sourceValue('PR_HEAD_SHA')}", + "PR_ID=${sourceValue('PR_ID')}", + "PR_TITLE=${sourceValue('PR_TITLE') ?: ''}" ] } void createGithubCheck() { - if (params.SOURCE_MODE != 'pull_request' || !hasGithubAppCredential()) { + if (sourceValue('SOURCE_MODE') != 'pull_request' || !hasGithubAppCredential()) { echo 'GitHub Check creation skipped: GitHub App credential is not configured.' return } @@ -394,7 +519,7 @@ void createGithubCheck() { throw new IllegalStateException("GitHub returned an invalid Check Run ID '${checkRunId}'.") } env.GITHUB_CHECK_RUN_ID = checkRunId - echo "Created GitHub Check Run ${checkRunId} on ${params.PR_HEAD_SHA}." + echo "Created GitHub Check Run ${checkRunId} on ${sourceValue('PR_HEAD_SHA')}." } } } catch (Exception e) { @@ -435,10 +560,10 @@ void completeGithubCheck(String conclusion) { } void resolvePresubmitMailRecipient() { - def webhookEmail = params.PR_AUTHOR_EMAIL?.trim() + def webhookEmail = sourceValue('PR_AUTHOR_EMAIL')?.trim() env.PRESUBMIT_MAIL_TO = webhookEmail?.contains('@') && !webhookEmail.equalsIgnoreCase('null') ? webhookEmail : '' - if (env.PRESUBMIT_MAIL_TO || params.SOURCE_MODE != 'pull_request' || + if (env.PRESUBMIT_MAIL_TO || sourceValue('SOURCE_MODE') != 'pull_request' || !hasGithubAppCredential()) { return } @@ -485,14 +610,14 @@ String presubmitInfoRow(String label, String valueHtml) { } String presubmitStartHtml() { - def source = params.SOURCE_MODE == 'branch' ? - "branch ${params.SOURCE_BRANCH}" : "PR-${params.PR_ID}" - def sourceHtml = params.PR_URL ==~ /^https:\/\/github\.com\/[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\/pull\/[0-9]+$/ ? - "${htmlEscape(source)}" : + def source = sourceValue('SOURCE_MODE') == 'branch' ? + "branch ${params.SOURCE_BRANCH}" : "PR-${sourceValue('PR_ID')}" + def sourceHtml = sourceValue('PR_URL') ==~ /^https:\/\/github\.com\/[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\/pull\/[0-9]+$/ ? + "${htmlEscape(source)}" : htmlEscape(source) def diffNumber = env.PRESUBMIT_DIFF_NUMBER?.trim() def rows = presubmitInfoRow('Source', sourceHtml) + - presubmitInfoRow('Title', htmlEscape(params.PR_TITLE ?: 'not provided')) + presubmitInfoRow('Title', htmlEscape(sourceValue('PR_TITLE') ?: 'not provided')) if (diffNumber) { rows += presubmitInfoRow('Diff', "#${htmlEscape(diffNumber)}") } @@ -524,12 +649,13 @@ results follows when this run finishes.

// GitHub App diagnostic skip them, as does a request that `Check eligibility` // rejected by setting NOT_BUILT. boolean isWorkerStageEnabled() { - return params.SOURCE_MODE in ['pull_request', 'branch'] && - currentBuild.result != 'NOT_BUILT' + return sourceValue('SOURCE_MODE') in ['pull_request', 'branch'] && + currentBuild.result != 'NOT_BUILT' && + currentBuild.currentResult != 'NOT_BUILT' } boolean isDiagnoseRun() { - return params.SOURCE_MODE == 'diagnose' + return sourceValue('SOURCE_MODE') == 'diagnose' } void discoverPullRequests() { @@ -668,10 +794,10 @@ void validateSourceRequest() { TimeZone.getTimeZone('UTC') ) recordStageEvent('Validate source request', 'STARTED') - if (!(params.SOURCE_MODE in ['pull_request', 'branch'])) { - error("Unsupported SOURCE_MODE '${params.SOURCE_MODE}'.") + if (!(sourceValue('SOURCE_MODE') in ['pull_request', 'branch'])) { + error("Unsupported SOURCE_MODE '${sourceValue('SOURCE_MODE')}'.") } - if (params.SOURCE_MODE == 'branch') { + if (sourceValue('SOURCE_MODE') == 'branch') { if (!isValidBranchName(params.SOURCE_BRANCH)) { error('SOURCE_BRANCH is not a valid Git branch name.') } @@ -679,16 +805,16 @@ void validateSourceRequest() { error('SOURCE_SHA must be a full 40-character Git SHA.') } } else { - if (!(params.PR_ID ==~ /^[0-9]+$/)) { + if (!(sourceValue('PR_ID') ==~ /^[0-9]+$/)) { error('PR_ID must be the numeric GitHub pull request number.') } - if (params.PR_REPOSITORY != params.EXPECTED_REPOSITORY) { - error("Repository '${params.PR_REPOSITORY}' is not '${params.EXPECTED_REPOSITORY}'.") + if (sourceValue('PR_REPOSITORY') != params.EXPECTED_REPOSITORY) { + error("Repository '${sourceValue('PR_REPOSITORY')}' is not '${params.EXPECTED_REPOSITORY}'.") } - if (!(params.PR_HEAD_BRANCH ==~ /^[A-Za-z0-9._\/-]+$/)) { + if (!(sourceValue('PR_HEAD_BRANCH') ==~ /^[A-Za-z0-9._\/-]+$/)) { error('PR_HEAD_BRANCH contains unsupported characters.') } - if (!(params.PR_HEAD_SHA ==~ /^[0-9a-fA-F]{40}$/) && params.PR_ACTION != 'closed') { + if (!(sourceValue('PR_HEAD_SHA') ==~ /^[0-9a-fA-F]{40}$/) && sourceValue('PR_ACTION') != 'closed') { error('PR_HEAD_SHA must be a full 40-character Git SHA.') } } @@ -718,15 +844,23 @@ void validateSourceRequest() { } } try { - def sourceKey = params.SOURCE_MODE == 'branch' ? - "branch:${params.SOURCE_BRANCH}" : - "pull_request:${params.PR_ID}" - env.PRESUBMIT_DIFF_NUMBER = diffNumberForSource( - env.JOB_NAME, - env.BUILD_NUMBER.toInteger(), - sourceKey, - env.RESOLVED_SOURCE_SHA - ).toString() + if (isMultibranchPullRequest()) { + env.PRESUBMIT_DIFF_NUMBER = diffNumberForResolvedSha( + env.JOB_NAME, + env.BUILD_NUMBER.toInteger(), + env.RESOLVED_SOURCE_SHA + ).toString() + } else { + def sourceKey = sourceValue('SOURCE_MODE') == 'branch' ? + "branch:${params.SOURCE_BRANCH}" : + "pull_request:${sourceValue('PR_ID')}" + env.PRESUBMIT_DIFF_NUMBER = diffNumberForSource( + env.JOB_NAME, + env.BUILD_NUMBER.toInteger(), + sourceKey, + env.RESOLVED_SOURCE_SHA + ).toString() + } } catch (Exception e) { env.PRESUBMIT_DIFF_NUMBER = '' echo "Unable to determine the diff number: ${e}" @@ -736,37 +870,51 @@ void validateSourceRequest() { // five-minute discovery runs. def diffLabel = env.PRESUBMIT_DIFF_NUMBER ? " diff #${env.PRESUBMIT_DIFF_NUMBER}" : '' - if (params.SOURCE_MODE == 'branch') { + if (sourceValue('SOURCE_MODE') == 'branch') { currentBuild.displayName = "#${env.BUILD_NUMBER} branch ${params.SOURCE_BRANCH}${diffLabel}" currentBuild.description = "Branch ${params.SOURCE_BRANCH} @ ${params.SOURCE_SHA.take(12)}" } else { currentBuild.displayName = - "#${env.BUILD_NUMBER} PR-${params.PR_ID}${diffLabel} ${params.PR_ACTION}" + "#${env.BUILD_NUMBER} PR-${sourceValue('PR_ID')}${diffLabel} ${sourceValue('PR_ACTION')}" currentBuild.description = - "${env.RESOLVED_SOURCE_SHA?.take(12)} ${params.PR_TITLE ?: params.PR_URL}" + "${env.RESOLVED_SOURCE_SHA?.take(12)} ${sourceValue('PR_TITLE') ?: sourceValue('PR_URL')}" } } void checkPresubmitEligibility() { recordStageEvent('Check eligibility', 'STARTED') - if (params.SOURCE_MODE == 'pull_request') { + if (sourceValue('SOURCE_MODE') == 'pull_request') { def eligibleActions = ['opened', 'synchronize', 'reopened', 'ready_for_review'] - if (params.PR_ACTION == 'closed' || - params.PR_DRAFT.toBoolean() || - params.PR_BASE_BRANCH != 'main' || - !(params.PR_ACTION in eligibleActions)) { + if (sourceValue('PR_ACTION') == 'closed' || + sourceValue('PR_DRAFT').toBoolean() || + sourceValue('PR_BASE_BRANCH') != 'main' || + !(sourceValue('PR_ACTION') in eligibleActions)) { currentBuild.result = 'NOT_BUILT' - echo "Ignoring PR-${params.PR_ID}: action=${params.PR_ACTION}, draft=${params.PR_DRAFT}, base=${params.PR_BASE_BRANCH}" + echo "Ignoring PR-${sourceValue('PR_ID')}: action=${sourceValue('PR_ACTION')}, draft=${sourceValue('PR_DRAFT')}, base=${sourceValue('PR_BASE_BRANCH')}" } } } void abortSupersededRun() { recordStageEvent('Abort superseded run', 'STARTED') - def sourceKey = params.SOURCE_MODE == 'branch' ? + if (isMultibranchPullRequest()) { + try { + supersedeMultibranchBuilds( + env.JOB_NAME, + env.BUILD_NUMBER.toInteger(), + env.RESOLVED_SOURCE_SHA + ).each { result -> + echo result + } + } catch (Exception e) { + echo "Unable to inspect older builds; no build was aborted: ${e}" + } + return + } + def sourceKey = sourceValue('SOURCE_MODE') == 'branch' ? "branch:${params.SOURCE_BRANCH}" : - "pull_request:${params.PR_ID}" + "pull_request:${sourceValue('PR_ID')}" try { abortOlderBuildsForSource( env.JOB_NAME, @@ -782,8 +930,110 @@ void abortSupersededRun() { void checkoutCiScripts() { recordStageEvent('Checkout CI scripts', 'STARTED') + if (env.PRESUBMIT_CI_SHA?.trim() && fileExists('private-cicd/scripts/resolve-pull-request.py')) { + echo "CI scripts already checked out from main at ${env.PRESUBMIT_CI_SHA}." + return + } deleteDir() - checkout(scm) + def remote = [ + url: params.CLOUDSTACK_GIT_URL, + refspec: '+refs/heads/main:refs/remotes/origin/main' + ] + def credentialId = params.GIT_CREDENTIALS_ID?.trim() + if (!credentialId) { + credentialId = params.GITHUB_APP_CREDENTIALS_ID?.trim() + } + if (credentialId && !credentialId.startsWith('PLACEHOLDER-')) { + remote.credentialsId = credentialId + } + def cloneOptions = [[ + $class: 'CloneOption', + honorRefspec: true, + noTags: true, + shallow: true, + depth: 1 + ]] + try { + checkout([ + $class: 'GitSCM', + branches: [[name: '*/main']], + doGenerateSubmoduleConfigurations: false, + extensions: cloneOptions + [[ + $class: 'SparseCheckoutPaths', + sparseCheckoutPaths: [[path: 'private-cicd']] + ]], + userRemoteConfigs: [remote] + ]) + } catch (Exception e) { + echo "Sparse CI checkout unavailable, using a full main checkout: ${e}" + deleteDir() + checkout([ + $class: 'GitSCM', + branches: [[name: '*/main']], + doGenerateSubmoduleConfigurations: false, + extensions: cloneOptions, + userRemoteConfigs: [remote] + ]) + } + env.PRESUBMIT_CI_SHA = sh(script: 'git rev-parse HEAD', returnStdout: true).trim() + echo "Checked out CI scripts from main at ${env.PRESUBMIT_CI_SHA}." +} + +void prepareMultibranchPullRequest() { + checkoutCiScripts() + def indexed = indexedHeadSha(currentBuild.rawBuild) + if (!(indexed ==~ /^[0-9a-fA-F]{40}$/)) { + currentBuild.result = 'NOT_BUILT' + echo "Not building PR-${env.CHANGE_ID}: Jenkins did not index a 40-character pull request head SHA." + return + } + def status = 1 + withCredentials([ + usernamePassword( + credentialsId: params.GITHUB_APP_CREDENTIALS_ID, + usernameVariable: 'GITHUB_APP', + passwordVariable: 'GITHUB_ACCESS_TOKEN' + ) + ]) { + withEnv([ + "PRESUBMIT_REPOSITORY=${params.EXPECTED_REPOSITORY}", + "PRESUBMIT_PR_ID=${env.CHANGE_ID}", + "PRESUBMIT_INDEXED_SHA=${indexed}", + "PRESUBMIT_CHANGE_TARGET=${env.CHANGE_TARGET ?: ''}" + ]) { + status = sh( + script: '''python3 private-cicd/scripts/resolve-pull-request.py \ + --repository "$PRESUBMIT_REPOSITORY" \ + --pr-id "$PRESUBMIT_PR_ID" \ + --indexed-sha "$PRESUBMIT_INDEXED_SHA" \ + --change-target "$PRESUBMIT_CHANGE_TARGET" \ + --output multibranch-source.json''', + returnStatus: true + ) + } + } + def decision = fileExists('multibranch-source.json') ? readJSON(file: 'multibranch-source.json') : [:] + if (status != 0 || !decision.eligible) { + currentBuild.result = 'NOT_BUILT' + echo "Not building PR-${env.CHANGE_ID}: ${decision.reason ?: 'pull request lookup failed.'}" + return + } + env.PRESUBMIT_MULTIBRANCH_READY = 'true' + env.EFFECTIVE_SOURCE_MODE = 'pull_request' + env.EFFECTIVE_PR_ID = decision.pr_id.toString() + env.EFFECTIVE_PR_ACTION = 'synchronize' + env.EFFECTIVE_PR_DRAFT = 'false' + env.EFFECTIVE_PR_REPOSITORY = params.EXPECTED_REPOSITORY + env.EFFECTIVE_PR_BASE_BRANCH = decision.pr_base_branch.toString() + env.EFFECTIVE_PR_HEAD_BRANCH = decision.pr_head_branch.toString() + env.EFFECTIVE_PR_HEAD_SHA = decision.pr_head_sha.toString() + env.EFFECTIVE_PR_AUTHOR_LOGIN = decision.pr_author_login?.toString() ?: '' + env.EFFECTIVE_PR_AUTHOR_EMAIL = decision.pr_author_email?.toString() ?: '' + env.EFFECTIVE_PR_URL = decision.pr_url?.toString() ?: '' + env.EFFECTIVE_PR_TITLE = decision.pr_title?.toString() ?: '' + env.RESOLVED_SOURCE_SHA = env.EFFECTIVE_PR_HEAD_SHA + env.RESOLVED_SOURCE_ID = "pr-${env.EFFECTIVE_PR_ID}" + echo "Accepted PR-${env.EFFECTIVE_PR_ID} at ${env.RESOLVED_SOURCE_SHA}." } // A full worker run takes over an hour, so reporting problems are diagnosed @@ -793,11 +1043,11 @@ void diagnoseGithubApp() { if (!hasGithubAppCredential()) { error('GITHUB_APP_CREDENTIALS_ID is unset or still a placeholder.') } - def headSha = params.PR_HEAD_SHA?.trim() ?: params.SOURCE_SHA?.trim() + def headSha = sourceValue('PR_HEAD_SHA')?.trim() ?: params.SOURCE_SHA?.trim() if (!(headSha ==~ /^[0-9a-f]{40}$/)) { error('Set PR_HEAD_SHA to a full 40-character commit SHA to diagnose.') } - def repository = params.PR_REPOSITORY?.trim() ?: params.EXPECTED_REPOSITORY + def repository = sourceValue('PR_REPOSITORY')?.trim() ?: params.EXPECTED_REPOSITORY withCredentials([ usernamePassword( credentialsId: params.GITHUB_APP_CREDENTIALS_ID, @@ -808,7 +1058,7 @@ void diagnoseGithubApp() { withEnv([ "GITHUB_REPOSITORY=${repository}", "GITHUB_HEAD_SHA=${headSha}", - "PR_ID=${params.PR_ID ?: ''}" + "PR_ID=${sourceValue('PR_ID') ?: ''}" ]) { sh 'private-cicd/scripts/github-check-run.sh diagnose' } @@ -863,18 +1113,18 @@ void checkoutSource() { recordStageEvent('Checkout source', 'STARTED') dir(env.SOURCE_DIR) { deleteDir() - def isBranch = params.SOURCE_MODE == 'branch' - def sourceBranch = isBranch ? params.SOURCE_BRANCH : params.PR_HEAD_BRANCH - def sourceSha = isBranch ? params.SOURCE_SHA : params.PR_HEAD_SHA - def sourceLabel = isBranch ? "branch:${sourceBranch}" : "pull_request:${params.PR_ID}" + def isBranch = sourceValue('SOURCE_MODE') == 'branch' + def sourceBranch = isBranch ? params.SOURCE_BRANCH : sourceValue('PR_HEAD_BRANCH') + def sourceSha = isBranch ? params.SOURCE_SHA : sourceValue('PR_HEAD_SHA') + def sourceLabel = isBranch ? "branch:${sourceBranch}" : "pull_request:${sourceValue('PR_ID')}" def remoteRef = isBranch ? "refs/remotes/origin/source/${sourceBranch}" : - "refs/remotes/origin/pr/${params.PR_ID}" + "refs/remotes/origin/pr/${sourceValue('PR_ID')}" def remote = [ url: params.CLOUDSTACK_GIT_URL, refspec: isBranch ? "+refs/heads/${sourceBranch}:${remoteRef}" : - "+refs/pull/${params.PR_ID}/head:${remoteRef}" + "+refs/pull/${sourceValue('PR_ID')}/head:${remoteRef}" ] if (params.GIT_CREDENTIALS_ID?.trim()) { remote.credentialsId = params.GIT_CREDENTIALS_ID @@ -987,9 +1237,9 @@ void deployAndRunOntapIntegration() { env.VM_NAME = vm.vcenter_vm as String env.VM_SNAPSHOT = vm.snapshot as String env.VM_BRIDGE = vm.bridge as String - def sourceDescription = params.SOURCE_MODE == 'branch' ? + def sourceDescription = sourceValue('SOURCE_MODE') == 'branch' ? "branch=${params.SOURCE_BRANCH}" : - "PR-${params.PR_ID}" + "PR-${sourceValue('PR_ID')}" currentBuild.description = "${sourceDescription} VM=${env.VM_ID}" echo "Reserved compatible VM '${env.VM_ID}' via '${env.LOCKED_VM}'." @@ -1063,7 +1313,11 @@ void deployAndRunOntapIntegration() { } void publishPresubmitResults() { - if (params.SOURCE_MODE == 'discover') { + if (isMultibranchPullRequest() && currentBuild.currentResult == 'NOT_BUILT') { + echo "Multibranch PR-${env.CHANGE_ID} was not built." + return + } + if (sourceValue('SOURCE_MODE') == 'discover') { echo 'Discovery build completed; worker reporting was skipped.' return } @@ -1088,15 +1342,15 @@ void publishPresubmitResults() { if (!env.PRESUBMIT_MAIL_TO?.trim()) { resolvePresubmitMailRecipient() } - def sourceName = params.SOURCE_MODE == 'branch' ? - "branch ${params.SOURCE_BRANCH}" : "PR-${params.PR_ID}" + def sourceName = sourceValue('SOURCE_MODE') == 'branch' ? + "branch ${params.SOURCE_BRANCH}" : "PR-${sourceValue('PR_ID')}" try { if (fileExists('private-cicd/scripts/generate-presubmit-report.py')) { withEnv([ "PRESUBMIT_REPORT_RESULT=${result}", "PRESUBMIT_REPORT_SOURCE=${sourceName}", - "PRESUBMIT_REPORT_SOURCE_URL=${params.PR_URL ?: ''}", - "PRESUBMIT_REPORT_TITLE=${params.PR_TITLE ?: ''}", + "PRESUBMIT_REPORT_SOURCE_URL=${sourceValue('PR_URL') ?: ''}", + "PRESUBMIT_REPORT_TITLE=${sourceValue('PR_TITLE') ?: ''}", "PRESUBMIT_REPORT_SHA=${env.RESOLVED_SOURCE_SHA}", "PRESUBMIT_REPORT_DIFF=${env.PRESUBMIT_DIFF_NUMBER ?: ''}", "PRESUBMIT_REPORT_VM=${env.VM_ID ?: ''}" @@ -1193,15 +1447,11 @@ spec: timeout(time: 8, unit: 'HOURS') } - triggers { - cron('H/5 * * * *') - } - parameters { choice( name: 'SOURCE_MODE', choices: ['discover', 'pull_request', 'branch', 'diagnose'], - description: 'Timer builds use discover. Self-queued workers use pull_request. Use branch for a manual exact-SHA build, or diagnose to probe GitHub App Checks access using PR_HEAD_SHA.' + description: 'Legacy discover mode is not scheduled. Multibranch pull requests ignore this default. Use pull_request for a manual PR build, branch for an exact SHA, or diagnose to probe GitHub App Checks access.' ) string(name: 'SOURCE_BRANCH', defaultValue: '', description: 'Manual branch mode: remote branch name, for example feature/CSTACKEX-223.') string(name: 'SOURCE_SHA', defaultValue: '', description: 'Manual branch mode: exact full 40-character commit SHA.') @@ -1260,7 +1510,7 @@ spec: stage('Discover PR revisions') { when { expression { - return params.SOURCE_MODE == 'discover' + return sourceValue('SOURCE_MODE') == 'discover' && !isMultibranchPullRequest() } } steps { @@ -1270,6 +1520,19 @@ spec: } } + stage('Prepare multibranch pull request') { + when { + expression { + return isMultibranchPullRequest() + } + } + steps { + script { + prepareMultibranchPullRequest() + } + } + } + stage('Validate source request') { when { expression { diff --git a/private-cicd/README.md b/private-cicd/README.md index ae48b35a9a75..df978fd69ddf 100644 --- a/private-cicd/README.md +++ b/private-cicd/README.md @@ -28,10 +28,11 @@ current implementation contract, see ## How it works -The production Jenkins job loads [`Jenkinsfile`](Jenkinsfile) and helper scripts -from the protected NetApp `main` branch. Every five minutes a short discovery -build polls GitHub and self-queues a worker for each unseen PR head SHA. Each -worker checks out the exact commit into a separate `cloudstack-src` directory, +The production job is the multibranch folder `cloudstack-ontap-presubmit-mb`. +Jenkins scans GitHub about once a minute. That scan is not a build. Each +non-draft origin pull request into `main` gets its own child job. The child +loads [`Jenkinsfile`](Jenkinsfile) and helper scripts from protected `main`, +then checks the exact pull-request head SHA out into `cloudstack-src`. A child then: 1. validates the request, builder, credentials, and VM inventory; @@ -48,10 +49,10 @@ replacing the underlying build or test result. ## Entry points -- **Pull request:** `SOURCE_MODE=discover` notices an open non-draft PR update - to `main` and self-queues at most five `SOURCE_MODE=pull_request` workers in - the same job. Extra SHAs wait for a later poll. Workers then wait on the - `cloudstack-presubmit-vm` lock for a lab VM. +- **Pull request:** a folder scan creates one child job per open non-draft + origin pull request into `main`. Drafts get no job. Compiles run together. + Integration waits on the `cloudstack-presubmit-vm` lock. There is no + five-worker cap. GitHub cannot call this Jenkins, so there is no webhook. - **Manual branch:** a separate triggerless job uses `SOURCE_MODE=branch` with a remote branch and exact 40-character commit SHA. - **Local validation:** `scripts/validate-local.sh` checks the CI files without diff --git a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md index c3ce8623a945..9e1b46871e85 100644 --- a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md +++ b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md @@ -19,12 +19,18 @@ # Create the auto-triggered ONTAP presubmit job -This guide creates **one** Pipeline job, `cloudstack-ontap-presubmit`. Timer -builds poll GitHub every five minutes and self-queue one worker build for each -new eligible PR head SHA. Worker builds compile and package concurrently, then -wait for an available `cloudstack-presubmit-vm` Lockable Resource before -integration. Until `feature/CSTACKEX-223` is merged, the job loads trusted CI -from that branch. After merge, change only **Branches to build** to `*/main`. +The current job is the multibranch folder `cloudstack-ontap-presubmit-mb`. +It scans GitHub about once a minute and creates one child job for every open +non-draft origin pull request into `main`. Remote Jenkinsfile Provider loads +`private-cicd/Jenkinsfile` from `main`. Match branches stays off. Draft pull +requests get no job. The folder include filter can be narrowed to one `PR-` +for the first run, then returned to `PR-*`. Do not add a webhook. + +The sections below describe the retired single Pipeline job and its five-minute +`SOURCE_MODE=discover` timer. Do not create that second job. Keep +`cloudstack-ontap-presubmit` disabled after the folder works. Do not delete the +GitHub App, credentials, or the VM lock until a multibranch pull request has +passed. Do not create a GitHub repository webhook. OpenLab Jenkins is not reachable from GitHub.com, so inbound `pull_request` deliveries fail with @@ -36,8 +42,8 @@ item for this presubmit: that would run `Jenkinsfile` from the PR branch. This design loads trusted CI from a fixed branch and checks the PR SHA out separately. -Do not create a separate poller, Multibranch Pipeline, or second production -copy of this job. +The multibranch folder above is the production job. Do not create another +poller or a second production copy. This guide starts before the Jenkins item exists and ends after polling and required Check have been verified. Follow sections 1 through 21 in order. Do diff --git a/private-cicd/docs/PRIVATE-CICD-GUIDE.md b/private-cicd/docs/PRIVATE-CICD-GUIDE.md index a5617af9e47a..367408093e84 100644 --- a/private-cicd/docs/PRIVATE-CICD-GUIDE.md +++ b/private-cicd/docs/PRIVATE-CICD-GUIDE.md @@ -30,9 +30,9 @@ This is the consolidated operator and developer reference. The current [`Jenkins The guide covers: -- the trusted production Pipeline-from-SCM job with five-minute GitHub API - discovery, including first-PR cutover from `feature/CSTACKEX-223` to `main` - ([`CREATE-PRESUBMIT-JOB.md`](CREATE-PRESUBMIT-JOB.md)); +- the multibranch folder `cloudstack-ontap-presubmit-mb`, which scans about + once a minute and runs trusted CI from `main` for every non-draft origin + pull request into `main` ([`CREATE-PRESUBMIT-JOB.md`](CREATE-PRESUBMIT-JOB.md)); - the separate triggerless manual branch job; - local CI-file validation and direct disposable-VM validation; - Stage 1 build, unit-test, and Debian package handoff; @@ -68,12 +68,10 @@ VM lock. ## 3. Architecture and data flow ```text -Jenkins timer -> discover mode (every 5 minutes) - -> GitHub API -> unseen PR head SHAs - -> self-queue at most MAX_ACTIVE_WORKERS pull_request builds - -> extra SHAs deferred with watermark held - -> self-queued worker builds in the same job - -> trusted Jenkinsfile/scripts from protected main +Folder scan about once a minute (not a pipeline build) + -> one child job per open non-draft origin PR into main + -> trusted Jenkinsfile/scripts from protected main + -> exact indexed PR head SHA, confirmed with the pull request API -> request, eligibility, credential, inventory, and builder checks -> exact PR or branch SHA checkout into cloudstack-src -> Maven clean install and unit tests @@ -156,12 +154,10 @@ Lightweight checkout: off PR code cannot replace trusted CI helpers before credentials are used. The source under test is checked out separately. -Until `feature/CSTACKEX-223` is merged, that Jenkinsfile is not on `main`. Create one job named -`cloudstack-ontap-presubmit` that loads CI from `*/feature/CSTACKEX-223`, using -[`CREATE-PRESUBMIT-JOB.md`](CREATE-PRESUBMIT-JOB.md). Its timer builds discover work and self-queue workers in the -same item. After merge, change only **Branches to build** to `*/main`. Keep the same job, GitHub App, credentials, -resources, and Check. Do not create a GitHub repository webhook: OpenLab Jenkins is not reachable from GitHub.com. -Do not create a GitHub Organization folder for this presubmit. +Production is the multibranch folder `cloudstack-ontap-presubmit-mb`. Remote Jenkinsfile +Provider loads this Jenkinsfile from `main` with Match branches off. A folder scan, +not a webhook, finds pull requests. Keep the old `cloudstack-ontap-presubmit` job +disabled. Do not create a GitHub repository webhook or a GitHub Organization folder. ### Manual-job trust boundary diff --git a/private-cicd/scripts/resolve-pull-request.py b/private-cicd/scripts/resolve-pull-request.py new file mode 100644 index 000000000000..9430b1ddea58 --- /dev/null +++ b/private-cicd/scripts/resolve-pull-request.py @@ -0,0 +1,133 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Decide whether a multibranch build may test one origin pull request.""" + +import argparse +import json +import os +import sys +import urllib.error +import urllib.request + + +API_ROOT = "https://api.github.com" + + +def parse_args(): + parser = argparse.ArgumentParser() + parser.add_argument("--repository", required=True) + parser.add_argument("--pr-id", required=True) + parser.add_argument("--indexed-sha", required=True) + parser.add_argument("--change-target", required=True) + parser.add_argument("--output", required=True) + return parser.parse_args() + + +def github_get(url, token): + request = urllib.request.Request( + url, + headers={ + "Accept": "application/vnd.github+json", + "Authorization": f"Bearer {token}", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "cloudstack-ontap-presubmit", + }, + ) + try: + with urllib.request.urlopen(request, timeout=60) as response: + return json.loads(response.read().decode("utf-8")) + except urllib.error.HTTPError as error: + detail = error.read().decode("utf-8", errors="replace") + raise SystemExit(f"GitHub API {error.code} for {url}: {detail}") from error + + +def is_main(ref): + return ref in ("main", "refs/heads/main") + + +def assess(pull, pr_id, indexed_sha, change_target, expected_repository): + head = (pull or {}).get("head") or {} + base = (pull or {}).get("base") or {} + user = (pull or {}).get("user") or {} + repository = head.get("repo") or {} + decision = { + "eligible": False, + "reason": "", + "pr_id": str((pull or {}).get("number") or ""), + "pr_head_sha": head.get("sha") or "", + "pr_head_branch": head.get("ref") or "", + "pr_base_branch": base.get("ref") or "", + "pr_draft": bool((pull or {}).get("draft")), + "pr_title": (pull or {}).get("title") or "", + "pr_url": (pull or {}).get("html_url") or "", + "pr_author_login": user.get("login") or "", + "pr_author_email": user.get("email") or "", + "head_repository": repository.get("full_name") or "", + } + if not isinstance(pull, dict): + decision["reason"] = "GitHub pull request payload is missing." + elif decision["pr_id"] != str(pr_id): + decision["reason"] = "GitHub pull request number does not match the build." + elif decision["head_repository"] != expected_repository: + decision["reason"] = ( + f"head repository '{decision['head_repository']}' is not " + f"'{expected_repository}'." + ) + elif not is_main(decision["pr_base_branch"]) or not is_main(change_target): + decision["reason"] = ( + f"base branch is '{decision['pr_base_branch']}' and change target " + f"is '{change_target}'." + ) + elif decision["pr_draft"]: + decision["reason"] = "pull request is a draft." + elif decision["pr_head_sha"].lower() != (indexed_sha or "").lower(): + decision["reason"] = "indexed revision does not match the pull request head SHA." + else: + decision["eligible"] = True + return decision + + +def main(): + args = parse_args() + token = os.environ.get("GITHUB_ACCESS_TOKEN", "").strip() + if not token: + raise SystemExit("GITHUB_ACCESS_TOKEN is empty.") + if not args.pr_id.isdigit(): + raise SystemExit("PR id must be numeric.") + pull = github_get( + f"{API_ROOT}/repos/{args.repository}/pulls/{args.pr_id}", + token, + ) + decision = assess( + pull, + args.pr_id, + args.indexed_sha, + args.change_target, + args.repository, + ) + with open(args.output, "w", encoding="utf-8") as handle: + json.dump(decision, handle) + handle.write("\n") + if not decision["eligible"]: + print(decision["reason"], file=sys.stderr) + raise SystemExit(2) + + +if __name__ == "__main__": + main() diff --git a/private-cicd/scripts/test_resolve_pull_request.py b/private-cicd/scripts/test_resolve_pull_request.py new file mode 100644 index 000000000000..ca1a6b403e47 --- /dev/null +++ b/private-cicd/scripts/test_resolve_pull_request.py @@ -0,0 +1,82 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Unit tests for resolve-pull-request.py.""" + +import importlib.util +import unittest +from pathlib import Path + + +SCRIPT = Path(__file__).with_name("resolve-pull-request.py") +SPEC = importlib.util.spec_from_file_location("resolve_pull_request", SCRIPT) +MODULE = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(MODULE) + +SHA = "a" * 40 + + +def pull(number="98", draft=False, base="main", repo="NetApp/cloudstack", sha=SHA): + return { + "number": int(number), + "title": "Test", + "html_url": f"https://github.com/NetApp/cloudstack/pull/{number}", + "draft": draft, + "base": {"ref": base}, + "head": { + "ref": "feature/test", + "sha": sha, + "repo": {"full_name": repo} if repo is not None else None, + }, + "user": {"login": "tester", "email": None}, + } + + +class AssessPullRequestTest(unittest.TestCase): + def test_accepts_matching_origin_pr(self): + decision = MODULE.assess(pull(), "98", SHA, "main", "NetApp/cloudstack") + + self.assertTrue(decision["eligible"]) + self.assertEqual("", decision["reason"]) + self.assertEqual("tester", decision["pr_author_login"]) + self.assertEqual("", decision["pr_author_email"]) + + def test_rejects_draft_fork_base_and_sha_mismatch(self): + cases = [ + pull(draft=True), + pull(repo="other/cloudstack"), + pull(repo=None), + pull(base="release"), + pull(sha="b" * 40), + ] + + for candidate in cases: + decision = MODULE.assess( + candidate, "98", SHA, "main", "NetApp/cloudstack") + self.assertFalse(decision["eligible"], candidate) + + def test_rejects_change_target_other_than_main(self): + decision = MODULE.assess( + pull(), "98", SHA, "feature/other", "NetApp/cloudstack") + + self.assertFalse(decision["eligible"]) + self.assertIn("change target", decision["reason"]) + + +if __name__ == "__main__": + unittest.main() From 72bcea35c7b412810218eda3a365a5bb4156ac4b Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 09:18:59 +0530 Subject: [PATCH 02/10] Check CI scripts out of the PR branch for the presubmit pilot. The new pull-request script is not on main yet, so the PR-119 trial has to load it from feature/CSTACKEX-340. Change this back to main before the folder builds every pull request. --- private-cicd/Jenkinsfile | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 396154123ba6..92b52c57809a 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -930,14 +930,16 @@ void abortSupersededRun() { void checkoutCiScripts() { recordStageEvent('Checkout CI scripts', 'STARTED') + // Pilot for PR-119. Change this back to main before the folder includes every PR. + def ciBranch = 'feature/CSTACKEX-340' if (env.PRESUBMIT_CI_SHA?.trim() && fileExists('private-cicd/scripts/resolve-pull-request.py')) { - echo "CI scripts already checked out from main at ${env.PRESUBMIT_CI_SHA}." + echo "CI scripts already checked out from ${ciBranch} at ${env.PRESUBMIT_CI_SHA}." return } deleteDir() def remote = [ url: params.CLOUDSTACK_GIT_URL, - refspec: '+refs/heads/main:refs/remotes/origin/main' + refspec: "+refs/heads/${ciBranch}:refs/remotes/origin/${ciBranch}" ] def credentialId = params.GIT_CREDENTIALS_ID?.trim() if (!credentialId) { @@ -956,7 +958,7 @@ void checkoutCiScripts() { try { checkout([ $class: 'GitSCM', - branches: [[name: '*/main']], + branches: [[name: "*/${ciBranch}"]], doGenerateSubmoduleConfigurations: false, extensions: cloneOptions + [[ $class: 'SparseCheckoutPaths', @@ -965,18 +967,18 @@ void checkoutCiScripts() { userRemoteConfigs: [remote] ]) } catch (Exception e) { - echo "Sparse CI checkout unavailable, using a full main checkout: ${e}" + echo "Sparse CI checkout unavailable, using a full ${ciBranch} checkout: ${e}" deleteDir() checkout([ $class: 'GitSCM', - branches: [[name: '*/main']], + branches: [[name: "*/${ciBranch}"]], doGenerateSubmoduleConfigurations: false, extensions: cloneOptions, userRemoteConfigs: [remote] ]) } env.PRESUBMIT_CI_SHA = sh(script: 'git rev-parse HEAD', returnStdout: true).trim() - echo "Checked out CI scripts from main at ${env.PRESUBMIT_CI_SHA}." + echo "Checked out CI scripts from ${ciBranch} at ${env.PRESUBMIT_CI_SHA}." } void prepareMultibranchPullRequest() { From 733a0ed8863a1c3298ac61b72d9c057e1b8dde6b Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 10:25:14 +0530 Subject: [PATCH 03/10] Look up the indexed pull request commit without opening the raw build. Jenkins warns that approving RunWrapper.getRawBuild lets a pipeline step outside the script sandbox. The existing job lookup already used for older builds can read the same revision. --- private-cicd/Jenkinsfile | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 92b52c57809a..cd90f072d48c 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -163,7 +163,9 @@ String revisionHeadSha(revision) { } @NonCPS -String indexedHeadSha(build) { +String indexedHeadSha(String jobName, int buildNumber) { + def job = jenkins.model.Jenkins.get().getItemByFullName(jobName) + def build = job?.getBuildByNumber(buildNumber) return revisionHeadSha(build?.getAction(jenkins.scm.api.SCMRevisionAction)?.revision) } @@ -983,7 +985,7 @@ void checkoutCiScripts() { void prepareMultibranchPullRequest() { checkoutCiScripts() - def indexed = indexedHeadSha(currentBuild.rawBuild) + def indexed = indexedHeadSha(env.JOB_NAME, env.BUILD_NUMBER.toInteger()) if (!(indexed ==~ /^[0-9a-fA-F]{40}$/)) { currentBuild.result = 'NOT_BUILT' echo "Not building PR-${env.CHANGE_ID}: Jenkins did not index a 40-character pull request head SHA." From 24fbfa54d38e724be46e73a1fd086c03c0d22a4e Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 10:49:34 +0530 Subject: [PATCH 04/10] Read the indexed pull request commit from the multibranch job. A manual build does not copy the scan revision onto the build, so the prepare stage treated a real PR as unindexed. --- private-cicd/Jenkinsfile | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index cd90f072d48c..37341030941c 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -165,7 +165,15 @@ String revisionHeadSha(revision) { @NonCPS String indexedHeadSha(String jobName, int buildNumber) { def job = jenkins.model.Jenkins.get().getItemByFullName(jobName) - def build = job?.getBuildByNumber(buildNumber) + if (job == null) { + return '' + } + // A manual build does not copy the indexed revision onto the build. The scan stores it on the job. + def hash = revisionHeadSha(job.getAction(jenkins.scm.api.SCMRevisionAction)?.revision) + if (hash) { + return hash + } + def build = job.getBuildByNumber(buildNumber) return revisionHeadSha(build?.getAction(jenkins.scm.api.SCMRevisionAction)?.revision) } From bcd52efcfe7df8426bb01be312b803f4162d9fd4 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 10:59:32 +0530 Subject: [PATCH 05/10] Use the GitHub pull request head when Jenkins exposes no indexed SHA. The multibranch job never attaches a pull request revision, so refusing the build there skipped the GitHub eligibility check. --- private-cicd/Jenkinsfile | 5 ++--- private-cicd/scripts/resolve-pull-request.py | 4 +++- private-cicd/scripts/test_resolve_pull_request.py | 6 ++++++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 37341030941c..13ba47909786 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -995,9 +995,8 @@ void prepareMultibranchPullRequest() { checkoutCiScripts() def indexed = indexedHeadSha(env.JOB_NAME, env.BUILD_NUMBER.toInteger()) if (!(indexed ==~ /^[0-9a-fA-F]{40}$/)) { - currentBuild.result = 'NOT_BUILT' - echo "Not building PR-${env.CHANGE_ID}: Jenkins did not index a 40-character pull request head SHA." - return + indexed = '' + echo "Jenkins did not expose an indexed head SHA for PR-${env.CHANGE_ID}. Using the GitHub pull request head." } def status = 1 withCredentials([ diff --git a/private-cicd/scripts/resolve-pull-request.py b/private-cicd/scripts/resolve-pull-request.py index 9430b1ddea58..01f90de0a50a 100644 --- a/private-cicd/scripts/resolve-pull-request.py +++ b/private-cicd/scripts/resolve-pull-request.py @@ -96,7 +96,9 @@ def assess(pull, pr_id, indexed_sha, change_target, expected_repository): ) elif decision["pr_draft"]: decision["reason"] = "pull request is a draft." - elif decision["pr_head_sha"].lower() != (indexed_sha or "").lower(): + elif not decision["pr_head_sha"] or len(decision["pr_head_sha"]) != 40: + decision["reason"] = "GitHub pull request head SHA is missing." + elif (indexed_sha or "").strip() and decision["pr_head_sha"].lower() != indexed_sha.strip().lower(): decision["reason"] = "indexed revision does not match the pull request head SHA." else: decision["eligible"] = True diff --git a/private-cicd/scripts/test_resolve_pull_request.py b/private-cicd/scripts/test_resolve_pull_request.py index ca1a6b403e47..739fe10a89e0 100644 --- a/private-cicd/scripts/test_resolve_pull_request.py +++ b/private-cicd/scripts/test_resolve_pull_request.py @@ -70,6 +70,12 @@ def test_rejects_draft_fork_base_and_sha_mismatch(self): candidate, "98", SHA, "main", "NetApp/cloudstack") self.assertFalse(decision["eligible"], candidate) + def test_accepts_github_head_when_jenkins_sha_is_missing(self): + decision = MODULE.assess(pull(), "98", "", "main", "NetApp/cloudstack") + + self.assertTrue(decision["eligible"]) + self.assertEqual(SHA, decision["pr_head_sha"]) + def test_rejects_change_target_other_than_main(self): decision = MODULE.assess( pull(), "98", SHA, "feature/other", "NetApp/cloudstack") From a7efdbc17989e4ea84c7a9c8c590c673f5a601e2 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 22:09:33 +0530 Subject: [PATCH 06/10] Fix multibranch source identity and document the PR-119 pilot setup. The environment block declared RESOLVED_SOURCE_ID and RESOLVED_SOURCE_SHA from build parameters. A multibranch pull request has no such parameters, and the block wraps every stage, so it shadowed the values set in prepareMultibranchPullRequest(). They read back empty, which caused a NullPointerException in presubmitSubjectBase and an empty SHA in the Maven and deploy shell steps. Set them in resolveSourceIdentity() instead. Also name the trusted CI branch in one place, trustedCiBranch(), and describe the multibranch folder configuration for the pilot. Co-Authored-By: Claude Sonnet 5.5 --- private-cicd/Jenkinsfile | 38 ++++++++++++++++---- private-cicd/docs/CREATE-PRESUBMIT-JOB.md | 44 ++++++++++++++++++----- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 13ba47909786..24f8c173a48a 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -796,7 +796,26 @@ void discoverPullRequests() { } } +// Branch mode leaves PR_ID and PR_HEAD_SHA empty, so scripts receive a +// mode-independent identity. The branch is sanitised because it reaches +// a filesystem path on the target VM. A multibranch pull request has already +// set both values in prepareMultibranchPullRequest(). +void resolveSourceIdentity() { + if (isMultibranchPullRequest()) { + return + } + if (params.SOURCE_MODE == 'branch') { + env.RESOLVED_SOURCE_ID = + 'branch-' + params.SOURCE_BRANCH.replaceAll('[^A-Za-z0-9._-]', '-') + env.RESOLVED_SOURCE_SHA = params.SOURCE_SHA + } else { + env.RESOLVED_SOURCE_ID = 'pr-' + params.PR_ID + env.RESOLVED_SOURCE_SHA = params.PR_HEAD_SHA + } +} + void validateSourceRequest() { + resolveSourceIdentity() sh 'rm -f "${WORKSPACE}@tmp/presubmit-stage-events.tsv"' env.PRESUBMIT_INTEGRATION_STARTED = 'false' env.PRESUBMIT_STARTED_AT = new Date().format( @@ -938,10 +957,16 @@ void abortSupersededRun() { } } +// Branch that supplies the trusted CI scripts. It must equal the branch that +// Remote Jenkinsfile Provider loads this Jenkinsfile from. PR-119 pilot value; +// change it to 'main' when the folder is cut over to every pull request. +String trustedCiBranch() { + return 'feature/CSTACKEX-340' +} + void checkoutCiScripts() { recordStageEvent('Checkout CI scripts', 'STARTED') - // Pilot for PR-119. Change this back to main before the folder includes every PR. - def ciBranch = 'feature/CSTACKEX-340' + def ciBranch = trustedCiBranch() if (env.PRESUBMIT_CI_SHA?.trim() && fileExists('private-cicd/scripts/resolve-pull-request.py')) { echo "CI scripts already checked out from ${ciBranch} at ${env.PRESUBMIT_CI_SHA}." return @@ -1510,11 +1535,10 @@ spec: environment { SOURCE_DIR = 'cloudstack-src' PRESUBMIT_RESULTS = 'presubmit-results' - // Branch mode leaves PR_ID and PR_HEAD_SHA empty, so scripts receive a - // mode-independent identity. The branch is sanitised because it reaches - // a filesystem path on the target VM. - RESOLVED_SOURCE_ID = "${params.SOURCE_MODE == 'branch' ? 'branch-' + params.SOURCE_BRANCH.replaceAll('[^A-Za-z0-9._-]', '-') : 'pr-' + params.PR_ID}" - RESOLVED_SOURCE_SHA = "${params.SOURCE_MODE == 'branch' ? params.SOURCE_SHA : params.PR_HEAD_SHA}" + // RESOLVED_SOURCE_ID and RESOLVED_SOURCE_SHA are deliberately not + // declared here. This block wraps every stage in withEnv, so a value + // declared here shadows the multibranch assignment made later and + // reads back empty. See resolveSourceIdentity(). } stages { diff --git a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md index 9e1b46871e85..5d4cbc52ec0c 100644 --- a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md +++ b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md @@ -22,16 +22,44 @@ The current job is the multibranch folder `cloudstack-ontap-presubmit-mb`. It scans GitHub about once a minute and creates one child job for every open non-draft origin pull request into `main`. Remote Jenkinsfile Provider loads -`private-cicd/Jenkinsfile` from `main`. Match branches stays off. Draft pull -requests get no job. The folder include filter can be narrowed to one `PR-` -for the first run, then returned to `PR-*`. Do not add a webhook. - -The sections below describe the retired single Pipeline job and its five-minute -`SOURCE_MODE=discover` timer. Do not create that second job. Keep -`cloudstack-ontap-presubmit` disabled after the folder works. Do not delete the -GitHub App, credentials, or the VM lock until a multibranch pull request has +`private-cicd/Jenkinsfile` from the trusted CI branch. Match branches stays off. +Draft pull requests get no job. Do not add a webhook. + +During the PR-119 pilot the trusted CI branch is `feature/CSTACKEX-340` and the +folder include filter is `PR-119`. Both the Remote Jenkinsfile Provider branch +and `trustedCiBranch()` in the Jenkinsfile must name that branch. Loading `main` +before PR-119 is merged would run the old Jenkinsfile, which knows nothing about +`CHANGE_ID` and defaults to `SOURCE_MODE=discover`. At cutover, change the +provider branch and `trustedCiBranch()` to `main` and the filter to `PR-*`. + +The numbered sections below that create a single Pipeline job with a five-minute +`SOURCE_MODE=discover` timer (11 to 16) are retired. Sections on plugins, +credentials, the inventory, Lockable Resources, and the GitHub App still apply. +Keep `cloudstack-ontap-presubmit` disabled after the folder works. Do not delete +the GitHub App, credentials, or the VM lock until a multibranch pull request has passed. +## Multibranch folder configuration + +Open `cloudstack-ontap-presubmit-mb` > **Configure**: + +| Setting | Value | +| --- | --- | +| Branch Source | GitHub, credential `cloudstack-presubmit-github-app`, repository `NetApp/cloudstack` | +| Behaviours | Discover pull requests from origin only, strategy **The current pull request revision**; remove branch and fork discovery | +| Filter by name | `PR-119` for the pilot, then `PR-*` | +| Build Configuration | **by Remote Jenkinsfile Provider**, repository `https://github.com/NetApp/cloudstack.git`, branch `feature/CSTACKEX-340` (pilot) then `main`, file `private-cicd/Jenkinsfile`, **Match branches off** | +| Scan Repository Triggers | Periodically, 1 to 5 minutes | +| Orphaned items | Keep 14 days, 50 items | + +Run **Scan Repository Now** and expect one child, `PR-119`. The first build may +stop on Script Security. Approve only signatures requested by this Jenkinsfile +and rebuild. + +The single-branch Pipeline job is now only the manual job for +`SOURCE_MODE=branch` and `diagnose`. Point it at the same trusted branch, add no +triggers, and never run `SOURCE_MODE=discover`. + Do not create a GitHub repository webhook. OpenLab Jenkins is not reachable from GitHub.com, so inbound `pull_request` deliveries fail with `failed to connect to host`. That is expected. NetApp's From ed68d585417ec71ad110e1d742fbf38d69e8abcf Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 22:38:14 +0530 Subject: [PATCH 07/10] Retry failing unit tests twice in the presubmit Maven build. Build 8 of PR-119 failed on the upstream, timing-sensitive ConfigKeyScheduledExecutionWrapperTest, which counts scheduled ticks in a fixed window and jitters in the CPU-limited agent pod. Add an opt-in RERUN_FAILING_TESTS setting to mvn-full.sh that passes -Dsurefire.rerunFailingTestsCount, and set it to 2 in the Jenkinsfile. The default stays 0 for local runs. Co-Authored-By: Claude Sonnet 5.5 --- private-cicd/Jenkinsfile | 1 + private-cicd/scripts/mvn-full.sh | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 24f8c173a48a..f9610902023e 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -1218,6 +1218,7 @@ void runUnitTests() { SOURCE_SHA="$RESOLVED_SOURCE_SHA" \ ENABLE_SIMULATOR=false \ MAVEN_THREADS=1C \ + RERUN_FAILING_TESTS=2 \ MAVEN_OPTS="-Xms1g -Xmx8g" \ private-cicd/scripts/mvn-full.sh ''' diff --git a/private-cicd/scripts/mvn-full.sh b/private-cicd/scripts/mvn-full.sh index 7aa9821a1d24..605a71ee1ec6 100755 --- a/private-cicd/scripts/mvn-full.sh +++ b/private-cicd/scripts/mvn-full.sh @@ -25,7 +25,9 @@ # RESULT_DIR — if set, SOURCE_ID and SOURCE_SHA are required (Jenkins handoff); # PR_ID and PR_HEAD_SHA are accepted as fallbacks for pull-request builds. # Optional: SKIP_TESTS, ENABLE_NOREDIST, ENABLE_SIMULATOR (default true), -# MAVEN_THREADS (Jenkins uses 1C; empty disables -T; unset uses nproc). +# MAVEN_THREADS (Jenkins uses 1C; empty disables -T; unset uses nproc), +# RERUN_FAILING_TESTS (default 0; Jenkins uses 2 so a timing-flaky upstream +# test is retried before it fails the run). # Commands: mvn, java. Does not call other private-cicd scripts. # When RESULT_DIR is set, apply debian/rules packaging flags so build-debs.sh # can reuse target/ without a second mvn clean. @@ -43,6 +45,7 @@ SKIP_TESTS="${SKIP_TESTS:-false}" ENABLE_NOREDIST="${ENABLE_NOREDIST:-false}" ENABLE_SIMULATOR="${ENABLE_SIMULATOR:-true}" THREADS="${MAVEN_THREADS-$(nproc)}" +RERUN_FAILING_TESTS="${RERUN_FAILING_TESTS:-0}" RESULT_DIR="${RESULT_DIR:-}" mvn_args=(-B -P developer,systemvm) @@ -59,6 +62,9 @@ fi if [[ -n "$THREADS" ]]; then mvn_args+=(-T"$THREADS") fi +if [[ "$RERUN_FAILING_TESTS" != "0" ]]; then + mvn_args+=("-Dsurefire.rerunFailingTestsCount=$RERUN_FAILING_TESTS") +fi echo "==> Full build in $SOURCE_DIR" From 1d2679e37992dc127f8aa76439c48936bad42f53 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 23:37:20 +0530 Subject: [PATCH 08/10] Include the trusted ONTAP prerequisite checker in the CI sparse checkout. The multibranch CI checkout fetched only private-cicd, so Phase 2 stopped with "Trusted ONTAP prerequisite checker is missing" right after the snapshot revert. run-phase2-remote.sh runs the trusted copy of test/integration/plugins/ontap/check_ontap_prereqs.py from the CI checkout, so add that single file to the sparse paths. Co-Authored-By: Claude Sonnet 5.5 --- private-cicd/Jenkinsfile | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index f9610902023e..3b25df5e38a0 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -997,7 +997,12 @@ void checkoutCiScripts() { doGenerateSubmoduleConfigurations: false, extensions: cloneOptions + [[ $class: 'SparseCheckoutPaths', - sparseCheckoutPaths: [[path: 'private-cicd']] + // run-phase2-remote.sh runs the trusted copy of the ONTAP + // prerequisite checker from this checkout, never the PR's copy. + sparseCheckoutPaths: [ + [path: 'private-cicd'], + [path: 'test/integration/plugins/ontap/check_ontap_prereqs.py'] + ] ]], userRemoteConfigs: [remote] ]) From e7b0789f9392161a8f2b66fd5ac45bd245dc11c6 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 06:19:18 +0530 Subject: [PATCH 09/10] Show the exception on the failed test's HTML log page and in the mail. The HTML log link of a failed ONTAP test opened the suite stdout log, which only holds the pass/fail summary, so the reason for the failure was not visible. Parse the ERROR and FAIL blocks of the suite's results.txt, put them in a "Failure details" section above the suite log, and link a failed test straight to its block. Add an Error column to the mail table with a one-line reason, leading with the CloudStack job error text when there is one, and link the archived management and agent logs from the failure section. Co-Authored-By: Claude Sonnet 5.5 --- .../scripts/generate-presubmit-report.py | 152 +++++++++++++++- .../scripts/test_generate_presubmit_report.py | 162 ++++++++++++++++++ 2 files changed, 308 insertions(+), 6 deletions(-) create mode 100644 private-cicd/scripts/test_generate_presubmit_report.py diff --git a/private-cicd/scripts/generate-presubmit-report.py b/private-cicd/scripts/generate-presubmit-report.py index 3067cfa26724..d60c8c4b8f02 100644 --- a/private-cicd/scripts/generate-presubmit-report.py +++ b/private-cicd/scripts/generate-presubmit-report.py @@ -21,9 +21,11 @@ import argparse import html import json +import os import re import shutil from datetime import datetime, timezone +from functools import lru_cache from pathlib import Path from urllib.parse import quote @@ -121,7 +123,115 @@ def html_document(title, body): """ -def log_to_html(source, destination): +FAILURE_HEADER = re.compile(r"^(ERROR|FAIL):\s*(.*)$") +DESCRIBED_TEST = re.compile( + r"^(.*?)\s*\.\.\.\s*=== TestName:\s+(\S+)\s+\|\s+Status\s+:\s+(\S+)") +JOB_ERROR = re.compile( + r"errorcode\s*:\s*(\d+)\s*,\s*errortext\s*:\s*'(.*?)(?:'\s*[,}]|$)") +MAX_FAILURE_LINES = 80 +MAX_ERROR_CHARS = 400 + + +@lru_cache(maxsize=None) +def parse_suite_failures(results_txt): + """Return the ERROR and FAIL blocks of one Marvin suite results.txt. + + Marvin prints each block with the test's docstring as its title, while the + summary lines pair the same docstring with the test name, so the two are + joined on that text. A block that cannot be joined (for example a + setUpClass error) keeps an empty name list and is still shown. + """ + try: + text = ANSI_ESCAPE.sub( + "", results_txt.read_text(encoding="utf-8", errors="replace")) + except OSError: + return () + lines = text.splitlines() + names = {} + for line in lines: + described = DESCRIBED_TEST.match(line) + if described: + names.setdefault(described.group(1).strip(), []).append( + described.group(2)) + failures = [] + index = 0 + while index < len(lines): + header = FAILURE_HEADER.match(lines[index]) + if not (header and index > 0 and lines[index - 1].startswith("=====")): + index += 1 + continue + block = [] + index += 1 + while index < len(lines): + line = lines[index] + if (line.startswith("=====") or line.startswith("Ran ") + or ">> begin captured" in line): + break + block.append(line) + index += 1 + while block and (not block[0].strip() + or set(block[0].strip()) == {"-"}): + block.pop(0) + while block and (not block[-1].strip() + or set(block[-1].strip()) == {"-"}): + block.pop() + description = header.group(2).strip() + error = block[-1].strip() if block else "" + # A failed CloudStack async job prints a long object of account and + # domain IDs before the useful text, so lead with the job's own message. + job = JOB_ERROR.search(error) + if job: + error = f"Job failed (code {job.group(1)}): {job.group(2)}" + if len(error) > MAX_ERROR_CHARS: + error = error[:MAX_ERROR_CHARS] + "…" + failures.append({ + "kind": header.group(1), + "description": description, + "names": tuple(names.get(description, ())), + "lines": tuple(block[:MAX_FAILURE_LINES]), + "error": error, + }) + return tuple(failures) + + +def failure_anchor(test_name): + return f"fail-{slug(test_name)}" + + +def render_failures(failures, related=()): + if not failures: + return "" + sections = [] + if related: + links = " · ".join( + f'{html.escape(label)}' + for label, path in related) + sections.append( + "

The API error above is often only a symptom. The cause is " + f"usually in the CloudStack logs: {links}

") + for number, failure in enumerate(failures, 1): + names = failure["names"] or (f"unmapped-{number}",) + anchors = "".join( + f'' for name in names) + title = ", ".join(failure["names"]) or failure["description"] + sections.append( + f'{anchors}

' + f'{html.escape(failure["kind"])}: {html.escape(title)}

' + f'
'
+            f'{html.escape(chr(10).join(failure["lines"]))}
') + return "

Failure details

" + "".join(sections) + "

Suite log

" + + +def failure_for_test(summary_path, test): + suite_dir = summary_path.parent / "suites" / test.get("tag", "") + for failure in parse_suite_failures(suite_dir / "results.txt"): + if test.get("name") in failure["names"]: + return failure + return None + + +def log_to_html(source, destination, failures=(), related=()): lines = source.read_text(encoding="utf-8", errors="replace").splitlines() rendered = [] test_pattern = re.compile(r"TestName:\s+(\S+)\s+\|\s+Status\s+:\s+(\S+)") @@ -154,7 +264,8 @@ def log_to_html(source, destination): rendered.append(f"{anchor}{escaped}") body = ( f"

{html.escape(source.name)}

" - f'
'
         + "\n".join(rendered)
         + "
" @@ -175,6 +286,12 @@ def generate_log_artifacts(results, debian_log): results / "phase2" / "configure-cloudstack.log", results / "phase2" / "health-check.log", ] + runtime = results / "phase2" / "runtime-logs" + related_logs = ( + ("Management server log", + runtime / "var-log-cloudstack-management" / "management-server.log"), + ("Agent log", runtime / "var-log-cloudstack-agent" / "agent.log"), + ) marvin = results / "phase2" / "marvin" candidates.extend(marvin.glob("*.log")) candidates.extend( @@ -183,7 +300,15 @@ def generate_log_artifacts(results, debian_log): for source in sorted(set(candidates)): if not source.is_file(): continue - log_to_html(source, Path(f"{source}.html")) + failures = ( + parse_suite_failures(source.with_name("results.txt")) + if source.name == "stdout.log" else ()) + destination = Path(f"{source}.html") + related = tuple( + (label, os.path.relpath(path, destination.parent)) + for label, path in related_logs if path.is_file() + ) if failures else () + log_to_html(source, destination, failures, related) def parse_stage_events(stage_events, results): @@ -339,13 +464,26 @@ def test_rows(tests, results, build_url): for test in tests: status = display_test_status(test.get("status")) log_path = test_log_for_summary(test["_summary"], test) + failure = None + if status not in ("PASS", "SKIP"): + failure = failure_for_test(test["_summary"], test) if log_path: relative = log_path.relative_to(results.parent) - link = artifact_url( - build_url, relative, f"test-{slug(test.get('name', 'test'))}") + name = test.get("name", "test") + # The failure section sits at the top of the suite page, so a + # failed test links straight to its exception. + fragment = ( + failure_anchor(name) + if failure and log_path.name == "stdout.log.html" + else f"test-{slug(name)}") + link = artifact_url(build_url, relative, fragment) log_cell = f'HTML log' else: log_cell = "—" + error_cell = ( + f'' + f'{html.escape(failure["error"])}' + if failure and failure["error"] else "") suite = str(test.get("label") or test.get("tag") or "") rows.append( "" @@ -353,6 +491,7 @@ def test_rows(tests, results, build_url): f"{html.escape(suite)}" f"{html.escape(str(test.get('name', '')))}" f'{html.escape(status)}' + f"{error_cell}" f"{log_cell}" "") return "\n".join(rows) @@ -437,7 +576,8 @@ def main(): "

ONTAP tests

" "

Failures are listed first. Select a log link to jump to the " "test.

" - + table(["Protocol", "Suite", "Test", "Status", "Details"], tests_html) + + table(["Protocol", "Suite", "Test", "Status", "Error", "Details"], + tests_html) if tests else "

ONTAP tests

No Marvin test summary was available.

") body = ( diff --git a/private-cicd/scripts/test_generate_presubmit_report.py b/private-cicd/scripts/test_generate_presubmit_report.py new file mode 100644 index 000000000000..02dd29c95aa9 --- /dev/null +++ b/private-cicd/scripts/test_generate_presubmit_report.py @@ -0,0 +1,162 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import importlib.util +import tempfile +import unittest +from pathlib import Path + + +SPEC = importlib.util.spec_from_file_location( + "generate_presubmit_report", + Path(__file__).with_name("generate-presubmit-report.py")) +report = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(report) + +SEP = "=" * 70 +DASH = "-" * 70 +RESULTS = f"""Start the VM. ... === TestName: test_06_start | Status : EXCEPTION === +ERROR +Check the pool. ... === TestName: test_07_pool | Status : FAIL === +FAIL +Detach it. ... === TestName: test_08_detach | Status : SUCCESS === +ok + +{SEP} +ERROR: Start the VM. +{DASH} +Traceback (most recent call last): + File "test.py", line 750, in test_06_start + self.apiClient.startVirtualMachine(cmd) +Exception: Job failed: {{errortext : 'Unable to orchestrate the start of VM'}} +-------------------- >> begin captured logging << -------------------- +captured line that must not be shown +--------------------- >> end captured logging << --------------------- + +{SEP} +FAIL: Check the pool. +{DASH} +Traceback (most recent call last): + File "test.py", line 90, in test_07_pool + self.assertEqual(a, b) +AssertionError: 1 != 2 +{DASH} +Ran 3 tests in 1.0s + +FAILED (errors=1, failures=1) +""" +SETUP_ERROR = f"""{SEP} +ERROR: setUpClass (suite.Class) +{DASH} +Traceback (most recent call last): +RuntimeError: pool create failed +{DASH} +Ran 0 tests in 0.1s +""" + + +class ParseSuiteFailuresTest(unittest.TestCase): + def parse(self, text): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "results.txt" + path.write_text(text, encoding="utf-8") + return report.parse_suite_failures(path) + + def test_error_and_fail_blocks_are_mapped_to_test_names(self): + failures = self.parse(RESULTS) + self.assertEqual([f["kind"] for f in failures], ["ERROR", "FAIL"]) + self.assertEqual(failures[0]["names"], ("test_06_start",)) + self.assertEqual(failures[1]["names"], ("test_07_pool",)) + + def test_error_is_the_exception_line_without_captured_logging(self): + failures = self.parse(RESULTS) + self.assertTrue(failures[0]["error"].startswith("Exception: Job failed")) + self.assertIn("Unable to orchestrate", failures[0]["error"]) + self.assertEqual(failures[1]["error"], "AssertionError: 1 != 2") + self.assertNotIn( + "must not be shown", "\n".join(failures[0]["lines"])) + + def test_cloudstack_job_failure_leads_with_its_message(self): + line = ("Exception: Job failed: {accountid : 'a', account : 'admin', " + "jobresult : {errorcode : 530, errortext : 'Unable to start " + "the VM'}}") + text = f"{SEP}\nERROR: d\n{DASH}\n{line}\n" + self.assertEqual( + self.parse(text)[0]["error"], + "Job failed (code 530): Unable to start the VM") + + def test_unmapped_block_is_kept_without_names(self): + failures = self.parse(SETUP_ERROR) + self.assertEqual(len(failures), 1) + self.assertEqual(failures[0]["names"], ()) + self.assertEqual(failures[0]["error"], "RuntimeError: pool create failed") + + def test_missing_file_or_clean_run_gives_no_failures(self): + self.assertEqual( + report.parse_suite_failures(Path("/nonexistent/results.txt")), ()) + self.assertEqual(self.parse("Detach. ... === TestName: t | Status : SUCCESS ===\nok\n"), ()) + + def test_long_error_is_truncated(self): + long_line = "Exception: " + "x" * 2000 + text = f"{SEP}\nERROR: d\n{DASH}\n{long_line}\n" + error = self.parse(text)[0]["error"] + self.assertEqual(len(error), report.MAX_ERROR_CHARS + 1) + + +class FailurePageTest(unittest.TestCase): + def test_suite_page_shows_failure_before_the_log(self): + with tempfile.TemporaryDirectory() as directory: + suite = (Path(directory) / "phase2" / "marvin" / "ontap-results" + / "run-iscsi" / "suites" / "vm") + suite.mkdir(parents=True) + (suite / "results.txt").write_text(RESULTS, encoding="utf-8") + log = suite / "stdout.log" + log.write_text( + "=== TestName: test_06_start | Status : EXCEPTION ===\n" + "=== TestName: test_08_detach | Status : SUCCESS ===\n", + encoding="utf-8") + management = (Path(directory) / "phase2" / "runtime-logs" + / "var-log-cloudstack-management") + management.mkdir(parents=True) + (management / "management-server.log").write_text("log\n") + report.generate_log_artifacts(Path(directory), None) + page = (suite / "stdout.log.html").read_text(encoding="utf-8") + self.assertIn('id="fail-test_06_start"', page) + self.assertIn("Unable to orchestrate", page) + self.assertIn('id="test-test_08_detach"', page) + self.assertIn("Management server log", page) + self.assertIn( + "../../../../../runtime-logs/var-log-cloudstack-management/" + "management-server.log", page) + self.assertNotIn("Agent log", page) + self.assertLess(page.index("Failure details"), page.index("Suite log")) + self.assertLess(page.index("Suite log"), page.index("TestName")) + + def test_clean_suite_page_has_no_failure_section(self): + with tempfile.TemporaryDirectory() as directory: + log = Path(directory) / "stdout.log" + log.write_text("=== TestName: t | Status : SUCCESS ===\n", + encoding="utf-8") + destination = Path(directory) / "stdout.log.html" + report.log_to_html(log, destination) + self.assertNotIn( + "Failure details", destination.read_text(encoding="utf-8")) + + +if __name__ == "__main__": + unittest.main() From 8fee48b57e2be49e421382632e195c75c3478082 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 06:25:18 +0530 Subject: [PATCH 10/10] Make the ONTAP VM tests tolerate a briefly unreachable virtual router. test_06_start_vm_lun_remapped failed once with "Unable to apply dhcp entry on router": the guest network's router had reported failed health checks about a minute earlier, and the test started the VM with no check and no retry. Add to OntapTestBase a bounded router readiness gate, a deploy wrapper that waits before and after the deploy (the first deploy creates the router), and a start wrapper that retries a VM that rolled back to Stopped, at most three attempts, and raises every attempt's error if they all fail. A single wait timeout makes the class stop waiting, so a permanently flagged router cannot slow every VM operation. Use the wrappers in the iSCSI and NFS3 VM tests. Co-Authored-By: Claude Sonnet 5.5 --- .../iscsi/instance/test_vm_volume_attach.py | 9 +- .../nfs3/instance/test_vm_volume_attach.py | 9 +- .../plugins/ontap/ontap_test_base.py | 120 ++++++++++++++++++ 3 files changed, 126 insertions(+), 12 deletions(-) diff --git a/test/integration/plugins/ontap/iscsi/instance/test_vm_volume_attach.py b/test/integration/plugins/ontap/iscsi/instance/test_vm_volume_attach.py index 2e247c5e2f87..6646b47f6076 100644 --- a/test/integration/plugins/ontap/iscsi/instance/test_vm_volume_attach.py +++ b/test/integration/plugins/ontap/iscsi/instance/test_vm_volume_attach.py @@ -93,7 +93,6 @@ listNetworks as listNetworksAPI, listVirtualMachines as listVirtualMachinesAPI, listVolumes as listVolumesAPI, - startVirtualMachine as startVirtualMachineAPI, stopVirtualMachine as stopVirtualMachineAPI, ) from marvin.lib.base import ServiceOffering, StoragePool @@ -551,7 +550,7 @@ def test_03_deploy_vm(self): if self.__class__.network_id: cmd.networkids = self.__class__.network_id - vm = self.apiClient.deployVirtualMachine(cmd) + vm = self._deploy_vm(cmd) self.__class__.vm = vm result = self._poll_vm_state(vm.id, "Running") @@ -602,7 +601,7 @@ def test_03a_deploy_second_vm_reuses_template_cache(self): if self.__class__.network_id: cmd.networkids = self.__class__.network_id - vm2 = self.apiClient.deployVirtualMachine(cmd) + vm2 = self._deploy_vm(cmd) self.assertIsNotNone(vm2, "deployVirtualMachine returned None") self.__class__.vm2 = vm2 self._poll_vm_state(vm2.id, "Running") @@ -745,9 +744,7 @@ def test_06_start_vm_lun_remapped(self): if self.__class__.vm is None: self.skipTest("VM not deployed — test_03 was skipped") - cmd = startVirtualMachineAPI.startVirtualMachineCmd() - cmd.id = self.__class__.vm.id - self.apiClient.startVirtualMachine(cmd) + self._start_vm(self.__class__.vm.id) result = self._poll_vm_state(self.__class__.vm.id, "Running", timeout=300) diff --git a/test/integration/plugins/ontap/nfs3/instance/test_vm_volume_attach.py b/test/integration/plugins/ontap/nfs3/instance/test_vm_volume_attach.py index dbd6b210ce85..6581c80ad94e 100644 --- a/test/integration/plugins/ontap/nfs3/instance/test_vm_volume_attach.py +++ b/test/integration/plugins/ontap/nfs3/instance/test_vm_volume_attach.py @@ -78,7 +78,6 @@ listNetworks as listNetworksAPI, listVirtualMachines as listVirtualMachinesAPI, listVolumes as listVolumesAPI, - startVirtualMachine as startVirtualMachineAPI, stopVirtualMachine as stopVirtualMachineAPI, updateStoragePool as updateStoragePoolAPI, ) @@ -483,7 +482,7 @@ def test_03_deploy_vm(self): if self.__class__.network_id: cmd.networkids = self.__class__.network_id - vm = self.apiClient.deployVirtualMachine(cmd) + vm = self._deploy_vm(cmd) self.assertIsNotNone(vm, "deployVirtualMachine returned None") self.__class__.vm = vm @@ -523,7 +522,7 @@ def test_03a_deploy_second_vm_reuses_template_cache(self): if self.__class__.network_id: cmd.networkids = self.__class__.network_id - vm2 = self.apiClient.deployVirtualMachine(cmd) + vm2 = self._deploy_vm(cmd) self.assertIsNotNone(vm2, "deployVirtualMachine returned None") self.__class__.vm2 = vm2 self._poll_vm_state(vm2.id, "Running") @@ -709,9 +708,7 @@ def test_06_start_vm_volume_accessible(self): vm = self.__class__.vm vol = self.__class__.volume - cmd = startVirtualMachineAPI.startVirtualMachineCmd() - cmd.id = vm.id - self.apiClient.startVirtualMachine(cmd) + self._start_vm(vm.id) result = self._poll_vm_state(vm.id, "Running", timeout=300) self.assertEqual( diff --git a/test/integration/plugins/ontap/ontap_test_base.py b/test/integration/plugins/ontap/ontap_test_base.py index 17bec2c1251c..89b4b57c1dfc 100644 --- a/test/integration/plugins/ontap/ontap_test_base.py +++ b/test/integration/plugins/ontap/ontap_test_base.py @@ -41,10 +41,13 @@ createVolume as createVolumeAPI, deleteStoragePool as deleteStoragePoolAPI, deleteVolume as deleteVolumeAPI, + deployVirtualMachine as deployVirtualMachineAPI, enableStorageMaintenance, listDiskOfferings as listDiskOfferingsAPI, + listRouters as listRoutersAPI, listVirtualMachines as listVirtualMachinesAPI, listVolumes as listVolumesAPI, + startVirtualMachine as startVirtualMachineAPI, updateStoragePool as updateStoragePoolAPI, ) from marvin.cloudstackAPI import listHosts as listHostsAPI @@ -468,6 +471,17 @@ class OntapTestBase(cloudstackTestCase): # VM states from which the VM will never reach any other target state. _VM_TERMINAL_STATES = ("error", "destroyed", "expunging") + # The guest network's virtual router is created by the first VM deploy + # and can be briefly unreachable afterwards, which makes a VM start fail + # with "Unable to apply dhcp entry on router". Every wait and retry below + # is bounded so a genuine failure is reported, never waited out forever. + ROUTER_HEALTHY_TIMEOUT = 180 # seconds, per wait + ROUTER_POLL_INTERVAL = 10 # seconds + VM_START_ATTEMPTS = 3 # total tries, including the first + VM_START_RETRY_DELAY = 15 # seconds between tries + # Set once a router wait times out; see _wait_for_routers_healthy(). + _router_wait_exhausted = False + # ---- zone guard ---------------------------------------------------- @classmethod @@ -768,6 +782,112 @@ def _poll_pool_state(self, pool_id, target_state, timeout=120, interval=5): % (pool_id, target_state, timeout, current_state) ) + # ---- virtual router readiness and VM operations -------------------- + + def _unhealthy_routers(self): + """Describe the test account's routers that are not ready. + + A router is not ready when it is not Running or reports failed health + checks. Returns [] when every router is ready, or when none exists + yet (the first VM deploy creates it). + """ + cmd = listRoutersAPI.listRoutersCmd() + cmd.listall = True + if self.zone is not None: + cmd.zoneid = self.zone.id + if self.account is not None: + cmd.account = self.account.name + cmd.domainid = self.account.domainid + unready = [] + for router in self.apiClient.listRouters(cmd) or []: + state = getattr(router, "state", "") or "" + failed = bool(getattr(router, "healthchecksfailed", False)) + if state.lower() != "running" or failed: + unready.append( + "%s (state=%s, healthchecksfailed=%s)" + % (getattr(router, "name", "router"), state, failed)) + return unready + + def _wait_for_routers_healthy(self, timeout=None): + """Wait, for a bounded time, until the guest router is ready. + + Returns True when ready (or when readiness cannot be determined) and + False when it is still not ready after the timeout. It never raises: + a router that stays flagged is reported in the log, and the VM + operation that follows then decides the test. After one timeout the + class stops waiting, so a router with a permanent health warning + costs one wait, not one per VM operation. + """ + timeout = self.ROUTER_HEALTHY_TIMEOUT if timeout is None else timeout + if self.__class__._router_wait_exhausted: + timeout = 0 + deadline = time.time() + timeout + while True: + try: + unready = self._unhealthy_routers() + except Exception as error: + log_progress( + logger, "warning", + "Cannot read router health, continuing: %s", error) + return True + if not unready: + return True + if time.time() >= deadline: + break + log_progress( + logger, "info", "Waiting for router to be ready: %s", + "; ".join(unready)) + time.sleep(self.ROUTER_POLL_INTERVAL) + if timeout: + self.__class__._router_wait_exhausted = True + log_progress( + logger, "warning", + "Router still not ready after %ss: %s", timeout, + "; ".join(unready)) + return False + + def _deploy_vm(self, cmd): + """Deploy a VM once the router is ready, and let a new one settle.""" + self._wait_for_routers_healthy() + vm = self.apiClient.deployVirtualMachine(cmd) + # The first deploy creates the guest network's router. + self._wait_for_routers_healthy() + return vm + + def _start_vm(self, vm_id): + """Start a stopped VM, retrying a bounded number of times. + + A start can fail only because the router was briefly unreachable + ("Unable to apply dhcp entry on router"); CloudStack then rolls the + VM back to Stopped. That, and only that, is retried. A VM in any + other state, or the last attempt failing, raises with every attempt's + error so the cause is not hidden. + """ + cmd = startVirtualMachineAPI.startVirtualMachineCmd() + cmd.id = vm_id + attempts = self.VM_START_ATTEMPTS + errors = [] + for attempt in range(1, attempts + 1): + self._wait_for_routers_healthy() + try: + return self.apiClient.startVirtualMachine(cmd) + except Exception as error: + errors.append("attempt %d/%d: %s" % (attempt, attempts, error)) + vms = self.apiClient.listVirtualMachines( + _list_vms_cmd(vm_id)) or [] + state = (vms[0].state if vms else "") or "" + if attempt == attempts or state.lower() != "stopped": + raise Exception( + "startVirtualMachine failed after %d attempt(s), " + "VM state '%s':\n%s" + % (attempt, state, "\n".join(errors))) from error + log_progress( + logger, "warning", + "startVirtualMachine attempt %d/%d failed, retrying in " + "%ss: %s", attempt, attempts, self.VM_START_RETRY_DELAY, + error) + time.sleep(self.VM_START_RETRY_DELAY) + def _poll_vm_state(self, vm_id, target_state, timeout=300, interval=10): """ Poll listVirtualMachines until the VM reaches target_state.