Fix port conflicts#82360
Conversation
|
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:
WalkthroughThe baremetal load-balancer scripts extend setup timeouts, configure IPv6 DHCP explicitly for ChangesBaremetal load-balancer DHCP lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/pj-rehearse ack |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
a0910dd to
c7d4254
Compare
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.19-amd64-nightly-baremetal-sno-agent-ipv4-static-connected-fips-f28 |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
removed
the
rehearsals-ack
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ci-operator/step-registry/baremetal/lab/post/load-balancer/baremetal-lab-post-load-balancer-commands.sh (1)
41-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the IPv6 release call with startup
The startup command already uses
-cf /dev/null, but the release call does not. Add the same flag here so both dhclient invocations use the same configuration path.🤖 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/baremetal/lab/post/load-balancer/baremetal-lab-post-load-balancer-commands.sh` around lines 41 - 45, Update the IPv6 release dhclient invocation in the bare-metal load-balancer post command to include the same -cf /dev/null configuration flag used by startup, while preserving the existing release arguments and fallback message.
🤖 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/baremetal/lab/post/load-balancer/baremetal-lab-post-load-balancer-commands.sh`:
- Around line 32-37: Update the DHCP release commands in the load-balancer
cleanup flow, including the eth1/eth2 command and the additional release block,
so failures from nsenter or dhclient -r are not converted into success by `||
echo`. Allow only an explicitly confirmed missing lease to be treated as a
no-op; otherwise preserve the nonzero status and report the release failure
before container removal.
- Around line 32-45: Serialize the DHCP cleanup in the post load-balancer
teardown with the existing /tmp/dhclient_lease.lock. In the flow containing the
IPv4 and isolated IPv6 dhclient releases, acquire the lock before mutating
DHCP/OVS state and hold it through the lease release and port teardown, then
release it reliably after cleanup; otherwise enforce an equivalent guarantee
that this post step cannot overlap another lifecycle step.
---
Nitpick comments:
In
`@ci-operator/step-registry/baremetal/lab/post/load-balancer/baremetal-lab-post-load-balancer-commands.sh`:
- Around line 41-45: Update the IPv6 release dhclient invocation in the
bare-metal load-balancer post command to include the same -cf /dev/null
configuration flag used by startup, while preserving the existing release
arguments and fallback message.
🪄 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: 2519836a-55b3-495d-b9cb-a59c373e8959
📒 Files selected for processing (2)
ci-operator/step-registry/baremetal/lab/post/load-balancer/baremetal-lab-post-load-balancer-commands.shci-operator/step-registry/baremetal/lab/pre/load-balancer/run/baremetal-lab-pre-load-balancer-run-commands.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/baremetal/lab/pre/load-balancer/run/baremetal-lab-pre-load-balancer-run-commands.sh
c7d4254 to
efdac86
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/baremetal/lab/pre/load-balancer/run/baremetal-lab-pre-load-balancer-run-commands.sh (1)
130-137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the eth2 DHCPv6 option requests
-cf /dev/nullbypasses theeth2stanza that requestsdhcp6.name-servers,dhcp6.domain-search,dhcp6.fqdn, anddhcp6.sntp-servers, so this invocation now loses those IPv6 config options. If the fix only needs to avoid the port conflict, split the config so thisdhclient -6call still uses the eth2-specific requests.🤖 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/baremetal/lab/pre/load-balancer/run/baremetal-lab-pre-load-balancer-run-commands.sh` around lines 130 - 137, Update the IPv6 dhclient invocation in the eth2.br-int branch to retain the eth2-specific DHCPv6 option requests, including name servers, domain search, FQDN, and SNTP servers. Replace the /dev/null configuration bypass with a dedicated configuration that preserves those requests while still avoiding the port conflict, and keep the existing PID and lease file behavior.
🤖 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.
Outside diff comments:
In
`@ci-operator/step-registry/baremetal/lab/pre/load-balancer/run/baremetal-lab-pre-load-balancer-run-commands.sh`:
- Around line 130-137: Update the IPv6 dhclient invocation in the eth2.br-int
branch to retain the eth2-specific DHCPv6 option requests, including name
servers, domain search, FQDN, and SNTP servers. Replace the /dev/null
configuration bypass with a dedicated configuration that preserves those
requests while still avoiding the port conflict, and keep the existing PID and
lease file behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1da2da0f-4141-4092-a94d-cbf20b291352
📒 Files selected for processing (2)
ci-operator/step-registry/baremetal/lab/post/load-balancer/baremetal-lab-post-load-balancer-commands.shci-operator/step-registry/baremetal/lab/pre/load-balancer/run/baremetal-lab-pre-load-balancer-run-commands.sh
7222c99 to
c130c25
Compare
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.19-amd64-nightly-baremetal-sno-agent-ipv4-static-connected-fips-f28 |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.19-amd64-nightly-baremetal-sno-agent-ipv4-static-connected-fips-f28 |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
c130c25 to
b5fa100
Compare
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.19-amd64-nightly-baremetal-sno-agent-ipv4-static-connected-fips-f28 |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse abort |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.19-amd64-nightly-baremetal-sno-agent-ipv4-static-connected-fips-f28 |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
b5fa100 to
16ba231
Compare
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.19-amd64-nightly-baremetal-sno-agent-ipv4-static-connected-fips-f28 |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
A total of 659 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bmanzari, mhanss The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/pj-rehearse ack |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
/retest-required |
1 similar comment
|
/retest-required |
|
@mhanss: The following test 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. |
873641b
into
openshift:main
2 participants
Summary by CodeRabbit
ci-operator/step-registry/baremetal/labto reduce port/DHCP conflicts when multiple DHCP/lease operations run concurrently (notably affecting the HAProxy container namespace)....pre-load-balancer-run-commands.sh):/tmp/dhclient_lease.lockon FD201, waiting up to 20 minutes (flock -w 1200) with acleanupfunction andtrap.eth2(only wheneth2.br-intis present) to rundhclientwith an explicit empty config (dhclient -6 -v -cf /dev/null ...) instead of relying on-N....post-load-balancer-commands.sh):/tmp/dhclient_lease.lockFD201+flock -w 1200+trappattern.eth1/eth2using the HAProxy dhclient v4 pid/lease files only if the v4 lease file exists and contains activelease {records.eth2only ifeth2.br-intis present and the v6 lease file exists and contains activelease {records.