From 1c4f49a948d9d05160087cdf671cbd9f5641954d Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 13 Oct 2026 08:59:27 +0530 Subject: [PATCH 01/14] 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/14] 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/14] 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/14] 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/14] 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 daf804f1521c705a24044e83a0d846a3813eb468 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 22:09:33 +0530 Subject: [PATCH 06/14] Fix multibranch source identity and document the PR-119 pilot setup. The environment block declared RESOLVED_SOURCE_ID and RESOLVED_SOURCE_SHA from build parameters. A multibranch pull request has no such parameters, and the block wraps every stage, so it shadowed the values set in prepareMultibranchPullRequest(). They read back empty, which caused a NullPointerException in presubmitSubjectBase and an empty SHA in the Maven and deploy shell steps. Set them in resolveSourceIdentity() instead. Also name the trusted CI branch in one place, trustedCiBranch(), and describe the multibranch folder configuration for the pilot. Co-Authored-By: Claude Sonnet 5.5 --- private-cicd/Jenkinsfile | 38 ++++++++++++++++---- private-cicd/docs/CREATE-PRESUBMIT-JOB.md | 44 ++++++++++++++++++----- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 13ba47909786..24f8c173a48a 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -796,7 +796,26 @@ void discoverPullRequests() { } } +// Branch mode leaves PR_ID and PR_HEAD_SHA empty, so scripts receive a +// mode-independent identity. The branch is sanitised because it reaches +// a filesystem path on the target VM. A multibranch pull request has already +// set both values in prepareMultibranchPullRequest(). +void resolveSourceIdentity() { + if (isMultibranchPullRequest()) { + return + } + if (params.SOURCE_MODE == 'branch') { + env.RESOLVED_SOURCE_ID = + 'branch-' + params.SOURCE_BRANCH.replaceAll('[^A-Za-z0-9._-]', '-') + env.RESOLVED_SOURCE_SHA = params.SOURCE_SHA + } else { + env.RESOLVED_SOURCE_ID = 'pr-' + params.PR_ID + env.RESOLVED_SOURCE_SHA = params.PR_HEAD_SHA + } +} + void validateSourceRequest() { + resolveSourceIdentity() sh 'rm -f "${WORKSPACE}@tmp/presubmit-stage-events.tsv"' env.PRESUBMIT_INTEGRATION_STARTED = 'false' env.PRESUBMIT_STARTED_AT = new Date().format( @@ -938,10 +957,16 @@ void abortSupersededRun() { } } +// Branch that supplies the trusted CI scripts. It must equal the branch that +// Remote Jenkinsfile Provider loads this Jenkinsfile from. PR-119 pilot value; +// change it to 'main' when the folder is cut over to every pull request. +String trustedCiBranch() { + return 'feature/CSTACKEX-340' +} + void checkoutCiScripts() { recordStageEvent('Checkout CI scripts', 'STARTED') - // Pilot for PR-119. Change this back to main before the folder includes every PR. - def ciBranch = 'feature/CSTACKEX-340' + def ciBranch = trustedCiBranch() if (env.PRESUBMIT_CI_SHA?.trim() && fileExists('private-cicd/scripts/resolve-pull-request.py')) { echo "CI scripts already checked out from ${ciBranch} at ${env.PRESUBMIT_CI_SHA}." return @@ -1510,11 +1535,10 @@ spec: environment { SOURCE_DIR = 'cloudstack-src' PRESUBMIT_RESULTS = 'presubmit-results' - // Branch mode leaves PR_ID and PR_HEAD_SHA empty, so scripts receive a - // mode-independent identity. The branch is sanitised because it reaches - // a filesystem path on the target VM. - RESOLVED_SOURCE_ID = "${params.SOURCE_MODE == 'branch' ? 'branch-' + params.SOURCE_BRANCH.replaceAll('[^A-Za-z0-9._-]', '-') : 'pr-' + params.PR_ID}" - RESOLVED_SOURCE_SHA = "${params.SOURCE_MODE == 'branch' ? params.SOURCE_SHA : params.PR_HEAD_SHA}" + // RESOLVED_SOURCE_ID and RESOLVED_SOURCE_SHA are deliberately not + // declared here. This block wraps every stage in withEnv, so a value + // declared here shadows the multibranch assignment made later and + // reads back empty. See resolveSourceIdentity(). } stages { diff --git a/private-cicd/docs/CREATE-PRESUBMIT-JOB.md b/private-cicd/docs/CREATE-PRESUBMIT-JOB.md index 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 102dd910e2de31749ff249a4e2f8bb0a66e65bc5 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 22:38:14 +0530 Subject: [PATCH 07/14] Retry failing unit tests twice in the presubmit Maven build. Build 8 of PR-119 failed on the upstream, timing-sensitive ConfigKeyScheduledExecutionWrapperTest, which counts scheduled ticks in a fixed window and jitters in the CPU-limited agent pod. Add an opt-in RERUN_FAILING_TESTS setting to mvn-full.sh that passes -Dsurefire.rerunFailingTestsCount, and set it to 2 in the Jenkinsfile. The default stays 0 for local runs. Co-Authored-By: Claude Sonnet 5.5 --- private-cicd/Jenkinsfile | 1 + private-cicd/scripts/mvn-full.sh | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index 24f8c173a48a..f9610902023e 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -1218,6 +1218,7 @@ void runUnitTests() { SOURCE_SHA="$RESOLVED_SOURCE_SHA" \ ENABLE_SIMULATOR=false \ MAVEN_THREADS=1C \ + RERUN_FAILING_TESTS=2 \ MAVEN_OPTS="-Xms1g -Xmx8g" \ private-cicd/scripts/mvn-full.sh ''' diff --git a/private-cicd/scripts/mvn-full.sh b/private-cicd/scripts/mvn-full.sh index 7aa9821a1d24..605a71ee1ec6 100755 --- a/private-cicd/scripts/mvn-full.sh +++ b/private-cicd/scripts/mvn-full.sh @@ -25,7 +25,9 @@ # RESULT_DIR — if set, SOURCE_ID and SOURCE_SHA are required (Jenkins handoff); # PR_ID and PR_HEAD_SHA are accepted as fallbacks for pull-request builds. # Optional: SKIP_TESTS, ENABLE_NOREDIST, ENABLE_SIMULATOR (default true), -# MAVEN_THREADS (Jenkins uses 1C; empty disables -T; unset uses nproc). +# MAVEN_THREADS (Jenkins uses 1C; empty disables -T; unset uses nproc), +# RERUN_FAILING_TESTS (default 0; Jenkins uses 2 so a timing-flaky upstream +# test is retried before it fails the run). # Commands: mvn, java. Does not call other private-cicd scripts. # When RESULT_DIR is set, apply debian/rules packaging flags so build-debs.sh # can reuse target/ without a second mvn clean. @@ -43,6 +45,7 @@ SKIP_TESTS="${SKIP_TESTS:-false}" ENABLE_NOREDIST="${ENABLE_NOREDIST:-false}" ENABLE_SIMULATOR="${ENABLE_SIMULATOR:-true}" THREADS="${MAVEN_THREADS-$(nproc)}" +RERUN_FAILING_TESTS="${RERUN_FAILING_TESTS:-0}" RESULT_DIR="${RESULT_DIR:-}" mvn_args=(-B -P developer,systemvm) @@ -59,6 +62,9 @@ fi if [[ -n "$THREADS" ]]; then mvn_args+=(-T"$THREADS") fi +if [[ "$RERUN_FAILING_TESTS" != "0" ]]; then + mvn_args+=("-Dsurefire.rerunFailingTestsCount=$RERUN_FAILING_TESTS") +fi echo "==> Full build in $SOURCE_DIR" From b22a87a1e104e489668d74d0157845f360af131e Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 4 Nov 2026 23:37:20 +0530 Subject: [PATCH 08/14] Include the trusted ONTAP prerequisite checker in the CI sparse checkout. The multibranch CI checkout fetched only private-cicd, so Phase 2 stopped with "Trusted ONTAP prerequisite checker is missing" right after the snapshot revert. run-phase2-remote.sh runs the trusted copy of test/integration/plugins/ontap/check_ontap_prereqs.py from the CI checkout, so add that single file to the sparse paths. Co-Authored-By: Claude Sonnet 5.5 --- private-cicd/Jenkinsfile | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/private-cicd/Jenkinsfile b/private-cicd/Jenkinsfile index f9610902023e..3b25df5e38a0 100644 --- a/private-cicd/Jenkinsfile +++ b/private-cicd/Jenkinsfile @@ -997,7 +997,12 @@ void checkoutCiScripts() { doGenerateSubmoduleConfigurations: false, extensions: cloneOptions + [[ $class: 'SparseCheckoutPaths', - sparseCheckoutPaths: [[path: 'private-cicd']] + // run-phase2-remote.sh runs the trusted copy of the ONTAP + // prerequisite checker from this checkout, never the PR's copy. + sparseCheckoutPaths: [ + [path: 'private-cicd'], + [path: 'test/integration/plugins/ontap/check_ontap_prereqs.py'] + ] ]], userRemoteConfigs: [remote] ]) From 1cec7e4fa14d09ce98afdc057271657b6bc41ab1 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 06:19:18 +0530 Subject: [PATCH 09/14] Show the exception on the failed test's HTML log page and in the mail. The HTML log link of a failed ONTAP test opened the suite stdout log, which only holds the pass/fail summary, so the reason for the failure was not visible. Parse the ERROR and FAIL blocks of the suite's results.txt, put them in a "Failure details" section above the suite log, and link a failed test straight to its block. Add an Error column to the mail table with a one-line reason, leading with the CloudStack job error text when there is one, and link the archived management and agent logs from the failure section. Co-Authored-By: Claude Sonnet 5.5 --- .../scripts/generate-presubmit-report.py | 152 +++++++++++++++- .../scripts/test_generate_presubmit_report.py | 162 ++++++++++++++++++ 2 files changed, 308 insertions(+), 6 deletions(-) create mode 100644 private-cicd/scripts/test_generate_presubmit_report.py diff --git a/private-cicd/scripts/generate-presubmit-report.py b/private-cicd/scripts/generate-presubmit-report.py index 3067cfa26724..d60c8c4b8f02 100644 --- a/private-cicd/scripts/generate-presubmit-report.py +++ b/private-cicd/scripts/generate-presubmit-report.py @@ -21,9 +21,11 @@ import argparse import html import json +import os import re import shutil from datetime import datetime, timezone +from functools import lru_cache from pathlib import Path from urllib.parse import quote @@ -121,7 +123,115 @@ def html_document(title, body): """ -def log_to_html(source, destination): +FAILURE_HEADER = re.compile(r"^(ERROR|FAIL):\s*(.*)$") +DESCRIBED_TEST = re.compile( + r"^(.*?)\s*\.\.\.\s*=== TestName:\s+(\S+)\s+\|\s+Status\s+:\s+(\S+)") +JOB_ERROR = re.compile( + r"errorcode\s*:\s*(\d+)\s*,\s*errortext\s*:\s*'(.*?)(?:'\s*[,}]|$)") +MAX_FAILURE_LINES = 80 +MAX_ERROR_CHARS = 400 + + +@lru_cache(maxsize=None) +def parse_suite_failures(results_txt): + """Return the ERROR and FAIL blocks of one Marvin suite results.txt. + + Marvin prints each block with the test's docstring as its title, while the + summary lines pair the same docstring with the test name, so the two are + joined on that text. A block that cannot be joined (for example a + setUpClass error) keeps an empty name list and is still shown. + """ + try: + text = ANSI_ESCAPE.sub( + "", results_txt.read_text(encoding="utf-8", errors="replace")) + except OSError: + return () + lines = text.splitlines() + names = {} + for line in lines: + described = DESCRIBED_TEST.match(line) + if described: + names.setdefault(described.group(1).strip(), []).append( + described.group(2)) + failures = [] + index = 0 + while index < len(lines): + header = FAILURE_HEADER.match(lines[index]) + if not (header and index > 0 and lines[index - 1].startswith("=====")): + index += 1 + continue + block = [] + index += 1 + while index < len(lines): + line = lines[index] + if (line.startswith("=====") or line.startswith("Ran ") + or ">> begin captured" in line): + break + block.append(line) + index += 1 + while block and (not block[0].strip() + or set(block[0].strip()) == {"-"}): + block.pop(0) + while block and (not block[-1].strip() + or set(block[-1].strip()) == {"-"}): + block.pop() + description = header.group(2).strip() + error = block[-1].strip() if block else "" + # A failed CloudStack async job prints a long object of account and + # domain IDs before the useful text, so lead with the job's own message. + job = JOB_ERROR.search(error) + if job: + error = f"Job failed (code {job.group(1)}): {job.group(2)}" + if len(error) > MAX_ERROR_CHARS: + error = error[:MAX_ERROR_CHARS] + "…" + failures.append({ + "kind": header.group(1), + "description": description, + "names": tuple(names.get(description, ())), + "lines": tuple(block[:MAX_FAILURE_LINES]), + "error": error, + }) + return tuple(failures) + + +def failure_anchor(test_name): + return f"fail-{slug(test_name)}" + + +def render_failures(failures, related=()): + if not failures: + return "" + sections = [] + if related: + links = " · ".join( + f'{html.escape(label)}' + for label, path in related) + sections.append( + "

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

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

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

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

