From 1c4f49a948d9d05160087cdf671cbd9f5641954d Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 08:59:27 +0530 Subject: [PATCH 01/19] 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 18fc9053d38a..01a362bf9cc0 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 54d4dae56a6f..bfc5edb4478d 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 30a0c7aad644359dbb5654d5a45089ac7b521590 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 09:18:59 +0530 Subject: [PATCH 02/19] 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 961596cc0f177a736debc3f97a4202d95ba2f725 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 10:25:14 +0530 Subject: [PATCH 03/19] 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 b8bbb2f78caa34dbcb1a8c87275c956721dcfc19 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 10:49:34 +0530 Subject: [PATCH 04/19] 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 e6bec6f88abdecc9d6e649e1589f37f9b098bee1 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 10:59:32 +0530 Subject: [PATCH 05/19] 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 5913e66a73d4af1177bbccc14e9c4511f4a3dc3a Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 22:09:33 +0530 Subject: [PATCH 06/19] 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. --- 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 01a362bf9cc0..68d6c77ef49d 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 4c85cd0fb4f1d141ef3fe36639c1faa5957b1198 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 22:38:14 +0530 Subject: [PATCH 07/19] 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. --- 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 07a8f1291bfb9df770ed8521cd7ea301f473095a Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 23:37:20 +0530 Subject: [PATCH 08/19] 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. --- 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 53afe82537bf19df2786fb29181e2e010b037af4 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 06:19:18 +0530 Subject: [PATCH 09/19] 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. --- .../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 a382d5735716dbe6cd580c2d24a6ca271284292a Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 06:25:18 +0530 Subject: [PATCH 10/19] 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. --- .../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 ab46b96e6a3b..4afca614c7b9 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 @@ -125,7 +125,6 @@ listNetworks as listNetworksAPI, listVirtualMachines as listVirtualMachinesAPI, listVolumes as listVolumesAPI, - startVirtualMachine as startVirtualMachineAPI, stopVirtualMachine as stopVirtualMachineAPI, ) from marvin.lib.base import ServiceOffering, Snapshot, StoragePool, VmSnapshot, Volume @@ -1043,7 +1042,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") @@ -1094,7 +1093,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") @@ -1336,9 +1335,7 @@ def test_08_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 d3450949baf2..d8e1fd9f75f2 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 @@ -110,7 +110,6 @@ listNetworks as listNetworksAPI, listVirtualMachines as listVirtualMachinesAPI, listVolumes as listVolumesAPI, - startVirtualMachine as startVirtualMachineAPI, stopVirtualMachine as stopVirtualMachineAPI, updateStoragePool as updateStoragePoolAPI, ) @@ -872,7 +871,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 @@ -912,7 +911,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") @@ -1213,9 +1212,7 @@ def test_08_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 e528aba2b25f..261de3d9a2bf 100644 --- a/test/integration/plugins/ontap/ontap_test_base.py +++ b/test/integration/plugins/ontap/ontap_test_base.py @@ -46,6 +46,7 @@ deleteNetwork as deleteNetworkAPI, deleteStoragePool as deleteStoragePoolAPI, deleteVolume as deleteVolumeAPI, + deployVirtualMachine as deployVirtualMachineAPI, destroyVirtualMachine as destroyVirtualMachineAPI, destroyVolume as destroyVolumeAPI, detachVolume as detachVolumeAPI, @@ -53,10 +54,12 @@ listDiskOfferings as listDiskOfferingsAPI, listNetworkOfferings as listNetworkOfferingsAPI, listNetworks as listNetworksAPI, + listRouters as listRoutersAPI, listServiceOfferings as listServiceOfferingsAPI, listTemplates as listTemplatesAPI, listVirtualMachines as listVirtualMachinesAPI, listVolumes as listVolumesAPI, + startVirtualMachine as startVirtualMachineAPI, stopVirtualMachine as stopVirtualMachineAPI, resizeVolume as resizeVolumeAPI, updateStoragePool as updateStoragePoolAPI, @@ -897,6 +900,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 @@ -1618,6 +1632,112 @@ def _other_ontap_pools_on_svm(self, current_pool_id): others.append(pool) return others + # ---- 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. From 05805537cddd271c5cdf8aef1e9cb75cf400fe7c Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 07:50:00 +0530 Subject: [PATCH 11/19] Show presubmit times in IST in the mail and report. The stage table, the report's Started and Finished rows and the start mail showed UTC. Display them in IST (UTC+5:30). The stored timestamps and the stage durations stay UTC, so the arithmetic is unchanged. --- private-cicd/Jenkinsfile | 12 ++++++++++-- .../scripts/generate-presubmit-report.py | 19 ++++++++++++++----- .../scripts/test_generate_presubmit_report.py | 18 ++++++++++++++++++ 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 3b25df5e38a0..2a899c3da655 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -633,7 +633,8 @@ String presubmitStartHtml() { } rows += presubmitInfoRow( 'Commit', "${htmlEscape(env.RESOLVED_SOURCE_SHA)}") - rows += presubmitInfoRow('Started', htmlEscape(env.PRESUBMIT_STARTED_AT)) + rows += presubmitInfoRow( + 'Started', htmlEscape(env.PRESUBMIT_STARTED_AT_IST ?: env.PRESUBMIT_STARTED_AT)) def consoleUrl = "${env.BUILD_URL}console" return """

CloudStack ONTAP presubmit

