Skip to content

Conversation

@lwr20
Copy link
Member

@lwr20 lwr20 commented Dec 9, 2025

Description

Related issues/PRs

Todos

  • Tests
  • Documentation
  • Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

@marvin-tigera marvin-tigera added this to the Calico v3.32.0 milestone Dec 9, 2025
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Dec 9, 2025
@lwr20 lwr20 marked this pull request as ready for review December 9, 2025 19:04
@lwr20 lwr20 requested a review from a team as a code owner December 9, 2025 19:04
Copilot AI review requested due to automatic review settings December 9, 2025 19:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for OpenShift Hosted Control Plane (HCP) testing to the end-to-end test suite. HCP is an architecture where the OpenShift control plane runs separately from worker nodes, allowing for more efficient resource usage. The changes enable running certification tests on both standalone OpenShift clusters and HCP-based clusters.

Key changes:

  • Adds HCP-specific initialization, provisioning, testing, and teardown logic to the CI scripts
  • Introduces a new multi-stage HCP testing workflow with hosting cluster setup, hosted cluster tests, and cleanup
  • Updates certification pipeline to test multiple OpenShift versions (4.18, 4.19, 4.20) on both standalone and HCP architectures

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 18 comments.

File Description
.semaphore/end-to-end/scripts/global_prologue.sh Adds HCP initialization logic including banzai-utils cloning, HCP-specific setup, cache restoration for hosting clusters, and disk space management
.semaphore/end-to-end/scripts/global_epilogue.sh Adds helper functions for cleanup, implements HCP-specific teardown and artifact handling, manages cache deletion for different HCP stages
.semaphore/end-to-end/scripts/body_standard.sh Adds HCP-enabled test execution path with conditional logic to handle HCP provisioning and testing separately from standard banzai workflow
.semaphore/end-to-end/pipelines/certification.yml Restructures certification tests to include standalone cluster tests, HCP hosting setup, and HCP hosted cluster tests across multiple OpenShift versions

echo "$std"; eval "$std"

restore_hcp_hosting_home="echo \"[INFO] Setting BZ_HOME env var from restored cache\""
restore_hcp_hosting_home="$restore_hcp_hosting_home; unset BZ_HOME; export BZ_HOME=$(cat ${BZ_LOGS_DIR}/restore.log | grep -oP 'Restored: \K(.*)(?=.)' || echo '')"
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern (?=.) at the end of the grep command is incorrect. This is a positive lookahead that matches a position before any character, which doesn't make sense in this context. If the intention is to trim the trailing period from the restored path, use sed 's/\.$//' or adjust the regex to 'Restored: \K(.*)' without the lookahead.

Suggested change
restore_hcp_hosting_home="$restore_hcp_hosting_home; unset BZ_HOME; export BZ_HOME=$(cat ${BZ_LOGS_DIR}/restore.log | grep -oP 'Restored: \K(.*)(?=.)' || echo '')"
restore_hcp_hosting_home="$restore_hcp_hosting_home; unset BZ_HOME; export BZ_HOME=$(cat ${BZ_LOGS_DIR}/restore.log | grep -oP 'Restored: \K.*' | sed 's/\.$//' || echo '')"

Copilot uses AI. Check for mistakes.
restore_hcp_hosting_home="$restore_hcp_hosting_home; unset BZ_HOME; export BZ_HOME=$(cat ${BZ_LOGS_DIR}/restore.log | grep -oP 'Restored: \K(.*)(?=.)' || echo '')"
if [[ "${HCP_STAGE}" == "hosting" || "${HCP_STAGE}" == "destroy-hosting" ]]; then echo "$restore_hcp_hosting_home"; eval "$restore_hcp_hosting_home"; fi

if [[ "${HCP_STAGE}" == "hosting" || "${HCP_STAGE}" == "destroy-hosting" ]]; then python3 -m pip install -r ${BZ_HOME}/scripts/requirements.txt; export PROVISIONER=aws-openshift; pip3 install --upgrade --user awscli; fi
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The condition [[ "${HCP_STAGE}" == "hosting" || "${HCP_STAGE}" == "destroy-hosting" ]] is repeated on line 182. Consider extracting this into a variable at the beginning of the script for better maintainability, e.g., IS_HOSTING_STAGE=$([[ "${HCP_STAGE}" =~ ^(hosting|destroy-hosting)$ ]] && echo "true" || echo "false").

