USHIFT-6810: Add C2CC upgrade test for RHEL 9.8 to RHEL 10.2#6894
USHIFT-6810: Add C2CC upgrade test for RHEL 9.8 to RHEL 10.2#6894vimauro wants to merge 7 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vimauro: This pull request references USHIFT-6810 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds RHEL9-to-RHEL10 C2CC upgrade presubmit test with new Robot Framework suite that sequentially upgrades three clusters and verifies health and connectivity. Supporting changes include GA repository enablement in container templates, SSH host key permission fixes via sshd drop-in, TARGET_REF and BOOTC_REGISTRY variable forwarding in ChangesC2CC RHEL9→RHEL10 Upgrade Test Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vimauro 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/suites/upgrade/upgrade-c2cc.robot`:
- Around line 135-153: The Cluster Rebooted And Healthy keyword re-registers all
clusters using Register Remote Cluster (which opens a new SSH connection), but
the initial setup uses Register Local Cluster cluster-a (which reuses the
existing SSH connection from Login MicroShift Host). Add conditional logic
within Cluster Rebooted And Healthy to detect if the alias is cluster-a and call
Register Local Cluster instead of Register Remote Cluster for that specific
cluster, ensuring consistent SSH connection handling and registration approach
between the initial setup and post-reboot re-registration.
🪄 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: Enterprise
Run ID: 70d633a2-942c-4ea0-bb3c-2a03a88f9a1a
📒 Files selected for processing (6)
test/bin/c2cc_common.shtest/image-blueprints-bootc/el10/layer1-base/group1/rhel102-test-agent.containerfiletest/image-blueprints-bootc/el10/layer2-presubmit/group1/rhel102-bootc-source.containerfiletest/image-blueprints-bootc/templates/rpm-repo-config.sh.templatetest/scenarios-bootc/el10/presubmits/el98-src@el102-src@c2cc-upgrade-ok.shtest/suites/upgrade/upgrade-c2cc.robot
|
/retest |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
vimauro
force-pushed
the
c2cc-upgrade-tests
branch
from
89c47c2 to
d5bc734
Compare
| [Documentation] Register all three clusters for SSH and oc access | ||
| ... and store connection details for reconnection after reboots. | ||
| Check Required Env Variables | ||
| Should Not Be Empty ${TARGET_REF} TARGET_REF variable is required |
There was a problem hiding this comment.
for consistency I think we should also add a check for $BOOTC_REGISTRY
| Should Not Be Empty ${TARGET_REF} TARGET_REF variable is required | |
| Should Not Be Empty ${TARGET_REF} TARGET_REF variable is required | |
| Should Not Be Empty ${BOOTC_REGISTRY} BOOTC_REGISTRY variable is required |
| local bootc_registry_var="" | ||
| if [ -n "${MIRROR_REGISTRY_URL:-}" ]; then | ||
| bootc_registry_var="--variable BOOTC_REGISTRY:${MIRROR_REGISTRY_URL}" | ||
| fi |
There was a problem hiding this comment.
don't think we need to wrap $BOOTC_REGISTRY in an if block to check if it's defined because this always set in common.sh but it's fine as it is.
| enable_eus_repositories | ||
| shift | ||
| ;; | ||
| --enable-ga) |
There was a problem hiding this comment.
where this new --enable-ga flag is used? I can't find it
There was a problem hiding this comment.
same as below: this was coming from a previous change that used 10.2 GA repos (which we discussed offline), I thought it was worth to keep it, but I'm happy to remove if it's too much of the scope of the PR
There was a problem hiding this comment.
if this not needed for this PR, I prefer to remove it
| # Fix SSH host key permissions for cross-version upgrades (RHEL 9 uses 0640 | ||
| # with ssh_keys group, RHEL 10 sshd requires 0600) | ||
| # Similar workaround is used in microshift-test-agent.sh | ||
| RUN mkdir -p /usr/lib/systemd/system/sshd.service.d && \ | ||
| printf '[Service]\nExecStartPre=/bin/bash -c "chmod 600 /etc/ssh/ssh_host_*_key 2>/dev/null || true"\n' \ | ||
| > /usr/lib/systemd/system/sshd.service.d/fix-hostkey-perms.conf |
There was a problem hiding this comment.
We have el98-src@el102-src@upgrade-ok.sh - which ran today and was green so we should investigate what's the problem here.
The workaround in microshift-test-agent.sh should work and we should not need to add this here.
| --enable-ga) | ||
| enable_ga_repositories | ||
| shift | ||
| ;; |
There was a problem hiding this comment.
This was coming from a previous change that used 10.2 GA repos (which we discussed offline), I thought it was worth to keep it, but I'm happy to remove if it's too much of the scope of the PR
There was a problem hiding this comment.
Let's move unrelated stuff to another PR - it would be to coordinate with the effort to reenable GA repos though
| VAR @{C2CC_REMOTE_ALIASES}= @{EMPTY} scope=SUITE | ||
| ${local_conn}= Get From Dictionary ${C2CC_SSH_IDS} cluster-a | ||
| SSHLibrary.Switch Connection ${local_conn} | ||
| ${status}= Run Keyword And Return Status SSHLibrary.Switch Connection ${local_conn} |
| Wait Until Keyword Succeeds 3m 10s | ||
| ... Verify RemoteCluster State ${alias} Healthy | ||
| END | ||
| Verify All RemoteClusters Healthy |
There was a problem hiding this comment.
Do we still need Ensure All Clusters Healthy keyword then? We can use Verify All RemoteClusters Healthy directly instead
There was a problem hiding this comment.
good catch, this used to be different initially :D
| @@ -0,0 +1 @@ | |||
| ../../c2cc/el98-src@el102-src@c2cc-upgrade-ok.sh No newline at end of file | |||
|
@vimauro: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Summary by CodeRabbit