@@ -818,10 +819,17 @@ void validateSourceRequest() { resolveSourceIdentity() sh 'rm -f "${WORKSPACE}@tmp/presubmit-stage-events.tsv"' env.PRESUBMIT_INTEGRATION_STARTED = 'false' - env.PRESUBMIT_STARTED_AT = new Date().format( + def startedAt = new Date() + // The UTC value feeds the report script and durations. The IST value is + // only what the start mail displays. + env.PRESUBMIT_STARTED_AT = startedAt.format( "yyyy-MM-dd'T'HH:mm:ssXXX", TimeZone.getTimeZone('UTC') ) + env.PRESUBMIT_STARTED_AT_IST = startedAt.format( + "yyyy-MM-dd HH:mm:ss 'IST'", + TimeZone.getTimeZone('Asia/Kolkata') + ) recordStageEvent('Validate source request', 'STARTED') if (!(sourceValue('SOURCE_MODE') in ['pull_request', 'branch'])) { error("Unsupported SOURCE_MODE '${sourceValue('SOURCE_MODE')}'.") diff --git a/private-cicd/scripts/generate-presubmit-report.py b/private-cicd/scripts/generate-presubmit-report.py index d60c8c4b8f02..dcafbb8f43d8 100644 --- a/private-cicd/scripts/generate-presubmit-report.py +++ b/private-cicd/scripts/generate-presubmit-report.py @@ -24,7 +24,7 @@ import os import re import shutil -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone from functools import lru_cache from pathlib import Path from urllib.parse import quote @@ -73,6 +73,16 @@ def parse_time(value): return datetime.fromisoformat(value.replace("Z", "+00:00")) +IST = timezone(timedelta(hours=5, minutes=30), "IST") + + +def format_ist(value): + """Show a parsed time in IST. Stored timestamps and durations stay UTC.""" + if value is None: + return "—" + return value.astimezone(IST).strftime("%Y-%m-%d %H:%M:%S IST") + + def format_duration(start, finish): if not start or not finish: return "—" @@ -388,8 +398,7 @@ def stage_rows(events, results, build_url): log_link = ( f'' "Console") - started_text = ( - started.strftime("%Y-%m-%d %H:%M:%S UTC") if started else "—") + started_text = format_ist(started) rows.append( "" f"{html.escape(stage)}" @@ -554,9 +563,9 @@ def main(): VM {html.escape(args.vm or 'not assigned')} Started -{html.escape(args.started_at)} +{html.escape(format_ist(started))} Finished -{html.escape(args.finished_at)} +{html.escape(format_ist(finished))} Total duration {html.escape(format_duration(started, finished))} diff --git a/private-cicd/scripts/test_generate_presubmit_report.py b/private-cicd/scripts/test_generate_presubmit_report.py index 02dd29c95aa9..f9cc443ff827 100644 --- a/private-cicd/scripts/test_generate_presubmit_report.py +++ b/private-cicd/scripts/test_generate_presubmit_report.py @@ -118,6 +118,24 @@ def test_long_error_is_truncated(self): self.assertEqual(len(error), report.MAX_ERROR_CHARS + 1) +class IstTimeTest(unittest.TestCase): + def test_utc_time_is_shown_in_ist(self): + value = report.parse_time("2026-11-05T01:54:07Z") + self.assertEqual(report.format_ist(value), "2026-11-05 07:24:07 IST") + + def test_offset_time_and_date_rollover(self): + value = report.parse_time("2026-11-05T22:00:00+00:00") + self.assertEqual(report.format_ist(value), "2026-11-06 03:30:00 IST") + + def test_missing_time_is_a_dash(self): + self.assertEqual(report.format_ist(None), "—") + + def test_durations_are_unaffected(self): + start = report.parse_time("2026-11-05T01:00:00Z") + finish = report.parse_time("2026-11-05T02:30:00Z") + self.assertEqual(finish - start, report.timedelta(hours=1, minutes=30)) + + class FailurePageTest(unittest.TestCase): def test_suite_page_shows_failure_before_the_log(self): with tempfile.TemporaryDirectory() as directory: From 6c0546244b098a399d15e6f71b7352f6944ea015 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 10:14:14 +0530 Subject: [PATCH 12/19] Use the shared _start_vm in the ONTAP VM volume tests. Resolving the merge conflicts brought back a local _start_vm in the iSCSI and NFS3 VM volume tests. It used startVirtualMachineAPI, whose import had been removed, so test_08 and test_09 failed with a NameError and the snapshot tests were skipped. Remove the local copies and make the shared _start_vm in ontap_test_base.py skip a VM that is already running and wait for Running, as the local copies did. --- .../ontap/iscsi/instance/test_vm_volume_attach.py | 8 -------- .../ontap/nfs3/instance/test_vm_volume_attach.py | 8 -------- test/integration/plugins/ontap/ontap_test_base.py | 10 ++++++++-- 3 files changed, 8 insertions(+), 18 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 4afca614c7b9..83c6d2bacdb5 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 @@ -797,14 +797,6 @@ def _stop_vm(self, vm_id): self.apiClient.stopVirtualMachine(cmd) self._poll_vm_state(vm_id, "Stopped", timeout=600) - def _start_vm(self, vm_id): - if (self._vm_state(vm_id) or "").lower() == "running": - return - cmd = startVirtualMachineAPI.startVirtualMachineCmd() - cmd.id = vm_id - self.apiClient.startVirtualMachine(cmd) - self._poll_vm_state(vm_id, "Running", timeout=900) - def _root_volume(self, vm_id): cmd = listVolumesAPI.listVolumesCmd() cmd.virtualmachineid = vm_id 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 d8e1fd9f75f2..4adf3378a757 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 @@ -629,14 +629,6 @@ def _stop_vm(self, vm_id): self.apiClient.stopVirtualMachine(cmd) self._poll_vm_state(vm_id, "Stopped", timeout=600) - def _start_vm(self, vm_id): - if (self._vm_state(vm_id) or "").lower() == "running": - return - cmd = startVirtualMachineAPI.startVirtualMachineCmd() - cmd.id = vm_id - self.apiClient.startVirtualMachine(cmd) - self._poll_vm_state(vm_id, "Running", timeout=900) - def _root_volume(self, vm_id): cmd = listVolumesAPI.listVolumesCmd() cmd.virtualmachineid = vm_id diff --git a/test/integration/plugins/ontap/ontap_test_base.py b/test/integration/plugins/ontap/ontap_test_base.py index 261de3d9a2bf..eb36929651a8 100644 --- a/test/integration/plugins/ontap/ontap_test_base.py +++ b/test/integration/plugins/ontap/ontap_test_base.py @@ -1711,8 +1711,12 @@ def _start_vm(self, vm_id): ("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. + error so the cause is not hidden. Returns once the VM is Running; a + VM that is already Running is left alone. """ + vms = self.apiClient.listVirtualMachines(_list_vms_cmd(vm_id)) or [] + if vms and (vms[0].state or "").lower() == "running": + return cmd = startVirtualMachineAPI.startVirtualMachineCmd() cmd.id = vm_id attempts = self.VM_START_ATTEMPTS @@ -1720,7 +1724,9 @@ def _start_vm(self, vm_id): for attempt in range(1, attempts + 1): self._wait_for_routers_healthy() try: - return self.apiClient.startVirtualMachine(cmd) + self.apiClient.startVirtualMachine(cmd) + self._poll_vm_state(vm_id, "Running", timeout=900) + return except Exception as error: errors.append("attempt %d/%d: %s" % (attempt, attempts, error)) vms = self.apiClient.listVirtualMachines( From ec0807b4260bcfa275cb7c9c8210cfd062ebabfb Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 14:56:56 +0530 Subject: [PATCH 13/19] Delete failed VM snapshots after each ONTAP VM volume test. When ONTAP rejects a createVMSnapshot that runs while a revert's clones exist, CloudStack keeps the snapshot record in Error. The retry in test_23 then succeeds, but the record has no id the test saw, so it was never deleted. Any Instance Snapshot blocks detach, resize and destroy, so test_24, test_25 and test_27 failed. Sweep Error-state VM snapshots of both test VMs in tearDown. --- .../iscsi/instance/test_vm_volume_attach.py | 25 +++++++++++++++++++ .../nfs3/instance/test_vm_volume_attach.py | 25 +++++++++++++++++++ 2 files changed, 50 insertions(+) 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 83c6d2bacdb5..706c272b71d7 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 @@ -617,6 +617,7 @@ def tearDown(self): VmSnapshot.deleteVMSnapshot(self.apiClient, snap_id) except Exception as exc: logger.warning("tearDown: VM snapshot %s: %s" % (snap_id, exc)) + self._delete_failed_vm_snapshots() for snap_id in self._volume_snapshot_ids: try: Snapshot({"id": snap_id}).delete(self.apiClient) @@ -893,6 +894,30 @@ def _take_vm_snapshot(self, vm_id): def _vm_snapshots_of(self, vm_id): return VmSnapshot.list(self.apiClient, virtualmachineid=vm_id, listall=True) or [] + def _delete_failed_vm_snapshots(self): + """Delete VM snapshots left in Error by a rejected create. + + A rejected createVMSnapshot (ONTAP refusing while a revert's clones + exist) leaves its record behind with no id the test ever saw. Any + Instance Snapshot, even in Error, blocks detach, resize and destroy. + """ + for vm in (self.__class__.vm, self.__class__.vm2): + if vm is None: + continue + try: + stale = [s for s in self._vm_snapshots_of(vm.id) + if (getattr(s, "state", "") or "").lower() == "error"] + except Exception as exc: + logger.warning("tearDown: could not list VM snapshots of %s: %s" + % (vm.id, exc)) + continue + for snap in stale: + try: + VmSnapshot.deleteVMSnapshot(self.apiClient, snap.id) + except Exception as exc: + logger.warning("tearDown: failed VM snapshot %s: %s" + % (snap.id, exc)) + def _volume_snapshot_listed(self, snap_id): listed = Snapshot.list(self.apiClient, id=snap_id, listall=True) or [] return any(getattr(item, "id", None) == snap_id for item in listed) 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 4adf3378a757..c3b7e36a7316 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 @@ -449,6 +449,7 @@ def tearDown(self): VmSnapshot.deleteVMSnapshot(self.apiClient, snap_id) except Exception as exc: logger.warning("tearDown: VM snapshot %s: %s" % (snap_id, exc)) + self._delete_failed_vm_snapshots() for snap_id in self._volume_snapshot_ids: try: Snapshot({"id": snap_id}).delete(self.apiClient) @@ -725,6 +726,30 @@ def _take_vm_snapshot(self, vm_id): def _vm_snapshots_of(self, vm_id): return VmSnapshot.list(self.apiClient, virtualmachineid=vm_id, listall=True) or [] + def _delete_failed_vm_snapshots(self): + """Delete VM snapshots left in Error by a rejected create. + + A rejected createVMSnapshot (ONTAP refusing while a revert's clones + exist) leaves its record behind with no id the test ever saw. Any + Instance Snapshot, even in Error, blocks detach, resize and destroy. + """ + for vm in (self.__class__.vm, self.__class__.vm2): + if vm is None: + continue + try: + stale = [s for s in self._vm_snapshots_of(vm.id) + if (getattr(s, "state", "") or "").lower() == "error"] + except Exception as exc: + logger.warning("tearDown: could not list VM snapshots of %s: %s" + % (vm.id, exc)) + continue + for snap in stale: + try: + VmSnapshot.deleteVMSnapshot(self.apiClient, snap.id) + except Exception as exc: + logger.warning("tearDown: failed VM snapshot %s: %s" + % (snap.id, exc)) + def _volume_snapshot_listed(self, snap_id): listed = Snapshot.list(self.apiClient, id=snap_id, listall=True) or [] return any(getattr(item, "id", None) == snap_id for item in listed) From 74d3e97d8177016f9d33726f7341598531ede467 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Fri, 6 Nov 2026 10:39:36 +0530 Subject: [PATCH 14/19] Load the trusted CI scripts from main instead of the pilot branch. trustedCiBranch() named feature/CSTACKEX-340 for the PR-119 pilot. Return main so the presubmit uses the merged CI scripts for every pull request, and update the job guide to match. The Jenkins folder still has to switch its Remote Jenkinsfile Provider branch to main and its filter to PR-* after this merges. --- private-cicd/Jenkinsfile | 6 +++--- private-cicd/docs/CREATE-PRESUBMIT-JOB.md | 20 +++++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 2a899c3da655..3873dded596c 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -966,10 +966,10 @@ 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. +// Remote Jenkinsfile Provider loads this Jenkinsfile from. Change the provider +// branch in the Jenkins folder to match when this value changes. String trustedCiBranch() { - return 'feature/CSTACKEX-340' + return 'main' } void checkoutCiScripts() { diff --git a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md index 68d6c77ef49d..cd0610f057be 100644 --- a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md +++ b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md @@ -25,12 +25,13 @@ non-draft origin pull request into `main`. Remote Jenkinsfile Provider loads `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 trusted CI branch is `main`. Both the Remote Jenkinsfile Provider branch and +`trustedCiBranch()` in the Jenkinsfile must name that branch. During the PR-119 +pilot the provider branch was `feature/CSTACKEX-340` and the folder include +filter was `PR-119`. Do not switch the provider branch to `main` until PR-119 is +merged: the previous `main` Jenkinsfile knows nothing about `CHANGE_ID` and +defaults to `SOURCE_MODE=discover`. After the merge, change the provider branch +to `main` and the filter to `PR-*`, and keep `feature/CSTACKEX-340` until then. 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, @@ -47,12 +48,13 @@ Open `cloudstack-ontap-presubmit-mb` > **Configure**: | --- | --- | | 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** | +| Filter by name | `PR-*` (`PR-119` during the pilot) | +| Build Configuration | **by Remote Jenkinsfile Provider**, repository `https://github.com/NetApp/cloudstack.git`, branch `main` (`feature/CSTACKEX-340` during the pilot), 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 +Run **Scan Repository Now** and expect one child for each open non-draft pull +request into `main` (only `PR-119` during the pilot). The first build may stop on Script Security. Approve only signatures requested by this Jenkinsfile and rebuild. From c6d1dfe5dd0c2ce2af70b4b6c9791f518b4d37cd Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 12 Nov 2026 14:52:42 +0530 Subject: [PATCH 15/19] Address review comments on the multibranch presubmit. - Choose the trusted CI branch from PRESUBMIT_TRUSTED_CI_BRANCH, then the manual job's own SCM, then main. A multibranch PR build never reads scm, since there it is the pull request's own code. Document the setting and that it must match the Remote Jenkinsfile Provider branch. - Reject a pull request that is not open in resolve-pull-request.py, because the closed-PR guard no longer fires in multibranch mode. Add tests. - End a CloudStack job error message at the quote that closes errortext, so quoted names inside the message are kept in the report. Add tests. --- private-cicd/Jenkinsfile | 39 +++++++++++++++++-- private-cicd/docs/CREATE-PRESUBMIT-JOB.md | 39 +++++++++++++++---- .../scripts/generate-presubmit-report.py | 7 +++- private-cicd/scripts/resolve-pull-request.py | 4 ++ .../scripts/test_generate_presubmit_report.py | 22 +++++++++++ .../scripts/test_resolve_pull_request.py | 13 ++++++- 6 files changed, 112 insertions(+), 12 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 3873dded596c..0e53e051339b 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -965,10 +965,43 @@ void abortSupersededRun() { } } -// Branch that supplies the trusted CI scripts. It must equal the branch that -// Remote Jenkinsfile Provider loads this Jenkinsfile from. Change the provider -// branch in the Jenkins folder to match when this value changes. +// Branch name from a Git SCM branch spec such as '*/main' or 'refs/heads/main'. +// Returns '' for a wildcard or any name Git would not accept. +String branchFromSpec(String spec) { + def name = (spec ?: '').trim() + if (name.startsWith('*/')) { + name = name.substring(2) + } + if (name.startsWith('refs/heads/')) { + name = name.substring('refs/heads/'.length()) + } + return name.contains('*') || !isValidBranchName(name) ? '' : name +} + +// Branch that supplies the trusted CI scripts, in this order: +// 1. PRESUBMIT_TRUSTED_CI_BRANCH, a job or folder environment variable. Set it +// to the Remote Jenkinsfile Provider branch when that is not 'main', for +// example while a Jenkinsfile change is piloted from a feature branch. +// 2. For the manual job, the branch of the job's own Pipeline SCM. +// 3. 'main'. +// A multibranch pull request build never reads scm: there it is the pull +// request's own code, and the trusted scripts must not come from the PR. String trustedCiBranch() { + def override = branchFromSpec(env.PRESUBMIT_TRUSTED_CI_BRANCH) + if (override) { + return override + } + if (!isMultibranchPullRequest()) { + try { + // A BranchSpec prints as its name; this avoids a new sandbox approval. + def fromJob = scm?.branches ? branchFromSpec("${scm.branches[0]}") : '' + if (fromJob) { + return fromJob + } + } catch (Exception e) { + echo "Could not read the job's SCM branch, using main: ${e}" + } + } return 'main' } diff --git a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md index cd0610f057be..37291f682777 100644 --- a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md +++ b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md @@ -25,13 +25,38 @@ non-draft origin pull request into `main`. Remote Jenkinsfile Provider loads `private-cicd/Jenkinsfile` from the trusted CI branch. Match branches stays off. Draft pull requests get no job. Do not add a webhook. -The trusted CI branch is `main`. Both the Remote Jenkinsfile Provider branch and -`trustedCiBranch()` in the Jenkinsfile must name that branch. During the PR-119 -pilot the provider branch was `feature/CSTACKEX-340` and the folder include -filter was `PR-119`. Do not switch the provider branch to `main` until PR-119 is -merged: the previous `main` Jenkinsfile knows nothing about `CHANGE_ID` and -defaults to `SOURCE_MODE=discover`. After the merge, change the provider branch -to `main` and the filter to `PR-*`, and keep `feature/CSTACKEX-340` until then. +The trusted CI branch is `main`. The Jenkinsfile and the CI scripts of every pull +request build come from the trusted branch, never from the pull request, so a PR +that changes `private-cicd/` is built with the unchanged trusted copy. Its +changes apply only after they are merged. To try a Jenkinsfile or script change +before the merge, run the manual job (`SOURCE_MODE=pull_request` or `branch`) +against the feature branch. + +The CI scripts branch is chosen by `trustedCiBranch()` in the Jenkinsfile: + +1. the `PRESUBMIT_TRUSTED_CI_BRANCH` environment variable, if the job or folder + defines it; +2. for the manual job, the branch of its own Pipeline SCM; +3. otherwise `main`. + +A multibranch pull request build cannot read its branch from `scm`, because +there `scm` is the pull request's own code. The value must therefore equal the +Remote Jenkinsfile Provider branch. When the provider branch is not `main`, set +`PRESUBMIT_TRUSTED_CI_BRANCH` to the same branch in the same step. A mismatch +shows as `NOT_BUILT` with "pull request lookup failed", because the scripts +are missing on the other branch. + +During the PR-119 pilot the provider branch was `feature/CSTACKEX-340`, the +folder include filter was `PR-119`, and `PRESUBMIT_TRUSTED_CI_BRANCH` had to name +that branch. Do not switch the provider branch to `main` until PR-119 is merged: +the previous `main` Jenkinsfile knows nothing about `CHANGE_ID` and defaults to +`SOURCE_MODE=discover`. After the merge, change the provider branch to `main`, +remove `PRESUBMIT_TRUSTED_CI_BRANCH` and change the filter to `PR-*`. Keep +`feature/CSTACKEX-340` until then. + +A pull request that is closed or merged is not built, even if someone selects +Rebuild on an orphaned item: the build ends `NOT_BUILT` with "pull request is not +open". 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, diff --git a/private-cicd/scripts/generate-presubmit-report.py b/private-cicd/scripts/generate-presubmit-report.py index dcafbb8f43d8..c80301a5cf04 100644 --- a/private-cicd/scripts/generate-presubmit-report.py +++ b/private-cicd/scripts/generate-presubmit-report.py @@ -136,8 +136,13 @@ def html_document(title, body): FAILURE_HEADER = re.compile(r"^(ERROR|FAIL):\s*(.*)$") DESCRIBED_TEST = re.compile( r"^(.*?)\s*\.\.\.\s*=== TestName:\s+(\S+)\s+\|\s+Status\s+:\s+(\S+)") +# The message ends at the quote that closes the errortext value: one followed by +# the closing brace or by the next "key :" pair. A quote inside the message, +# such as a quoted VM name, is kept. A message with no closing quote runs to the +# end of the line. JOB_ERROR = re.compile( - r"errorcode\s*:\s*(\d+)\s*,\s*errortext\s*:\s*'(.*?)(?:'\s*[,}]|$)") + r"errorcode\s*:\s*(\d+)\s*,\s*errortext\s*:\s*'" + r"(.*?)(?:'\s*(?:\}|,\s*\w+\s*:)|$)") MAX_FAILURE_LINES = 80 MAX_ERROR_CHARS = 400 diff --git a/private-cicd/scripts/resolve-pull-request.py b/private-cicd/scripts/resolve-pull-request.py index 01f90de0a50a..9f2200916958 100644 --- a/private-cicd/scripts/resolve-pull-request.py +++ b/private-cicd/scripts/resolve-pull-request.py @@ -73,6 +73,7 @@ def assess(pull, pr_id, indexed_sha, change_target, expected_repository): "pr_head_sha": head.get("sha") or "", "pr_head_branch": head.get("ref") or "", "pr_base_branch": base.get("ref") or "", + "pr_state": (pull or {}).get("state") or "", "pr_draft": bool((pull or {}).get("draft")), "pr_title": (pull or {}).get("title") or "", "pr_url": (pull or {}).get("html_url") or "", @@ -94,6 +95,9 @@ def assess(pull, pr_id, indexed_sha, change_target, expected_repository): f"base branch is '{decision['pr_base_branch']}' and change target " f"is '{change_target}'." ) + elif decision["pr_state"] != "open": + decision["reason"] = ( + f"pull request is not open (state: {decision['pr_state'] or 'unknown'}).") elif decision["pr_draft"]: decision["reason"] = "pull request is a draft." elif not decision["pr_head_sha"] or len(decision["pr_head_sha"]) != 40: diff --git a/private-cicd/scripts/test_generate_presubmit_report.py b/private-cicd/scripts/test_generate_presubmit_report.py index f9cc443ff827..7edcb90f060b 100644 --- a/private-cicd/scripts/test_generate_presubmit_report.py +++ b/private-cicd/scripts/test_generate_presubmit_report.py @@ -100,6 +100,28 @@ def test_cloudstack_job_failure_leads_with_its_message(self): self.parse(text)[0]["error"], "Job failed (code 530): Unable to start the VM") + def test_cloudstack_job_failure_keeps_quoted_names_in_the_message(self): + messages = [ + "Unable to start VM 'i-2-5-VM', host unreachable", + "Volume doesn't exist", + "Failed to detach 'vol-1' from 'VM-2'", + ] + for message in messages: + line = ("Exception: Job failed: {accountid : 'a', jobresult : " + "{errorcode : 530, errortext : '%s'}, jobinstancetype : " + "'Volume'}" % message) + text = f"{SEP}\nERROR: d\n{DASH}\n{line}\n" + self.assertEqual( + self.parse(text)[0]["error"], + "Job failed (code 530): " + message) + + def test_cloudstack_job_failure_without_closing_quote_shows_the_rest(self): + line = ("Exception: Job failed: {jobresult : {errorcode : 530, " + "errortext : 'cut off here") + text = f"{SEP}\nERROR: d\n{DASH}\n{line}\n" + self.assertEqual( + self.parse(text)[0]["error"], "Job failed (code 530): cut off here") + def test_unmapped_block_is_kept_without_names(self): failures = self.parse(SETUP_ERROR) self.assertEqual(len(failures), 1) diff --git a/private-cicd/scripts/test_resolve_pull_request.py b/private-cicd/scripts/test_resolve_pull_request.py index 739fe10a89e0..71c9320fa876 100644 --- a/private-cicd/scripts/test_resolve_pull_request.py +++ b/private-cicd/scripts/test_resolve_pull_request.py @@ -31,11 +31,13 @@ SHA = "a" * 40 -def pull(number="98", draft=False, base="main", repo="NetApp/cloudstack", sha=SHA): +def pull(number="98", draft=False, base="main", repo="NetApp/cloudstack", sha=SHA, + state="open"): return { "number": int(number), "title": "Test", "html_url": f"https://github.com/NetApp/cloudstack/pull/{number}", + "state": state, "draft": draft, "base": {"ref": base}, "head": { @@ -70,6 +72,15 @@ def test_rejects_draft_fork_base_and_sha_mismatch(self): candidate, "98", SHA, "main", "NetApp/cloudstack") self.assertFalse(decision["eligible"], candidate) + def test_rejects_closed_or_merged_pr(self): + # A merged PR is also reported by GitHub as state "closed". + for state in ("closed", "", None): + decision = MODULE.assess( + pull(state=state), "98", SHA, "main", "NetApp/cloudstack") + + self.assertFalse(decision["eligible"], state) + self.assertIn("not open", decision["reason"]) + def test_accepts_github_head_when_jenkins_sha_is_missing(self): decision = MODULE.assess(pull(), "98", "", "main", "NetApp/cloudstack") From 2cceb9a8eb6bdce134dca0a2158f9ab78fc3f92e Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 12 Nov 2026 15:02:06 +0530 Subject: [PATCH 16/19] Make the presubmit Check reliable enough to be required. - Complete the Check after the report and archive exist, so its summary names the failed stage and links the Jenkins build and the HTML report. - Retry the completing PATCH three times; a Check left in progress blocks a pull request that requires it. - Publish an already completed Check when the one created at the start is missing, and complete a started Check as cancelled when the build is not built, so a result always reaches the pull request. - Document what each build result does to a required Check. --- private-cicd/Jenkinsfile | 40 ++++++-- private-cicd/docs/PRIVATE-CICD-GUIDE.md | 23 +++++ private-cicd/scripts/github-check-run.sh | 125 ++++++++++++++--------- 3 files changed, 134 insertions(+), 54 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 0e53e051339b..15c4e7cbe432 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -545,10 +545,15 @@ void createGithubCheck() { } } -void completeGithubCheck(String conclusion) { - if (!env.GITHUB_CHECK_RUN_ID?.trim() || !hasGithubAppCredential()) { +// Publishes the final result. When the run could not be created at the start +// (no run ID), a completed check run is created instead, so the result still +// reaches the pull request. This matters once the check is required: a pull +// request without a reported result stays blocked. +void completeGithubCheck(String conclusion, String failedStage = '', String reportUrl = '') { + if (!hasGithubAppCredential() || sourceValue('SOURCE_MODE') != 'pull_request') { return } + def runId = env.GITHUB_CHECK_RUN_ID?.trim() try { withCredentials([ usernamePassword( @@ -558,13 +563,28 @@ void completeGithubCheck(String conclusion) { ) ]) { withEnv(githubEnvironment() + [ - "GITHUB_CHECK_CONCLUSION=${conclusion}" + "GITHUB_CHECK_CONCLUSION=${conclusion}", + "GITHUB_CHECK_FAILED_STAGE=${failedStage}", + "GITHUB_CHECK_REPORT_URL=${reportUrl}" ]) { - sh 'private-cicd/scripts/github-check-run.sh complete' - echo "Completed GitHub Check Run ${env.GITHUB_CHECK_RUN_ID} with conclusion ${conclusion}." + if (runId) { + sh 'private-cicd/scripts/github-check-run.sh complete' + echo "Completed GitHub Check Run ${runId} with conclusion ${conclusion}." + } else { + def created = sh( + script: 'private-cicd/scripts/github-check-run.sh create', + returnStdout: true + ).trim() + echo "No GitHub Check Run existed; published a completed one (${created}) " + + "with conclusion ${conclusion}." + } } } } catch (Exception e) { + currentBuild.description = ( + (currentBuild.description ? currentBuild.description + ' | ' : '') + + 'GitHub Check result not published' + ) echo "GitHub Check completion failed; presubmit result is unchanged: ${e}" } } @@ -1398,6 +1418,10 @@ void deployAndRunOntapIntegration() { void publishPresubmitResults() { if (isMultibranchPullRequest() && currentBuild.currentResult == 'NOT_BUILT') { echo "Multibranch PR-${env.CHANGE_ID} was not built." + // A check created before the build stopped must not stay in progress. + if (env.GITHUB_CHECK_RUN_ID?.trim()) { + completeGithubCheck('cancelled') + } return } if (sourceValue('SOURCE_MODE') == 'discover') { @@ -1421,7 +1445,6 @@ void publishPresubmitResults() { "yyyy-MM-dd'T'HH:mm:ssXXX", TimeZone.getTimeZone('UTC') ) - completeGithubCheck(conclusion) if (!env.PRESUBMIT_MAIL_TO?.trim()) { resolvePresubmitMailRecipient() } @@ -1472,6 +1495,11 @@ void publishPresubmitResults() { } def failedStage = fileExists("${env.PRESUBMIT_RESULTS}/failed-stage.txt") ? readFile("${env.PRESUBMIT_RESULTS}/failed-stage.txt").trim() : '' + // Completed after the report and the archive exist, so the check can + // name the failed stage and link the report. + def reportUrl = fileExists("${env.PRESUBMIT_RESULTS}/report.html") ? + "${env.BUILD_URL}artifact/${env.PRESUBMIT_RESULTS}/report.html" : '' + completeGithubCheck(conclusion, failedStage, reportUrl) def finalSubject = failedStage ? "${presubmitSubjectBase()}: ${result} in ${failedStage}" : "${presubmitSubjectBase()}: ${result}" diff --git a/private-cicd/docs/PRIVATE-CICD-GUIDE.md b/private-cicd/docs/PRIVATE-CICD-GUIDE.md index bfc5edb4478d..03fd20622f7a 100644 --- a/private-cicd/docs/PRIVATE-CICD-GUIDE.md +++ b/private-cicd/docs/PRIVATE-CICD-GUIDE.md @@ -546,6 +546,29 @@ cloudstack-ontap-presubmit Without protection it is informational. Verify pending/failed results block merge and each new SHA needs a new Check. +How the Check behaves once it is required: + +| Build result | Check conclusion | Merge | +| --- | --- | --- | +| `SUCCESS` | `success` | allowed | +| `FAILURE`, `UNSTABLE` | `failure` | blocked | +| `ABORTED`, superseded or timed out | `cancelled` | blocked until a new run passes | +| not built (draft, fork, closed, wrong base) | no Check, or `cancelled` if one was started | blocked until a run passes | + +- The Check is created `in_progress` at the start of the run and completed after the report is archived. The + completed Check names the failed stage and links the Jenkins build and the HTML report. +- If creating the Check failed at the start, the final step publishes an already completed Check instead, so the + result still reaches the pull request. If that also fails, the job description says + `GitHub Check result not published`, and the Check stays missing or in progress, which blocks the merge. + Fix the cause (run `SOURCE_MODE=diagnose`), then push a commit or rebuild the PR job. +- Completion is retried three times. A Jenkins crash or restart mid-run still leaves the Check in progress. Rebuild + the PR job, or push a commit, to replace it. A GitHub **Re-run** button does nothing, because Jenkins receives no + webhooks. +- In the branch rule, select the Check produced by the presubmit GitHub App as its source, not **Any source**. +- Do not require the `continuous-integration/jenkins/pr-head` status that GitHub Branch Source can also post. The + Check above is the only gate. +- A pull request from a fork never gets a Check, so it cannot merge while the Check is required. + ## 13. Manual branch job and exact SHA Create a separate Pipeline-from-SCM job: diff --git a/private-cicd/scripts/github-check-run.sh b/private-cicd/scripts/github-check-run.sh index e00821cede8c..62625ce6ef54 100755 --- a/private-cicd/scripts/github-check-run.sh +++ b/private-cicd/scripts/github-check-run.sh @@ -29,7 +29,11 @@ # process list. # GITHUB_HEAD_SHA — create, commit-email and diagnose. BUILD_URL — create and # complete. GITHUB_CHECK_RUN_ID, GITHUB_CHECK_CONCLUSION — complete. -# Optional: PR_ID and BUILD_TAG label the check run. +# Optional: PR_ID and BUILD_TAG label the check run. For create, setting +# GITHUB_CHECK_CONCLUSION publishes an already completed check run, which is +# how a result is still reported when the run could not be created at the +# start. GITHUB_CHECK_FAILED_STAGE and GITHUB_CHECK_REPORT_URL add the failed +# stage and the report link to the check's summary. # Called by the Jenkinsfile. Does not call other private-cicd scripts. set -euo pipefail @@ -129,35 +133,73 @@ with open(sys.argv[1], encoding="utf-8") as handle: PY } -create_check() { - require_env GITHUB_ACCESS_TOKEN GITHUB_REPOSITORY GITHUB_HEAD_SHA BUILD_URL - local payload response - payload="$(mktemp)" - response="$(mktemp)" - trap "rm -f '$payload' '$response'" EXIT - - python3 - "$payload" <<'PY' +# Builds the check-run payload for the "create" and "complete" subcommands from +# the environment. GITHUB_CHECK_CONCLUSION makes it a completed run. +write_payload() { + python3 - "$1" "$2" <<'PY' import json import os import sys +kind = sys.argv[2] +env = os.environ.get +conclusion = env("GITHUB_CHECK_CONCLUSION", "") +build_url = env("BUILD_URL", "") +sha = env("GITHUB_HEAD_SHA", "") +pr = env("PR_ID", "unknown") + payload = { "name": "cloudstack-ontap-presubmit", - "head_sha": os.environ["GITHUB_HEAD_SHA"], - "status": "in_progress", - "details_url": os.environ["BUILD_URL"], - "external_id": os.environ.get("BUILD_TAG", ""), - "output": { - "title": f"CloudStack ONTAP presubmit for PR-{os.environ.get('PR_ID', 'unknown')}", + "details_url": build_url, +} +if kind == "create": + payload["head_sha"] = sha + payload["external_id"] = env("BUILD_TAG", "") + +if kind == "create" and not conclusion: + payload["status"] = "in_progress" + payload["output"] = { + "title": f"CloudStack ONTAP presubmit for PR-{pr}", "summary": ( - f"Jenkins build started for {os.environ['GITHUB_HEAD_SHA'][:12]}. " - f"Details: {os.environ['BUILD_URL']}" + f"Jenkins build started for {sha[:12]}. " + f"Details: {build_url}" ), - }, -} + } +else: + verdict = { + "success": "passed", + "failure": "failed", + "cancelled": "was cancelled", + "timed_out": "timed out", + "neutral": "finished with a neutral result", + }.get(conclusion, conclusion) + lines = [f"The CloudStack ONTAP presubmit {verdict}."] + if env("GITHUB_CHECK_FAILED_STAGE"): + lines.append(f"Failed stage: {env('GITHUB_CHECK_FAILED_STAGE')}") + if sha: + lines.append(f"Commit: {sha[:12]}") + lines.append(f"Jenkins build and logs: {build_url}") + if env("GITHUB_CHECK_REPORT_URL"): + lines.append(f"Test report: {env('GITHUB_CHECK_REPORT_URL')}") + payload["status"] = "completed" + payload["conclusion"] = conclusion + payload["output"] = { + "title": f"CloudStack ONTAP presubmit {verdict}", + "summary": "\n\n".join(lines), + } with open(sys.argv[1], "w", encoding="utf-8") as output: json.dump(payload, output) PY +} + +create_check() { + require_env GITHUB_ACCESS_TOKEN GITHUB_REPOSITORY GITHUB_HEAD_SHA BUILD_URL + local payload response + payload="$(mktemp)" + response="$(mktemp)" + trap "rm -f '$payload' '$response'" EXIT + + write_payload "$payload" create github_api POST "/repos/${GITHUB_REPOSITORY}/check-runs" "$payload" > "$response" || { local status=$? @@ -178,41 +220,28 @@ PY complete_check() { require_env GITHUB_ACCESS_TOKEN GITHUB_REPOSITORY GITHUB_CHECK_RUN_ID \ GITHUB_CHECK_CONCLUSION BUILD_URL - local payload response + local payload response attempt payload="$(mktemp)" response="$(mktemp)" trap "rm -f '$payload' '$response'" EXIT - python3 - "$payload" <<'PY' -import json -import os -import sys + write_payload "$payload" complete -conclusion = os.environ["GITHUB_CHECK_CONCLUSION"] -payload = { - "name": "cloudstack-ontap-presubmit", - "status": "completed", - "conclusion": conclusion, - "details_url": os.environ["BUILD_URL"], - "output": { - "title": f"CloudStack ONTAP presubmit: {conclusion}", - "summary": ( - f"Jenkins build completed with conclusion {conclusion}. " - f"Details and logs: {os.environ['BUILD_URL']}" - ), - }, -} -with open(sys.argv[1], "w", encoding="utf-8") as output: - json.dump(payload, output) -PY - - if ! github_api PATCH \ - "/repos/${GITHUB_REPOSITORY}/check-runs/${GITHUB_CHECK_RUN_ID}" \ - "$payload" > "$response"; then - echo "GitHub refused to complete the check run:" >&2 + # A check left in progress blocks a pull request that requires it, so a + # transient GitHub error gets a few retries before the failure is reported. + for attempt in 1 2 3; do + if github_api PATCH \ + "/repos/${GITHUB_REPOSITORY}/check-runs/${GITHUB_CHECK_RUN_ID}" \ + "$payload" > "$response"; then + return 0 + fi + echo "GitHub refused to complete the check run (attempt ${attempt}/3):" >&2 print_github_error "$response" - return 1 - fi + if (( attempt < 3 )); then + sleep "${CHECK_RETRY_SECONDS:-10}" + fi + done + return 1 } # Read-only probes plus one write probe, ordered so that the first failure From 218232a52bb5aba31d49ca526a20ebc05d69f342 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 12 Nov 2026 18:48:52 +0530 Subject: [PATCH 17/19] Report unit-test coverage of the ONTAP plugin from the main build. An ontap-coverage profile in the plugin pom, activated by -Dontap.coverage, attaches JaCoCo to the build that already runs the plugin's unit tests, so no second test run is needed. mvn-full.sh turns it on with ONTAP_COVERAGE=true and keeps the report. ontap-coverage.py summarises the unit and functional reports, with the combined figure computed per source line, and the report generator embeds the summary in the mail and report.html. --- plugins/storage/volume/ontap/pom.xml | 38 +++ .../scripts/generate-presubmit-report.py | 12 +- private-cicd/scripts/mvn-full.sh | 20 ++ private-cicd/scripts/ontap-coverage.py | 279 ++++++++++++++++++ .../scripts/test_generate_presubmit_report.py | 16 + private-cicd/scripts/test_ontap_coverage.py | 155 ++++++++++ 6 files changed, 518 insertions(+), 2 deletions(-) create mode 100644 private-cicd/scripts/ontap-coverage.py create mode 100644 private-cicd/scripts/test_ontap_coverage.py diff --git a/plugins/storage/volume/ontap/pom.xml b/plugins/storage/volume/ontap/pom.xml index c18bf559f92a..05929c188c92 100644 --- a/plugins/storage/volume/ontap/pom.xml +++ b/plugins/storage/volume/ontap/pom.xml @@ -173,4 +173,42 @@ + + + + ontap-coverage + + + ontap.coverage + + + + + + org.jacoco + jacoco-maven-plugin + ${cs.jacoco-plugin.version} + + + ontap-coverage-agent + + prepare-agent + + + + ontap-coverage-report + verify + + report + + + + + + + + diff --git a/private-cicd/scripts/generate-presubmit-report.py b/private-cicd/scripts/generate-presubmit-report.py index c80301a5cf04..0799bb8ba41f 100644 --- a/private-cicd/scripts/generate-presubmit-report.py +++ b/private-cicd/scripts/generate-presubmit-report.py @@ -527,6 +527,14 @@ def table(headers, rows): f"{cells}{styled_rows}") +def coverage_section(results): + """The ONTAP coverage summary written by coverage-report.sh, if any.""" + try: + return (results / "coverage" / "summary.html").read_text(encoding="utf-8") + except OSError: + return "" + + def main(): args = parse_args() args.results.mkdir(parents=True, exist_ok=True) @@ -595,8 +603,8 @@ def main(): if tests else "

ONTAP tests

No Marvin test summary was available.

") body = ( - header + failure + "

Pipeline stages

" + - stage_table + test_section) + header + failure + coverage_section(args.results) + + "

Pipeline stages

" + stage_table + test_section) document = html_document("CloudStack ONTAP presubmit report", body) (args.results / "report.html").write_text(document, encoding="utf-8") (args.results / "email-body.html").write_text(body, encoding="utf-8") diff --git a/private-cicd/scripts/mvn-full.sh b/private-cicd/scripts/mvn-full.sh index 605a71ee1ec6..c9bb3add4715 100755 --- a/private-cicd/scripts/mvn-full.sh +++ b/private-cicd/scripts/mvn-full.sh @@ -26,6 +26,8 @@ # 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), +# ONTAP_COVERAGE=true also writes the ONTAP plugin JaCoCo report to +# $RESULT_DIR/coverage/unit, # 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. @@ -46,6 +48,7 @@ ENABLE_NOREDIST="${ENABLE_NOREDIST:-false}" ENABLE_SIMULATOR="${ENABLE_SIMULATOR:-true}" THREADS="${MAVEN_THREADS-$(nproc)}" RERUN_FAILING_TESTS="${RERUN_FAILING_TESTS:-0}" +ONTAP_COVERAGE="${ONTAP_COVERAGE:-false}" RESULT_DIR="${RESULT_DIR:-}" mvn_args=(-B -P developer,systemvm) @@ -65,6 +68,11 @@ fi if [[ "$RERUN_FAILING_TESTS" != "0" ]]; then mvn_args+=("-Dsurefire.rerunFailingTestsCount=$RERUN_FAILING_TESTS") fi +if [[ "$ONTAP_COVERAGE" == "true" ]]; then + # Activates the ontap-coverage profile of the ONTAP plugin module, so the + # same build that runs its unit tests also writes the JaCoCo report. + mvn_args+=(-Dontap.coverage=true) +fi echo "==> Full build in $SOURCE_DIR" @@ -118,6 +126,18 @@ if [[ "$report_count" -gt 0 ]]; then | tar --null -czf "$RESULT_DIR/surefire-reports.tar.gz" --files-from - fi +# JaCoCo report written by the ontap-coverage profile of the ONTAP plugin. +ontap_coverage_dir="plugins/storage/volume/ontap/target/site/jacoco" +if [[ "$ONTAP_COVERAGE" == "true" ]]; then + if [[ -f "$ontap_coverage_dir/jacoco.xml" ]]; then + mkdir -p "$RESULT_DIR/coverage" + cp -a "$ontap_coverage_dir" "$RESULT_DIR/coverage/unit" + echo "ONTAP unit-test coverage report: $RESULT_DIR/coverage/unit/jacoco.xml" + else + echo "ONTAP unit-test coverage was requested but $ontap_coverage_dir/jacoco.xml was not produced" >&2 + fi +fi + if [[ "$maven_status" -eq 0 ]]; then result="SUCCESS" else diff --git a/private-cicd/scripts/ontap-coverage.py b/private-cicd/scripts/ontap-coverage.py new file mode 100644 index 000000000000..1e711d3e52c6 --- /dev/null +++ b/private-cicd/scripts/ontap-coverage.py @@ -0,0 +1,279 @@ +#!/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. + +# Purpose: +# Summarise ONTAP storage plugin coverage for one presubmit run. Reads the +# JaCoCo XML reports of the unit tests (produced by the Maven build) and of +# the functional Marvin tests (produced from the management server's JaCoCo +# dump), and writes summary.json, summary.md and summary.html to --output-dir. +# The combined figure is computed per source line, so it does not depend on +# the two reports agreeing on class IDs: a line counts as covered when either +# run executed it. +# Dependencies: +# Python 3 standard library only. +# Called by: coverage-report.sh. Read by generate-presubmit-report.py and the +# Jenkinsfile, which publish the summary in the mail, the report, the GitHub +# Check and a pull-request comment. + +import argparse +import html +import json +import re +import sys +import xml.etree.ElementTree as ET +from pathlib import Path + +# Marks the pull-request comment so a later run updates it in place. +COMMENT_MARKER = "" +TITLE = "ONTAP storage plugin test coverage" +COUNTERS = ("LINE", "BRANCH", "METHOD", "CLASS") +DOCTYPE = re.compile(r"]*>") + + +def parse_report(path): + """Return {"totals": {...}, "files": {...}} for one JaCoCo XML report. + + files maps "package/File.java" to {"lines": {nr: (missed, covered)}}. + Returns None when the file is missing or is not a JaCoCo report. + """ + try: + text = Path(path).read_text(encoding="utf-8", errors="replace") + root = ET.fromstring(DOCTYPE.sub("", text, count=1)) + except (OSError, ET.ParseError): + return None + if root.tag != "report": + return None + totals = {} + for counter in root.findall("counter"): + kind = counter.get("type") + if kind in COUNTERS: + totals[kind] = (int(counter.get("missed", 0)), int(counter.get("covered", 0))) + files = {} + for package in root.findall("package"): + package_name = package.get("name", "") + for source in package.findall("sourcefile"): + lines = {} + for line in source.findall("line"): + lines[int(line.get("nr"))] = ( + int(line.get("mi", 0)), int(line.get("ci", 0))) + files[f"{package_name}/{source.get('name')}"] = {"lines": lines} + return {"totals": totals, "files": files} + + +def percent(covered, missed): + total = covered + missed + return round(100.0 * covered / total, 1) if total else None + + +def counter_summary(totals, kind): + missed, covered = totals.get(kind, (0, 0)) + return {"covered": covered, "missed": missed, "percent": percent(covered, missed)} + + +def covered_line_numbers(file_entry): + return {nr for nr, (_, covered) in file_entry["lines"].items() if covered > 0} + + +def combine(unit, functional): + """Line coverage of the union of both runs, plus a row for every file.""" + names = set() + for report in (unit, functional): + if report: + names.update(report["files"]) + rows = [] + total_lines = total_covered = 0 + for name in sorted(names): + line_sets = [] + all_lines = set() + for report in (unit, functional): + entry = report["files"].get(name) if report else None + if entry: + all_lines.update(entry["lines"]) + line_sets.append(covered_line_numbers(entry)) + else: + line_sets.append(set()) + if not all_lines: + continue + unit_covered, functional_covered = line_sets + combined_covered = unit_covered | functional_covered + rows.append({ + "file": name, + "lines": len(all_lines), + "unit": percent(len(unit_covered), len(all_lines) - len(unit_covered)) + if unit else None, + "functional": percent(len(functional_covered), + len(all_lines) - len(functional_covered)) + if functional else None, + "combined": percent(len(combined_covered), + len(all_lines) - len(combined_covered)), + "missed": len(all_lines) - len(combined_covered), + }) + total_lines += len(all_lines) + total_covered += len(combined_covered) + return { + "covered": total_covered, + "missed": total_lines - total_covered, + "percent": percent(total_covered, total_lines - total_covered), + }, rows + + +def build_summary(unit, functional, top, notes): + combined, rows = combine(unit, functional) + worst = sorted(rows, key=lambda row: (-row["missed"], row["file"])) + return { + "title": TITLE, + "unit": ({kind.lower(): counter_summary(unit["totals"], kind) for kind in COUNTERS} + if unit else None), + "functional": ({kind.lower(): counter_summary(functional["totals"], kind) + for kind in COUNTERS} if functional else None), + "combined_line": combined if (unit or functional) else None, + "least_covered": [row for row in worst if row["missed"] > 0][:top], + "notes": notes, + } + + +def fmt(value): + return "n/a" if value is None else f"{value:.1f}%" + + +def cell(section, kind): + return fmt(section[kind]["percent"]) if section else "not available" + + +def render_markdown(summary, links): + unit, functional = summary["unit"], summary["functional"] + combined = summary["combined_line"] + lines = [COMMENT_MARKER, f"### {summary['title']}", ""] + lines += [ + "| | Line | Branch | Method |", + "| --- | --- | --- | --- |", + f"| Unit tests | {cell(unit, 'line')} | {cell(unit, 'branch')} | {cell(unit, 'method')} |", + f"| Functional (Marvin) tests | {cell(functional, 'line')} | " + f"{cell(functional, 'branch')} | {cell(functional, 'method')} |", + f"| **Combined** | **{fmt(combined['percent']) if combined else 'not available'}** | | |", + "", + ] + if summary["least_covered"]: + lines += ["Least covered files (by lines not covered by either run):", "", + "| File | Lines | Unit | Functional | Combined |", + "| --- | --- | --- | --- | --- |"] + for row in summary["least_covered"]: + name = row["file"].replace("org/apache/cloudstack/storage/", "") + lines.append(f"| `{name}` | {row['lines']} | {fmt(row['unit'])} | " + f"{fmt(row['functional'])} | {fmt(row['combined'])} |") + lines.append("") + for note in summary["notes"]: + lines += [f"> {note}", ""] + for label, url in links: + if url: + lines.append(f"[{label}]({url}) ") + return "\n".join(lines).rstrip() + "\n" + + +def render_html(summary, links): + unit, functional = summary["unit"], summary["functional"] + combined = summary["combined_line"] + th = "text-align:left;background:#f2f4f7;border:1px solid #d0d5dd;padding:6px 10px" + td = "border:1px solid #d0d5dd;padding:6px 10px" + + def row(label, section, bold=False): + cells = [cell(section, kind) for kind in ("line", "branch", "method")] + style = td + (";font-weight:bold" if bold else "") + return (f'{html.escape(label)}' + + "".join(f'{html.escape(value)}' for value in cells) + + "") + + parts = [f'

{html.escape(summary["title"])}

', + '', + f'' + f'', + row("Unit tests", unit), row("Functional (Marvin) tests", functional), + f'' + f'' + f'', + "
LineBranchMethod
Combined' + f'{html.escape(fmt(combined["percent"]) if combined else "not available")}
"] + if summary["least_covered"]: + parts += ['

Least covered files ' + '(lines not covered by either run):

', + '', + f'' + f'' + f''] + for item in summary["least_covered"]: + name = item["file"].replace("org/apache/cloudstack/storage/", "") + parts.append( + f'' + f'' + f'' + f'' + f'') + parts.append("
FileLinesUnitFunctionalCombined
{html.escape(name)}{item["lines"]}{html.escape(fmt(item["unit"]))}{html.escape(fmt(item["functional"]))}{html.escape(fmt(item["combined"]))}
") + for note in summary["notes"]: + parts.append(f'

{html.escape(note)}

') + link_html = " | ".join( + f'{html.escape(label)}' + for label, url in links if url) + if link_html: + parts.append(f'

{link_html}

') + return "\n".join(parts) + "\n" + + +def parse_args(argv): + parser = argparse.ArgumentParser( + description="Summarise ONTAP plugin unit and functional test coverage.") + parser.add_argument("--unit", help="JaCoCo XML report of the unit tests") + parser.add_argument("--functional", help="JaCoCo XML report of the functional tests") + parser.add_argument("--output-dir", required=True, type=Path) + parser.add_argument("--top", type=int, default=10, + help="number of least covered files to list") + parser.add_argument("--unit-link", default="", help="URL of the unit coverage report") + parser.add_argument("--functional-link", default="", + help="URL of the functional coverage report") + return parser.parse_args(argv) + + +def main(argv=None): + args = parse_args(argv) + notes = [] + unit = parse_report(args.unit) if args.unit else None + functional = parse_report(args.functional) if args.functional else None + if unit is None: + notes.append("Unit-test coverage was not produced by this build.") + if functional is None: + notes.append("Functional-test coverage was not produced by this run.") + summary = build_summary(unit, functional, args.top, notes) + links = [("Unit coverage report", args.unit_link), + ("Functional coverage report", args.functional_link)] + args.output_dir.mkdir(parents=True, exist_ok=True) + (args.output_dir / "summary.json").write_text( + json.dumps(summary, indent=2), encoding="utf-8") + (args.output_dir / "summary.md").write_text( + render_markdown(summary, links), encoding="utf-8") + (args.output_dir / "summary.html").write_text( + render_html(summary, links), encoding="utf-8") + combined = summary["combined_line"] + print("ONTAP coverage: unit line %s, functional line %s, combined line %s" % ( + cell(summary["unit"], "line"), cell(summary["functional"], "line"), + fmt(combined["percent"]) if combined else "not available")) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/private-cicd/scripts/test_generate_presubmit_report.py b/private-cicd/scripts/test_generate_presubmit_report.py index 7edcb90f060b..b35eeac0dba4 100644 --- a/private-cicd/scripts/test_generate_presubmit_report.py +++ b/private-cicd/scripts/test_generate_presubmit_report.py @@ -140,6 +140,22 @@ def test_long_error_is_truncated(self): self.assertEqual(len(error), report.MAX_ERROR_CHARS + 1) +class CoverageSectionTest(unittest.TestCase): + def test_summary_fragment_is_included_when_present(self): + with tempfile.TemporaryDirectory() as directory: + results = Path(directory) + (results / "coverage").mkdir() + (results / "coverage" / "summary.html").write_text( + "

ONTAP storage plugin test coverage

", encoding="utf-8") + + self.assertIn("ONTAP storage plugin test coverage", + report.coverage_section(results)) + + def test_no_summary_gives_an_empty_section(self): + with tempfile.TemporaryDirectory() as directory: + self.assertEqual("", report.coverage_section(Path(directory))) + + class IstTimeTest(unittest.TestCase): def test_utc_time_is_shown_in_ist(self): value = report.parse_time("2026-11-05T01:54:07Z") diff --git a/private-cicd/scripts/test_ontap_coverage.py b/private-cicd/scripts/test_ontap_coverage.py new file mode 100644 index 000000000000..6b4e7d6fd6e2 --- /dev/null +++ b/private-cicd/scripts/test_ontap_coverage.py @@ -0,0 +1,155 @@ +#!/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 ontap-coverage.py.""" + +import importlib.util +import json +import tempfile +import unittest +from pathlib import Path + + +SCRIPT = Path(__file__).with_name("ontap-coverage.py") +SPEC = importlib.util.spec_from_file_location("ontap_coverage", SCRIPT) +MODULE = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(MODULE) + +DOCTYPE = ('' + '') + + +def report(files, line_missed, line_covered): + """A JaCoCo report. files maps a name to {line number: covered?}.""" + sources = "" + for name, lines in files.items(): + package, _, source = name.rpartition("/") + entries = "".join( + f'' + for nr, hit in lines.items()) + sources += (f'' + f"{entries}") + return (f'{DOCTYPE}{sources}' + f'' + '' + '' + '') + + +A = "org/apache/cloudstack/storage/service/A.java" +B = "org/apache/cloudstack/storage/driver/B.java" + + +def write(directory, name, text): + path = Path(directory) / name + path.write_text(text, encoding="utf-8") + return path + + +class CoverageTest(unittest.TestCase): + def setUp(self): + self.directory = tempfile.TemporaryDirectory() + self.addCleanup(self.directory.cleanup) + + def reports(self): + unit = write(self.directory.name, "unit.xml", + report({A: {1: True, 2: True, 3: False, 4: False}, B: {1: False}}, 3, 2)) + functional = write(self.directory.name, "functional.xml", + report({A: {1: False, 2: False, 3: True, 4: False}, B: {1: True}}, 3, 2)) + return unit, functional + + def test_parse_reads_totals_and_lines(self): + unit, _ = self.reports() + parsed = MODULE.parse_report(unit) + + self.assertEqual((3, 2), parsed["totals"]["LINE"]) + self.assertEqual((3, 1), parsed["totals"]["BRANCH"]) + self.assertEqual((0, 1), parsed["files"][A]["lines"][1]) + self.assertEqual((1, 0), parsed["files"][A]["lines"][3]) + + def test_combined_coverage_is_the_union_of_covered_lines(self): + unit, functional = self.reports() + summary = MODULE.build_summary( + MODULE.parse_report(unit), MODULE.parse_report(functional), 10, []) + + # A: lines 1,2 (unit) + 3 (functional) of 4; B: 1 (functional) of 1. + self.assertEqual(4, summary["combined_line"]["covered"]) + self.assertEqual(1, summary["combined_line"]["missed"]) + self.assertEqual(80.0, summary["combined_line"]["percent"]) + self.assertEqual(40.0, summary["unit"]["line"]["percent"]) + rows = {row["file"]: row for row in summary["least_covered"]} + self.assertEqual(75.0, rows[A]["combined"]) + self.assertEqual(50.0, rows[A]["unit"]) + self.assertEqual(25.0, rows[A]["functional"]) + self.assertNotIn(B, rows) # fully covered files are not listed + + def test_one_missing_report_still_summarises_the_other(self): + unit, _ = self.reports() + summary = MODULE.build_summary(MODULE.parse_report(unit), None, 10, []) + + self.assertIsNone(summary["functional"]) + self.assertEqual(40.0, summary["combined_line"]["percent"]) + + def test_missing_or_broken_report_is_not_a_report(self): + broken = write(self.directory.name, "broken.xml", "") + other = write(self.directory.name, "other.xml", "") + + self.assertIsNone(MODULE.parse_report(Path(self.directory.name) / "none.xml")) + self.assertIsNone(MODULE.parse_report(broken)) + self.assertIsNone(MODULE.parse_report(other)) + + def test_main_writes_json_markdown_and_html(self): + unit, functional = self.reports() + out = Path(self.directory.name) / "out" + code = MODULE.main(["--unit", str(unit), "--functional", str(functional), + "--output-dir", str(out), "--unit-link", "https://j/u"]) + + self.assertEqual(0, code) + summary = json.loads((out / "summary.json").read_text(encoding="utf-8")) + self.assertEqual(80.0, summary["combined_line"]["percent"]) + markdown = (out / "summary.md").read_text(encoding="utf-8") + self.assertTrue(markdown.startswith(MODULE.COMMENT_MARKER)) + self.assertIn("| **Combined** | **80.0%** |", markdown) + self.assertIn("[Unit coverage report](https://j/u)", markdown) + page = (out / "summary.html").read_text(encoding="utf-8") + self.assertIn("Combined", page) + self.assertIn('href="https://j/u"', page) + + def test_main_with_no_reports_explains_why(self): + out = Path(self.directory.name) / "out" + MODULE.main(["--unit", "/nonexistent.xml", "--output-dir", str(out)]) + + summary = json.loads((out / "summary.json").read_text(encoding="utf-8")) + self.assertIsNone(summary["combined_line"]) + self.assertEqual(2, len(summary["notes"])) + self.assertIn("not available", (out / "summary.md").read_text(encoding="utf-8")) + + def test_file_names_are_escaped_in_html(self): + unit = write(self.directory.name, "unit.xml", + report({"p/<b>.java": {1: False}}, 1, 0)) # XML-escaped name + out = Path(self.directory.name) / "out" + MODULE.main(["--unit", str(unit), "--output-dir", str(out)]) + + page = (out / "summary.html").read_text(encoding="utf-8") + self.assertNotIn(".java", page) + self.assertIn("<b>.java", page) + + +if __name__ == "__main__": + unittest.main() From 10a31cc4aed58dd1be63bae985d8ee2f781db7cd Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 12 Nov 2026 18:48:53 +0530 Subject: [PATCH 18/19] Collect functional-test coverage of the ONTAP plugin from the lab VM. The plugin runs in the management server, so the JaCoCo agent is attached to its JVM before the first start. marvin-run.sh dumps the data after the iSCSI and the NFS3 suites. coverage-report.sh merges the dumps and builds the report against the plugin jar in the deployed management package. Every step degrades on its own and never fails the integration run. --- .../scripts/configure-cloudstack-vm.sh | 19 ++++ private-cicd/scripts/coverage-report.sh | 105 ++++++++++++++++++ private-cicd/scripts/fetch-jacoco.sh | 62 +++++++++++ private-cicd/scripts/marvin-run.sh | 20 ++++ private-cicd/scripts/run-phase2-remote.sh | 15 ++- private-cicd/scripts/run-phase2-vm.sh | 14 +++ 6 files changed, 233 insertions(+), 2 deletions(-) create mode 100755 private-cicd/scripts/coverage-report.sh create mode 100755 private-cicd/scripts/fetch-jacoco.sh diff --git a/private-cicd/scripts/configure-cloudstack-vm.sh b/private-cicd/scripts/configure-cloudstack-vm.sh index 24d9746568c8..7aeeee85881d 100755 --- a/private-cicd/scripts/configure-cloudstack-vm.sh +++ b/private-cicd/scripts/configure-cloudstack-vm.sh @@ -428,6 +428,25 @@ until systemctl is-active --quiet libvirtd; do sleep 1 libvirtd_wait=$((libvirtd_wait + 1)) done +# Functional-test coverage: the ONTAP plugin runs inside the management server +# JVM, so the JaCoCo agent is attached to it before its first start. A problem +# here only loses the coverage data, it never fails the integration run. +if [[ -n "${JACOCO_AGENT_JAR:-}" ]]; then + defaults_file=/etc/default/cloudstack-management + if [[ ! -f "$JACOCO_AGENT_JAR" ]]; then + echo "JACOCO_AGENT_JAR $JACOCO_AGENT_JAR does not exist; functional coverage is off." >&2 + elif ! grep -q '^JAVA_OPTS="' "$defaults_file"; then + echo "No JAVA_OPTS line in $defaults_file; functional coverage is off." >&2 + elif grep -q 'jacocoagent' "$defaults_file"; then + echo "The JaCoCo agent is already configured in $defaults_file." + else + install -d -m 755 /opt/jacoco + install -m 644 "$JACOCO_AGENT_JAR" /opt/jacoco/jacocoagent.jar + agent_option="-javaagent:/opt/jacoco/jacocoagent.jar=output=tcpserver,address=127.0.0.1,port=${COVERAGE_PORT:-36320},includes=org.apache.cloudstack.storage.*" + sed -i "s|^JAVA_OPTS=\"\(.*\)\"\$|JAVA_OPTS=\"\1 ${agent_option}\"|" "$defaults_file" + echo "JaCoCo agent added to the management server JAVA_OPTS." + fi +fi db_trace_state "before first start of cloudstack-management" systemctl start cloudstack-management systemctl restart cloudstack-agent diff --git a/private-cicd/scripts/coverage-report.sh b/private-cicd/scripts/coverage-report.sh new file mode 100755 index 000000000000..65757c3fdea3 --- /dev/null +++ b/private-cicd/scripts/coverage-report.sh @@ -0,0 +1,105 @@ +#!/usr/bin/env bash +# 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. + +# Purpose: +# Builds the ONTAP storage plugin coverage summary of one presubmit run. The +# unit-test JaCoCo report already exists, written by the Maven build. The +# functional report is made here from the management server dumps that +# marvin-run.sh collected on the lab VM, using the plugin jar from the +# management package that was actually deployed. ontap-coverage.py then +# writes summary.json, summary.md and summary.html. +# This script never fails the presubmit: every problem is reported and the +# summary says which part of the coverage is missing. +# Dependencies: +# RESULTS_DIR — the presubmit results directory (required). Reads +# unit-tests/coverage/unit/jacoco.xml and phase2/coverage/*.exec, writes +# coverage/. +# SOURCE_DIR — CloudStack source checkout (required): ONTAP sources. +# Optional: DEB_DIR (default $SOURCE_DIR/dist/deb-all) for the deployed jar, +# BUILD_URL to link the reports. +# Commands: java, python3, dpkg-deb; fetch-jacoco.sh for +# the JaCoCo command line. +# Calls: fetch-jacoco.sh, ontap-coverage.py. + +set -uo pipefail + +RESULTS_DIR="${RESULTS_DIR:?Set RESULTS_DIR}" +SOURCE_DIR="${SOURCE_DIR:?Set SOURCE_DIR}" +DEB_DIR="${DEB_DIR:-$SOURCE_DIR/dist/deb-all}" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +ontap_module="plugins/storage/volume/ontap" +out_dir="$RESULTS_DIR/coverage" +unit_xml="$RESULTS_DIR/unit-tests/coverage/unit/jacoco.xml" +exec_dir="$RESULTS_DIR/phase2/coverage" +work_dir="$(mktemp -d)" +trap 'rm -rf "$work_dir"' EXIT +mkdir -p "$out_dir" + +functional_xml="" +shopt -s nullglob +exec_files=("$exec_dir"/*.exec) +shopt -u nullglob + +if [[ "${#exec_files[@]}" -eq 0 ]]; then + echo "No functional coverage dumps under $exec_dir." +elif ! "$SCRIPT_DIR/fetch-jacoco.sh" "$work_dir/jacoco" >/dev/null 2>&1; then + echo "Could not fetch the JaCoCo command line; functional coverage is skipped." >&2 +else + cli="$work_dir/jacoco/jacococli.jar" + classfiles="" + # The jar inside the deployed management package is the code that ran. + management_deb="$(find "$DEB_DIR" -maxdepth 1 -name 'cloudstack-management_*.deb' -print -quit 2>/dev/null)" + if [[ -n "$management_deb" ]] && dpkg-deb -x "$management_deb" "$work_dir/deployed" 2>/dev/null; then + classfiles="$(find "$work_dir/deployed" -name 'cloud-plugin-storage-volume-ontap-*.jar' \ + ! -name '*-sources.jar' ! -name '*-tests.jar' -print -quit)" + fi + if [[ -z "$classfiles" && -d "$SOURCE_DIR/$ontap_module/target/classes" ]]; then + classfiles="$SOURCE_DIR/$ontap_module/target/classes" + echo "Deployed plugin jar not found; using $classfiles." + fi + if [[ -z "$classfiles" ]]; then + echo "No ONTAP plugin classes found; functional coverage is skipped." >&2 + elif mkdir -p "$out_dir/functional" \ + && java -jar "$cli" merge "${exec_files[@]}" --destfile "$work_dir/functional.exec" >/dev/null \ + && java -jar "$cli" report "$work_dir/functional.exec" \ + --classfiles "$classfiles" \ + --sourcefiles "$SOURCE_DIR/$ontap_module/src/main/java" \ + --name "ONTAP functional tests" \ + --xml "$out_dir/functional/jacoco.xml" \ + --csv "$out_dir/functional/jacoco.csv" \ + --html "$out_dir/functional/html" >/dev/null; then + functional_xml="$out_dir/functional/jacoco.xml" + echo "Functional coverage report: $functional_xml" + else + echo "Could not build the functional coverage report." >&2 + fi +fi + +summary_args=(--output-dir "$out_dir") +[[ -f "$unit_xml" ]] && summary_args+=(--unit "$unit_xml") +[[ -n "$functional_xml" ]] && summary_args+=(--functional "$functional_xml") +if [[ -n "${BUILD_URL:-}" ]]; then + [[ -f "$unit_xml" ]] && summary_args+=( + --unit-link "${BUILD_URL}artifact/presubmit-results/unit-tests/coverage/unit/index.html") + [[ -n "$functional_xml" ]] && summary_args+=( + --functional-link "${BUILD_URL}artifact/presubmit-results/coverage/functional/html/index.html") +fi +python3 "$SCRIPT_DIR/ontap-coverage.py" "${summary_args[@]}" \ + || echo "Could not write the coverage summary." >&2 +exit 0 diff --git a/private-cicd/scripts/fetch-jacoco.sh b/private-cicd/scripts/fetch-jacoco.sh new file mode 100755 index 000000000000..7f73d049ea91 --- /dev/null +++ b/private-cicd/scripts/fetch-jacoco.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +# 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. + +# Purpose: +# Copies the JaCoCo agent and command-line jars into a directory as +# jacocoagent.jar and jacococli.jar. The agent is attached to the management +# server on the lab VM, and the command line dumps and reports coverage. +# Jars already present in the directory are kept, so a second call is free. +# Dependencies: +# Usage: fetch-jacoco.sh DEST_DIR +# Optional: JACOCO_VERSION (default 0.8.11, the version pinned in the root pom). +# Commands: mvn with access to Maven Central or a mirror. +# Called by: run-phase2-remote.sh, coverage-report.sh. + +set -euo pipefail + +DEST_DIR="${1:?Usage: fetch-jacoco.sh DEST_DIR}" +JACOCO_VERSION="${JACOCO_VERSION:-0.8.11}" +DEPENDENCY_PLUGIN="org.apache.maven.plugins:maven-dependency-plugin:3.6.1:copy" + +mkdir -p "$DEST_DIR" +DEST_DIR="$(cd "$DEST_DIR" && pwd)" +if [[ -f "$DEST_DIR/jacocoagent.jar" && -f "$DEST_DIR/jacococli.jar" ]]; then + exit 0 +fi + +work_dir="$(mktemp -d)" +trap 'rm -rf "$work_dir"' EXIT + +# The agent is the "runtime" classifier of org.jacoco.agent; the command line is +# the self-contained "nodeps" jar of org.jacoco.cli. +mvn -B -q "$DEPENDENCY_PLUGIN" \ + "-Dartifact=org.jacoco:org.jacoco.agent:${JACOCO_VERSION}:jar:runtime" \ + "-DoutputDirectory=${work_dir}/agent" -Dmdep.stripVersion=true +mvn -B -q "$DEPENDENCY_PLUGIN" \ + "-Dartifact=org.jacoco:org.jacoco.cli:${JACOCO_VERSION}:jar:nodeps" \ + "-DoutputDirectory=${work_dir}/cli" -Dmdep.stripVersion=true + +agent_jar="$(find "$work_dir/agent" -name '*.jar' -print -quit)" +cli_jar="$(find "$work_dir/cli" -name '*.jar' -print -quit)" +if [[ -z "$agent_jar" || -z "$cli_jar" ]]; then + echo "Could not fetch the JaCoCo ${JACOCO_VERSION} jars" >&2 + exit 1 +fi +cp "$agent_jar" "$DEST_DIR/jacocoagent.jar" +cp "$cli_jar" "$DEST_DIR/jacococli.jar" +echo "JaCoCo ${JACOCO_VERSION} jars are in $DEST_DIR" diff --git a/private-cicd/scripts/marvin-run.sh b/private-cicd/scripts/marvin-run.sh index 9e2996a9c74d..8c375ebdc5a5 100755 --- a/private-cicd/scripts/marvin-run.sh +++ b/private-cicd/scripts/marvin-run.sh @@ -58,6 +58,24 @@ run_suite() { return "$status" } +# Functional-test coverage: dump (and reset) the JaCoCo data of the management +# server after a suite, so a later restart cannot lose it. The data is only +# collected when the agent was attached; a failed dump never fails the run. +dump_coverage() { + local name="$1" + if [[ -z "${JACOCO_CLI:-}" || ! -f "$JACOCO_CLI" || -z "${COVERAGE_DIR:-}" ]]; then + return 0 + fi + mkdir -p "$COVERAGE_DIR" + if java -jar "$JACOCO_CLI" dump --address 127.0.0.1 \ + --port "${COVERAGE_PORT:-36320}" --reset \ + --destfile "$COVERAGE_DIR/${name}.exec"; then + echo "Functional coverage for ${name}: $COVERAGE_DIR/${name}.exec" + else + echo "Could not dump functional coverage for ${name}" >&2 + fi +} + echo "==> Creating and validating the CloudStack zone" if ! run_suite setup_zone; then echo "Zone setup failed; protocol suites cannot run" >&2 @@ -67,9 +85,11 @@ fi suite_status=0 echo "==> Running ONTAP iSCSI suites" run_suite iscsi || suite_status=1 +dump_coverage iscsi echo "==> Running ONTAP NFS3 suites" run_suite nfs3 || suite_status=1 +dump_coverage nfs3 ontap_results=test/integration/plugins/ontap/results if [[ -d "$ontap_results" ]]; then diff --git a/private-cicd/scripts/run-phase2-remote.sh b/private-cicd/scripts/run-phase2-remote.sh index 1669cec166ac..24ca017a9ab0 100755 --- a/private-cicd/scripts/run-phase2-remote.sh +++ b/private-cicd/scripts/run-phase2-remote.sh @@ -105,6 +105,17 @@ fi mkdir -p "$assets_root/test/integration/plugins" "$assets_root/marvin" cp -a "$SOURCE_DIR/test/integration/plugins/ontap" "$assets_root/test/integration/plugins/" cp "${marvin_archives[0]}" "$assets_root/marvin/" + +# Functional-test coverage needs the JaCoCo agent and command line on the VM. +# If they cannot be fetched the run goes on without coverage. +functional_coverage="${FUNCTIONAL_COVERAGE:-false}" +if [[ "$functional_coverage" == "true" ]]; then + if ! "$SCRIPT_DIR/fetch-jacoco.sh" "$assets_root/jacoco"; then + echo "Could not fetch the JaCoCo jars; functional coverage is off." >&2 + rm -rf "$assets_root/jacoco" + functional_coverage=false + fi +fi tar -czf "$assets_archive" -C "$assets_root" . remote="${VM_SSH_USER}@${VM_HOST}" @@ -189,8 +200,8 @@ ssh "${ssh_options[@]}" "$remote" \ chmod +x '$remote_scripts/'*.sh" printf -v remote_command \ - 'PHASE2_ROOT=%q BRIDGE=%q %q/run-phase2-vm.sh' \ - "$remote_root" "$BRIDGE" "$remote_scripts" + 'PHASE2_ROOT=%q BRIDGE=%q FUNCTIONAL_COVERAGE=%q %q/run-phase2-vm.sh' \ + "$remote_root" "$BRIDGE" "$functional_coverage" "$remote_scripts" set +e ssh "${ssh_options[@]}" "$remote" "$remote_command" diff --git a/private-cicd/scripts/run-phase2-vm.sh b/private-cicd/scripts/run-phase2-vm.sh index e24624b59e67..47aee947e698 100755 --- a/private-cicd/scripts/run-phase2-vm.sh +++ b/private-cicd/scripts/run-phase2-vm.sh @@ -68,10 +68,22 @@ on_exit() { } trap on_exit EXIT +# Functional-test coverage is on only when it was requested and the pipeline +# shipped the JaCoCo jars. Empty values leave it off. +coverage_agent="" +coverage_cli="" +if [[ "${FUNCTIONAL_COVERAGE:-false}" == "true" \ + && -f "$source_dir/jacoco/jacocoagent.jar" \ + && -f "$source_dir/jacoco/jacococli.jar" ]]; then + coverage_agent="$source_dir/jacoco/jacocoagent.jar" + coverage_cli="$source_dir/jacoco/jacococli.jar" +fi + DEB_DIR="$deb_dir" \ SECRETS_FILE="$secrets_file" \ ONTAP_CONFIG="$ontap_config" \ BRIDGE="$BRIDGE" \ +JACOCO_AGENT_JAR="$coverage_agent" \ "$scripts_dir/configure-cloudstack-vm.sh" \ 2>&1 | tee "$result_dir/configure-cloudstack.log" @@ -84,4 +96,6 @@ BRIDGE="$BRIDGE" \ CLOUDSTACK_DIR="$source_dir" \ PHASE2_RESULT_DIR="$result_dir/marvin" \ MARVIN_ARCHIVE="$marvin_archive" \ +JACOCO_CLI="$coverage_cli" \ +COVERAGE_DIR="$result_dir/coverage" \ "$scripts_dir/marvin-run.sh" From b5f778b4def12f5cd854b9072a5f8b31857dba90 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 12 Nov 2026 18:48:53 +0530 Subject: [PATCH 19/19] Publish ONTAP plugin coverage in the Check and as one PR comment. The coverage summary is built before the report so the mail carries it, then added to the Check summary and posted as a single comment that later runs update in place. PRESUBMIT_COVERAGE=false turns the feature off and PRESUBMIT_COVERAGE_COMMENT=false keeps only the comment off. The comment step fails on its own if the App lacks write access, without changing the result. --- private-cicd/Jenkinsfile | 64 +++++++++++++++- private-cicd/docs/PRIVATE-CICD-GUIDE.md | 31 ++++++++ private-cicd/scripts/github-check-run.sh | 94 +++++++++++++++++++++++- 3 files changed, 183 insertions(+), 6 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 15c4e7cbe432..5bb4d8277dfa 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -545,11 +545,63 @@ void createGithubCheck() { } } +// Builds the ONTAP plugin coverage summary from the unit-test report written by +// the Maven build and the functional dumps collected from the lab VM. It never +// fails the build: a missing part is named in the summary. Returns the path of +// the markdown summary, or '' when there is none. +String buildCoverageSummary() { + if ((env.PRESUBMIT_COVERAGE ?: 'true') == 'false' || + !fileExists('private-cicd/scripts/coverage-report.sh')) { + return '' + } + try { + withEnv([ + "RESULTS_DIR=${env.WORKSPACE}/${env.PRESUBMIT_RESULTS}", + "SOURCE_DIR=${env.WORKSPACE}/${env.SOURCE_DIR}" + ]) { + sh 'private-cicd/scripts/coverage-report.sh' + } + } catch (Exception e) { + echo "Unable to build the coverage summary: ${e}" + } + def summary = "${env.PRESUBMIT_RESULTS}/coverage/summary.md" + return fileExists(summary) ? summary : '' +} + +// Creates or updates the single coverage comment on the pull request. Turn it +// off with PRESUBMIT_COVERAGE_COMMENT=false. The App needs Pull requests or +// Issues write; without it only this step fails and the build is unchanged. +void postCoverageComment(String summaryFile) { + if (!summaryFile || !hasGithubAppCredential() || + sourceValue('SOURCE_MODE') != 'pull_request' || + (env.PRESUBMIT_COVERAGE_COMMENT ?: 'true') == 'false') { + return + } + try { + withCredentials([ + usernamePassword( + credentialsId: params.GITHUB_APP_CREDENTIALS_ID, + usernameVariable: 'GITHUB_APP', + passwordVariable: 'GITHUB_ACCESS_TOKEN' + ) + ]) { + withEnv(githubEnvironment() + [ + "GITHUB_COVERAGE_FILE=${env.WORKSPACE}/${summaryFile}" + ]) { + sh 'private-cicd/scripts/github-check-run.sh coverage-comment' + } + } + } catch (Exception e) { + echo "Coverage comment not posted; the presubmit result is unchanged: ${e}" + } +} + // Publishes the final result. When the run could not be created at the start // (no run ID), a completed check run is created instead, so the result still // reaches the pull request. This matters once the check is required: a pull // request without a reported result stays blocked. -void completeGithubCheck(String conclusion, String failedStage = '', String reportUrl = '') { +void completeGithubCheck(String conclusion, String failedStage = '', String reportUrl = '', + String coverageFile = '') { if (!hasGithubAppCredential() || sourceValue('SOURCE_MODE') != 'pull_request') { return } @@ -565,7 +617,8 @@ void completeGithubCheck(String conclusion, String failedStage = '', String repo withEnv(githubEnvironment() + [ "GITHUB_CHECK_CONCLUSION=${conclusion}", "GITHUB_CHECK_FAILED_STAGE=${failedStage}", - "GITHUB_CHECK_REPORT_URL=${reportUrl}" + "GITHUB_CHECK_REPORT_URL=${reportUrl}", + "GITHUB_CHECK_COVERAGE_FILE=${coverageFile}" ]) { if (runId) { sh 'private-cicd/scripts/github-check-run.sh complete' @@ -1285,6 +1338,7 @@ void runUnitTests() { ENABLE_SIMULATOR=false \ MAVEN_THREADS=1C \ RERUN_FAILING_TESTS=2 \ + ONTAP_COVERAGE="${PRESUBMIT_COVERAGE:-true}" \ MAVEN_OPTS="-Xms1g -Xmx8g" \ private-cicd/scripts/mvn-full.sh ''' @@ -1407,6 +1461,7 @@ void deployAndRunOntapIntegration() { SOURCE_ID="$RESOLVED_SOURCE_ID" \ SOURCE_SHA="$RESOLVED_SOURCE_SHA" \ BRIDGE="$VM_BRIDGE" \ + FUNCTIONAL_COVERAGE="${PRESUBMIT_COVERAGE:-true}" \ private-cicd/scripts/run-phase2-remote.sh ''' } @@ -1450,6 +1505,8 @@ void publishPresubmitResults() { } def sourceName = sourceValue('SOURCE_MODE') == 'branch' ? "branch ${params.SOURCE_BRANCH}" : "PR-${sourceValue('PR_ID')}" + // Before the report, which embeds the coverage summary. + def coverageFile = buildCoverageSummary() try { if (fileExists('private-cicd/scripts/generate-presubmit-report.py')) { withEnv([ @@ -1499,7 +1556,8 @@ void publishPresubmitResults() { // name the failed stage and link the report. def reportUrl = fileExists("${env.PRESUBMIT_RESULTS}/report.html") ? "${env.BUILD_URL}artifact/${env.PRESUBMIT_RESULTS}/report.html" : '' - completeGithubCheck(conclusion, failedStage, reportUrl) + completeGithubCheck(conclusion, failedStage, reportUrl, coverageFile) + postCoverageComment(coverageFile) def finalSubject = failedStage ? "${presubmitSubjectBase()}: ${result} in ${failedStage}" : "${presubmitSubjectBase()}: ${result}" diff --git a/private-cicd/docs/PRIVATE-CICD-GUIDE.md b/private-cicd/docs/PRIVATE-CICD-GUIDE.md index 03fd20622f7a..e7576b09241f 100644 --- a/private-cicd/docs/PRIVATE-CICD-GUIDE.md +++ b/private-cicd/docs/PRIVATE-CICD-GUIDE.md @@ -569,6 +569,37 @@ How the Check behaves once it is required: Check above is the only gate. - A pull request from a fork never gets a Check, so it cannot merge while the Check is required. +### ONTAP plugin test coverage + +Every run reports the coverage of the ONTAP storage plugin +(`plugins/storage/volume/ontap`) from two sources, in the mail, in `report.html`, in the summary of the +`cloudstack-ontap-presubmit` Check, and in one PR comment that later runs update in place. + +| Source | How it is produced | +| --- | --- | +| Unit tests | `mvn-full.sh` passes `-Dontap.coverage=true`, which activates the `ontap-coverage` profile in the plugin's `pom.xml`. The same Maven build that runs the tests writes `target/site/jacoco/jacoco.xml`. `mvn-full.sh` copies it to `presubmit-results/unit-tests/coverage/unit/`. | +| Functional (Marvin) tests | `run-phase2-remote.sh` fetches the JaCoCo agent and command line (`fetch-jacoco.sh`, version 0.8.11) and ships them to the VM. `configure-cloudstack-vm.sh` adds `-javaagent:...,output=tcpserver,address=127.0.0.1,port=36320` to the management server `JAVA_OPTS` before its first start. `marvin-run.sh` dumps and resets the data after the iSCSI suites and again after the NFS3 suites into `phase2/coverage/*.exec`. | +| Summary | `coverage-report.sh` merges the dumps and builds a report from the plugin jar inside the deployed management package, so the report matches the code that ran. `ontap-coverage.py` then writes `coverage/summary.{json,md,html}`. | + +Notes: + +- The **combined** figure is line coverage over the union of both runs, taken line by line from the two reports, so + it does not depend on the two reports agreeing on class IDs. Branch and method figures are shown per source only. +- Only the management server is instrumented, which is where the ONTAP plugin runs. The KVM agent is not. +- The coverage is informational. It never changes the build result or the Check conclusion. +- Every part degrades on its own. If the JaCoCo jars cannot be fetched, the agent cannot be attached, a dump fails or + the report cannot be built, the run goes on and the summary says which source is missing. +- A management server restart between two dumps loses the data collected since the last dump. +- The JaCoCo HTML under `coverage/functional/html` and `unit-tests/coverage/unit` is linked for detail, but Jenkins + blocks the scripts and styles of archived HTML, so those pages look plain. The tables in the mail and the PR come + from the summary instead. +- `PRESUBMIT_COVERAGE=false` (a job, folder or global environment variable) turns the whole feature off. + `PRESUBMIT_COVERAGE_COMMENT=false` keeps the summary in the mail and the Check but stops the PR comment. +- The PR comment needs **Pull requests: Read and write** (or **Issues: Read and write**) on the GitHub App. Without + it only the comment step fails; the log says so and the build is unchanged. The comment is found by the marker + `` among comments written by a bot, so it does not touch the `coverage-grade-bot` comment + that the repository's own workflow posts. + ## 13. Manual branch job and exact SHA Create a separate Pipeline-from-SCM job: diff --git a/private-cicd/scripts/github-check-run.sh b/private-cicd/scripts/github-check-run.sh index 62625ce6ef54..b5161b0fc4cb 100755 --- a/private-cicd/scripts/github-check-run.sh +++ b/private-cicd/scripts/github-check-run.sh @@ -22,6 +22,8 @@ # that run with a conclusion, commit-email prints the head commit author # address used for presubmit mail, and diagnose probes the App's token type, # repository access and Checks permissions to explain a refused check run. +# coverage-comment creates or updates the single pull-request comment that +# shows the ONTAP plugin coverage (needs Pull requests or Issues write). # Dependencies: # Commands: curl, python3 (stdlib), mktemp. # GITHUB_ACCESS_TOKEN, GITHUB_REPOSITORY — required by every subcommand; the @@ -33,7 +35,10 @@ # GITHUB_CHECK_CONCLUSION publishes an already completed check run, which is # how a result is still reported when the run could not be created at the # start. GITHUB_CHECK_FAILED_STAGE and GITHUB_CHECK_REPORT_URL add the failed -# stage and the report link to the check's summary. +# stage and the report link to the check's summary, and GITHUB_CHECK_COVERAGE_FILE +# appends the coverage summary (markdown) to it. coverage-comment needs PR_ID +# and GITHUB_COVERAGE_FILE, the markdown file whose first line is the marker +# that identifies the comment. # Called by the Jenkinsfile. Does not call other private-cicd scripts. set -euo pipefail @@ -181,11 +186,19 @@ else: lines.append(f"Jenkins build and logs: {build_url}") if env("GITHUB_CHECK_REPORT_URL"): lines.append(f"Test report: {env('GITHUB_CHECK_REPORT_URL')}") + summary = "\n\n".join(lines) + coverage_file = env("GITHUB_CHECK_COVERAGE_FILE") + if coverage_file and os.path.isfile(coverage_file): + with open(coverage_file, encoding="utf-8") as handle: + coverage = handle.read().replace("", "").strip() + if coverage: + summary += "\n\n" + coverage + # GitHub rejects a summary above 65535 characters. payload["status"] = "completed" payload["conclusion"] = conclusion payload["output"] = { "title": f"CloudStack ONTAP presubmit {verdict}", - "summary": "\n\n".join(lines), + "summary": summary[:60000], } with open(sys.argv[1], "w", encoding="utf-8") as output: json.dump(payload, output) @@ -386,6 +399,78 @@ print(payload.get("commit", {}).get("author", {}).get("email") or "") PY } +# Creates the coverage comment on the pull request, or updates the one a +# previous run left, so a PR carries a single coverage comment. The comment is +# found by the marker on its first line, among comments written by a bot. +coverage_comment() { + require_env GITHUB_ACCESS_TOKEN GITHUB_REPOSITORY PR_ID GITHUB_COVERAGE_FILE + local comments payload response comment_id page + comments="$(mktemp)" + payload="$(mktemp)" + response="$(mktemp)" + trap "rm -f '$comments' '$payload' '$response'" EXIT + + python3 - "$GITHUB_COVERAGE_FILE" "$payload" <<'PY' +import json +import sys + +with open(sys.argv[1], encoding="utf-8") as source: + body = source.read() +with open(sys.argv[2], "w", encoding="utf-8") as output: + json.dump({"body": body[:60000]}, output) +PY + + comment_id="" + for page in 1 2 3 4 5; do + github_api GET \ + "/repos/${GITHUB_REPOSITORY}/issues/${PR_ID}/comments?per_page=100&page=${page}" \ + > "$comments" || { + echo "GitHub refused to list the pull request comments:" >&2 + print_github_error "$comments" + return 1 + } + comment_id="$(python3 - "$comments" <<'PY' +import json +import sys + +marker = "" +with open(sys.argv[1], encoding="utf-8") as handle: + items = json.load(handle) +for item in items: + if (item.get("user") or {}).get("type") == "Bot" and marker in (item.get("body") or ""): + print(item["id"]) + break +PY +)" + if [[ -n "$comment_id" ]]; then + break + fi + # A full page means there may be more comments to search. + if [[ "$(python3 -c 'import json,sys; print(len(json.load(open(sys.argv[1]))))' "$comments")" -lt 100 ]]; then + break + fi + done + + if [[ -n "$comment_id" ]]; then + github_api PATCH "/repos/${GITHUB_REPOSITORY}/issues/comments/${comment_id}" \ + "$payload" > "$response" || { + echo "GitHub refused to update the coverage comment:" >&2 + print_github_error "$response" + return 1 + } + echo "Updated coverage comment ${comment_id} on PR-${PR_ID}." + else + github_api POST "/repos/${GITHUB_REPOSITORY}/issues/${PR_ID}/comments" \ + "$payload" > "$response" || { + echo "GitHub refused to create the coverage comment:" >&2 + print_github_error "$response" + echo " The App needs Pull requests or Issues: Read and write." >&2 + return 1 + } + echo "Created coverage comment on PR-${PR_ID}." + fi +} + case "${1:-}" in create) create_check @@ -399,8 +484,11 @@ case "${1:-}" in diagnose) diagnose_app ;; + coverage-comment) + coverage_comment + ;; *) - echo "Usage: $0 {create|complete|commit-email|diagnose}" >&2 + echo "Usage: $0 {create|complete|commit-email|diagnose|coverage-comment}" >&2 exit 2 ;; esac