Add draft s390x VPN connectivity test - #82562
Conversation
Introduce kubevirt-libvirt-vpn workflow/ref (pattern from openshift#72780) and a periodic connectivity job for libvirt-s390x-vpn-virt Signed-off-by: Thomas-David Griedel griedel911@gmail.com
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Davo911 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughAdds a scheduled s390x CI workflow that uses the Changess390x VPN connectivity CI
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ScheduledCI
participant VPNWorkflow
participant VPNStep
participant LeaseConfig
participant LibvirtHost
ScheduledCI->>VPNWorkflow: start e2e-s390x-vpn-connectivity
VPNWorkflow->>VPNStep: run kubevirt-libvirt-vpn
VPNStep->>LeaseConfig: resolve hostname and external_ip
VPNStep->>LibvirtHost: check port 7001 and run virsh list
LibvirtHost-->>VPNStep: return connectivity result
VPNStep-->>ScheduledCI: report success or failure
Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-commands.sh`:
- Around line 34-37: Remove the four diagnostic echo statements for
LEASED_RESOURCE, EXTERNAL_IP, HOSTNAME, and REMOTE_LIBVIRT_URI from the command
script. Do not print these sensitive lease or connectivity values; retain only
non-sensitive status logging if needed.
- Around line 46-52: Bound each network probe in the retry condition within the
attempt loop: configure both curl calls and the mock-nss.sh virsh invocation
using the repository’s established timeout options or variables, with limits
covering connection and operation time. Preserve the existing success condition
and retry behavior while ensuring a black-holed endpoint cannot block an
iteration indefinitely.
In
`@ci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-workflow.yaml`:
- Around line 3-7: Update the workflow definition containing
allow_best_effort_post_steps and the kubevirt-libvirt-vpn test reference to
declare all required workflow phases, not only test. Follow the established
step-registry phase structure, then run make validate-step-registry and resolve
any missing-phase or registry-reference validation errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ae1a7fd-6c0e-420e-9f9b-8789d6f7534d
📒 Files selected for processing (7)
ci-operator/config/kubevirt/ssp-operator/kubevirt-ssp-operator-main.yamlci-operator/step-registry/kubevirt/libvirt/vpn/OWNERSci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-commands.shci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-ref.metadata.jsonci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-ref.yamlci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-workflow.metadata.jsonci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-workflow.yaml
| echo "LEASED_RESOURCE=${LEASED_RESOURCE}" | ||
| echo "EXTERNAL_IP=${EXTERNAL_IP}" | ||
| echo "HOSTNAME=${HOSTNAME}" | ||
| echo "REMOTE_LIBVIRT_URI=${REMOTE_LIBVIRT_URI}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove private connectivity targets from CI logs.
REMOTE_LIBVIRT_URI, EXTERNAL_IP, and HOSTNAME expose internal cluster endpoints in CI logs. Log only non-sensitive status, and avoid printing the lease identifier as well.
As per coding guidelines, “Step registry command scripts must avoid leaking sensitive information into CI logs: do not echo passwords, tokens, API keys, cluster URLs, or kubeconfig contents.”
Proposed fix
-echo "LEASED_RESOURCE=${LEASED_RESOURCE}"
-echo "EXTERNAL_IP=${EXTERNAL_IP}"
-echo "HOSTNAME=${HOSTNAME}"
-echo "REMOTE_LIBVIRT_URI=${REMOTE_LIBVIRT_URI}"
+echo "Lease and connectivity targets loaded"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "LEASED_RESOURCE=${LEASED_RESOURCE}" | |
| echo "EXTERNAL_IP=${EXTERNAL_IP}" | |
| echo "HOSTNAME=${HOSTNAME}" | |
| echo "REMOTE_LIBVIRT_URI=${REMOTE_LIBVIRT_URI}" | |
| echo "Lease and connectivity targets loaded" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-commands.sh`
around lines 34 - 37, Remove the four diagnostic echo statements for
LEASED_RESOURCE, EXTERNAL_IP, HOSTNAME, and REMOTE_LIBVIRT_URI from the command
script. Do not print these sensitive lease or connectivity values; retain only
non-sensitive status logging if needed.
Source: Coding guidelines
| for ((i=1; i<=attempts; i++)); do | ||
| echo "$(date +%H:%M:%S) attempt ${i}/${attempts}" | ||
| # #CHECK: Port 7001 is from #72780 | ||
| # Confirm we expose the same | ||
| if curl -s "${EXTERNAL_IP}:7001" >/dev/null \ | ||
| && curl -s "${HOSTNAME}:7001" >/dev/null \ | ||
| && mock-nss.sh virsh -c "${REMOTE_LIBVIRT_URI}" list >/dev/null; then |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound every network probe.
curl and virsh have no connect or operation timeout. A black-holed VPN/DNS path can block the first iteration forever, so the retry loop does not cap job duration.
Proposed fix
- if curl -s "${EXTERNAL_IP}:7001" >/dev/null \
- && curl -s "${HOSTNAME}:7001" >/dev/null \
- && mock-nss.sh virsh -c "${REMOTE_LIBVIRT_URI}" list >/dev/null; then
+ if curl -s --connect-timeout 10 --max-time 20 "${EXTERNAL_IP}:7001" >/dev/null \
+ && curl -s --connect-timeout 10 --max-time 20 "${HOSTNAME}:7001" >/dev/null \
+ && timeout 30s mock-nss.sh virsh -c "${REMOTE_LIBVIRT_URI}" list >/dev/null; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for ((i=1; i<=attempts; i++)); do | |
| echo "$(date +%H:%M:%S) attempt ${i}/${attempts}" | |
| # #CHECK: Port 7001 is from #72780 | |
| # Confirm we expose the same | |
| if curl -s "${EXTERNAL_IP}:7001" >/dev/null \ | |
| && curl -s "${HOSTNAME}:7001" >/dev/null \ | |
| && mock-nss.sh virsh -c "${REMOTE_LIBVIRT_URI}" list >/dev/null; then | |
| for ((i=1; i<=attempts; i++)); do | |
| echo "$(date +%H:%M:%S) attempt ${i}/${attempts}" | |
| # `#CHECK`: Port 7001 is from `#72780` | |
| # Confirm we expose the same | |
| if curl -s --connect-timeout 10 --max-time 20 "${EXTERNAL_IP}:7001" >/dev/null \ | |
| && curl -s --connect-timeout 10 --max-time 20 "${HOSTNAME}:7001" >/dev/null \ | |
| && timeout 30s mock-nss.sh virsh -c "${REMOTE_LIBVIRT_URI}" list >/dev/null; then |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/kubevirt/libvirt/vpn/kubevirt-libvirt-vpn-commands.sh`
around lines 46 - 52, Bound each network probe in the retry condition within the
attempt loop: configure both curl calls and the mock-nss.sh virsh invocation
using the repository’s established timeout options or variables, with limits
covering connection and operation time. Preserve the existing success condition
and retry behavior while ensuring a black-holed endpoint cannot block an
iteration indefinitely.
There was a problem hiding this comment.
Not sure if this variable is defined.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
2 participants
Summary
libvirt-s390x-vpn-virte2e-s390x-vpn-connectivityon ssp-operator mainBlocked by
Summary by CodeRabbit
Adds draft s390x VPN connectivity coverage to the
ssp-operatorrepository’s periodic CI jobs.The workflow tests VPN access to libvirt hosts by checking port 7001 and running a remote libvirt query with retries. The configuration adds the required
libvirt-installerimage, s390x settings, ownership metadata, DNS servers, and a dailye2e-s390x-vpn-connectivityjob.Rehearsal depends on Boskos lease and secret bootstrap prerequisites.