Failure details

" + "".join(sections) + "

Suite log

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

{html.escape(source.name)}

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

ONTAP tests

" "

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

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

ONTAP tests

No Marvin test summary was available.

") body = ( diff --git a/private-cicd/scripts/test_generate_presubmit_report.py b/private-cicd/scripts/test_generate_presubmit_report.py new file mode 100644 index 000000000000..02dd29c95aa9 --- /dev/null +++ b/private-cicd/scripts/test_generate_presubmit_report.py @@ -0,0 +1,162 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import importlib.util +import tempfile +import unittest +from pathlib import Path + + +SPEC = importlib.util.spec_from_file_location( + "generate_presubmit_report", + Path(__file__).with_name("generate-presubmit-report.py")) +report = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(report) + +SEP = "=" * 70 +DASH = "-" * 70 +RESULTS = f"""Start the VM. ... === TestName: test_06_start | Status : EXCEPTION === +ERROR +Check the pool. ... === TestName: test_07_pool | Status : FAIL === +FAIL +Detach it. ... === TestName: test_08_detach | Status : SUCCESS === +ok + +{SEP} +ERROR: Start the VM. +{DASH} +Traceback (most recent call last): + File "test.py", line 750, in test_06_start + self.apiClient.startVirtualMachine(cmd) +Exception: Job failed: {{errortext : 'Unable to orchestrate the start of VM'}} +-------------------- >> begin captured logging << -------------------- +captured line that must not be shown +--------------------- >> end captured logging << --------------------- + +{SEP} +FAIL: Check the pool. +{DASH} +Traceback (most recent call last): + File "test.py", line 90, in test_07_pool + self.assertEqual(a, b) +AssertionError: 1 != 2 +{DASH} +Ran 3 tests in 1.0s + +FAILED (errors=1, failures=1) +""" +SETUP_ERROR = f"""{SEP} +ERROR: setUpClass (suite.Class) +{DASH} +Traceback (most recent call last): +RuntimeError: pool create failed +{DASH} +Ran 0 tests in 0.1s +""" + + +class ParseSuiteFailuresTest(unittest.TestCase): + def parse(self, text): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "results.txt" + path.write_text(text, encoding="utf-8") + return report.parse_suite_failures(path) + + def test_error_and_fail_blocks_are_mapped_to_test_names(self): + failures = self.parse(RESULTS) + self.assertEqual([f["kind"] for f in failures], ["ERROR", "FAIL"]) + self.assertEqual(failures[0]["names"], ("test_06_start",)) + self.assertEqual(failures[1]["names"], ("test_07_pool",)) + + def test_error_is_the_exception_line_without_captured_logging(self): + failures = self.parse(RESULTS) + self.assertTrue(failures[0]["error"].startswith("Exception: Job failed")) + self.assertIn("Unable to orchestrate", failures[0]["error"]) + self.assertEqual(failures[1]["error"], "AssertionError: 1 != 2") + self.assertNotIn( + "must not be shown", "\n".join(failures[0]["lines"])) + + def test_cloudstack_job_failure_leads_with_its_message(self): + line = ("Exception: Job failed: {accountid : 'a', account : 'admin', " + "jobresult : {errorcode : 530, errortext : 'Unable to start " + "the VM'}}") + text = f"{SEP}\nERROR: d\n{DASH}\n{line}\n" + self.assertEqual( + self.parse(text)[0]["error"], + "Job failed (code 530): Unable to start the VM") + + def test_unmapped_block_is_kept_without_names(self): + failures = self.parse(SETUP_ERROR) + self.assertEqual(len(failures), 1) + self.assertEqual(failures[0]["names"], ()) + self.assertEqual(failures[0]["error"], "RuntimeError: pool create failed") + + def test_missing_file_or_clean_run_gives_no_failures(self): + self.assertEqual( + report.parse_suite_failures(Path("/nonexistent/results.txt")), ()) + self.assertEqual(self.parse("Detach. ... === TestName: t | Status : SUCCESS ===\nok\n"), ()) + + def test_long_error_is_truncated(self): + long_line = "Exception: " + "x" * 2000 + text = f"{SEP}\nERROR: d\n{DASH}\n{long_line}\n" + error = self.parse(text)[0]["error"] + self.assertEqual(len(error), report.MAX_ERROR_CHARS + 1) + + +class FailurePageTest(unittest.TestCase): + def test_suite_page_shows_failure_before_the_log(self): + with tempfile.TemporaryDirectory() as directory: + suite = (Path(directory) / "phase2" / "marvin" / "ontap-results" + / "run-iscsi" / "suites" / "vm") + suite.mkdir(parents=True) + (suite / "results.txt").write_text(RESULTS, encoding="utf-8") + log = suite / "stdout.log" + log.write_text( + "=== TestName: test_06_start | Status : EXCEPTION ===\n" + "=== TestName: test_08_detach | Status : SUCCESS ===\n", + encoding="utf-8") + management = (Path(directory) / "phase2" / "runtime-logs" + / "var-log-cloudstack-management") + management.mkdir(parents=True) + (management / "management-server.log").write_text("log\n") + report.generate_log_artifacts(Path(directory), None) + page = (suite / "stdout.log.html").read_text(encoding="utf-8") + self.assertIn('id="fail-test_06_start"', page) + self.assertIn("Unable to orchestrate", page) + self.assertIn('id="test-test_08_detach"', page) + self.assertIn("Management server log", page) + self.assertIn( + "../../../../../runtime-logs/var-log-cloudstack-management/" + "management-server.log", page) + self.assertNotIn("Agent log", page) + self.assertLess(page.index("Failure details"), page.index("Suite log")) + self.assertLess(page.index("Suite log"), page.index("TestName")) + + def test_clean_suite_page_has_no_failure_section(self): + with tempfile.TemporaryDirectory() as directory: + log = Path(directory) / "stdout.log" + log.write_text("=== TestName: t | Status : SUCCESS ===\n", + encoding="utf-8") + destination = Path(directory) / "stdout.log.html" + report.log_to_html(log, destination) + self.assertNotIn( + "Failure details", destination.read_text(encoding="utf-8")) + + +if __name__ == "__main__": + unittest.main() From 4400b629cf0640dd33b70904b98f4cf51714fead Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 06:25:18 +0530 Subject: [PATCH 10/14] Make the ONTAP VM tests tolerate a briefly unreachable virtual router. test_06_start_vm_lun_remapped failed once with "Unable to apply dhcp entry on router": the guest network's router had reported failed health checks about a minute earlier, and the test started the VM with no check and no retry. Add to OntapTestBase a bounded router readiness gate, a deploy wrapper that waits before and after the deploy (the first deploy creates the router), and a start wrapper that retries a VM that rolled back to Stopped, at most three attempts, and raises every attempt's error if they all fail. A single wait timeout makes the class stop waiting, so a permanently flagged router cannot slow every VM operation. Use the wrappers in the iSCSI and NFS3 VM tests. Co-Authored-By: Claude Sonnet 5.5 --- .../iscsi/instance/test_vm_volume_attach.py | 9 +- .../nfs3/instance/test_vm_volume_attach.py | 9 +- .../plugins/ontap/ontap_test_base.py | 120 ++++++++++++++++++ 3 files changed, 126 insertions(+), 12 deletions(-) diff --git a/test/integration/plugins/ontap/iscsi/instance/test_vm_volume_attach.py b/test/integration/plugins/ontap/iscsi/instance/test_vm_volume_attach.py index 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 7e3e850a39b0828907f3aedbb8dd131a3b4905cd Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 07:50:00 +0530 Subject: [PATCH 11/14] 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. Co-Authored-By: Claude Sonnet 5.5 --- 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 8a9ca7e42c8b703536021865ab1ef4e172e01232 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 10:14:14 +0530 Subject: [PATCH 12/14] 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. Co-Authored-By: Claude Sonnet 5.5 --- .../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 38478ba6ca508aedaa95cbcf1c51b444d9a381e9 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 5 Nov 2026 14:56:56 +0530 Subject: [PATCH 13/14] 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. Co-Authored-By: Claude Sonnet 5.5 --- .../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 6674bc84df6a1c809ced83420b8fd6282b503be3 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Fri, 6 Nov 2026 10:39:36 +0530 Subject: [PATCH 14/14] 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. Co-Authored-By: Claude Sonnet 5.5 --- 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.