Copilot uses AI. Check for mistakes.
echo "[INFO] global_epilogue: hcp: pushing log artifacts"
artifact push job ${BZ_LOGS_DIR} --destination semaphore/logs || true
else
if [[ ${HCP_STAGE:-} != *?-hosting* ]]; then
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition pattern *-hosting* uses glob matching which will match strings like "pre-hosting-test" or "hosting-something". If the intention is to match only "setup-hosting" and "destroy-hosting", consider using an explicit check: [[ "${HCP_STAGE}" == "setup-hosting" || "${HCP_STAGE}" == "destroy-hosting" ]] for clarity and to prevent unintended matches.

Suggested change
if [[ ${HCP_STAGE:-} != *?-hosting* ]]; then
if [[ "${HCP_STAGE}" != "setup-hosting" && "${HCP_STAGE}" != "destroy-hosting" ]]; then

Copilot uses AI. Check for mistakes.
if [[ "${HCP_STAGE}" == "destroy-hosting" ]]; then
artifact yank workflow hosting-${HOSTING_CLUSTER}-kubeconfig
cache delete ${SEMAPHORE_WORKFLOW_ID}-hosting-${HOSTING_CLUSTER}
cache delete $(basename $PWD)
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command cache delete $(basename $PWD) uses command substitution on $PWD without quotes. If $PWD contains spaces or special characters, this could lead to unexpected behavior. Use "$(basename "$PWD")" to properly quote the variable.

Suggested change
cache delete $(basename $PWD)
cache delete "$(basename "$PWD")"

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not super-sure about the nested quotes though? maybe just use the outer ones?)

restore_hcp_hosting_home="$restore_hcp_hosting_home; unset BZ_HOME; export BZ_HOME=$(cat ${BZ_LOGS_DIR}/restore.log | grep -oP 'Restored: \K(.*)(?=.)' || echo '')"
if [[ "${HCP_STAGE}" == "hosting" || "${HCP_STAGE}" == "destroy-hosting" ]]; then echo "$restore_hcp_hosting_home"; eval "$restore_hcp_hosting_home"; fi

if [[ "${HCP_STAGE}" == "hosting" || "${HCP_STAGE}" == "destroy-hosting" ]]; then python3 -m pip install -r ${BZ_HOME}/scripts/requirements.txt; export PROVISIONER=aws-openshift; pip3 install --upgrade --user awscli; fi
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command pip3 install --upgrade --user awscli uses the --user flag which installs packages to the user's home directory. In CI environments, this can cause issues with package discovery. Consider removing --user flag or ensuring the PATH includes ~/.local/bin. Also, this line does both python3 -m pip install and pip3 install - for consistency, use one approach throughout.

Suggested change
if [[ "${HCP_STAGE}" == "hosting" || "${HCP_STAGE}" == "destroy-hosting" ]]; then python3 -m pip install -r ${BZ_HOME}/scripts/requirements.txt; export PROVISIONER=aws-openshift; pip3 install --upgrade --user awscli; fi
if [[ "${HCP_STAGE}" == "hosting" || "${HCP_STAGE}" == "destroy-hosting" ]]; then python3 -m pip install -r ${BZ_HOME}/scripts/requirements.txt; export PROVISIONER=aws-openshift; python3 -m pip install --upgrade awscli; fi

Copilot uses AI. Check for mistakes.
Comment on lines +64 to +68
- name: HCP setup hosting
dependencies: []
task:
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The agent configuration is missing for the "HCP setup hosting" job. While this may use defaults, for consistency with the other blocks ("Standalone cluster tests" at lines 45-48 and "HCP hosted setup and tests" at lines 89-92), consider explicitly specifying the agent machine type and OS image.

Copilot uses AI. Check for mistakes.
@lwr20 lwr20 added docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact and removed release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Dec 10, 2025
@lwr20 lwr20 force-pushed the lwr-hcp-runs branch 8 times, most recently from 55dc72f to 6126345 Compare December 11, 2025 11:33
@lwr20 lwr20 requested a review from a team as a code owner December 22, 2025 10:30
@lwr20 lwr20 enabled auto-merge December 22, 2025 19:13
@lwr20 lwr20 merged commit f60db73 into projectcalico:master Dec 22, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants