Integrate BYOIP with Cluster Profile Sets#5169
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
approved
📝 WalkthroughWalkthroughRefactors IP-pool lease initialization to accept an explicit ChangesIP Pool Lease Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🤖 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 `@pkg/api/leases.go`:
- Around line 30-45: The exported function signature for IPPoolLeaseForTest was
changed and will break downstream callers; restore the original exported
entrypoint IPPoolLeaseForTest(s *MultiStageTestConfigurationLiteral, metadata
Metadata) as a compatibility wrapper that adapts its parameters to the new
ClusterProfile+branch form and calls the new helper (e.g.,
ipPoolLeaseForTestFromProfile or IPPoolLeaseForTestProfile) which contains the
current logic that uses profile.IPPoolLeaseType(),
profile.IPPoolLeaseShouldValidateBranch(), and
branchValidForIPPoolLease(branch); keep the new logic in that helper and ensure
the wrapper returns the same StepLease type so existing vendored code continues
to compile.
In `@pkg/api/types.go`:
- Around line 2611-2616: ClusterProfileFromParams currently returns the error
from params.Get(ClusterProfileParam) which surfaces missing-parameter errors and
prevents callers from falling back to the static cluster_profile; change it so
that after calling params.Get(ClusterProfileParam) you detect the "parameter not
found" / missing-parameter error (e.g., parameters.ErrNotFound or the
package-specific not-found sentinel returned by Get) and treat that case as "no
override" by returning ClusterProfile("") and nil, while still returning any
other non-nil errors from params.Get unchanged.
In `@pkg/defaults/defaults.go`:
- Around line 366-367: The PR unconditionally wraps every multi-stage step with
steps.IPPoolStep which causes ipPoolStep.Validate to return
NoLeaseClientForIPErr when cfg.LeaseClient is nil; update the wrapping so
IPPoolStep is only applied when a lease client is available (or when the test is
actually IP-pool-eligible). Concretely, after creating the step via
multi_stage.MultiStageTestStep(*c, ...), guard the call to steps.IPPoolStep with
a check for cfg.LeaseClient != nil (or the existing IP-pool eligibility
predicate) so that IPPoolStep is not constructed/validated when cfg.LeaseClient
is nil; reference symbols: multi_stage.MultiStageTestStep, steps.IPPoolStep,
ipPoolStep.Validate, and NoLeaseClientForIPErr.
- Around line 358-367: The branch only wraps cfg.params into a per-test
DeferredParameters when leases exist, which lets later multi-stage tests
overwrite the shared provider; to fix, always clone cfg.params for each
multi-stage test by creating a per-test DeferredParameters unconditionally (use
api.NewDeferredParameters on cfg.params or otherwise assign params :=
api.NewDeferredParameters(cfg.params)) before calling
multi_stage.MultiStageTestStep and steps.IPPoolStep so IPPoolStep's provided
IP_POOL_AVAILABLE cannot leak between tests; update the code around params,
api.NewDeferredParameters, multi_stage.MultiStageTestStep and steps.IPPoolStep
accordingly.
🪄 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: 83e1126b-e78c-4ad0-b7ba-b77a8b25be07
📒 Files selected for processing (7)
pkg/api/leases.gopkg/api/leases_test.gopkg/api/types.gopkg/defaults/defaults.gopkg/steps/ip_pool.gopkg/steps/ip_pool_test.gopkg/steps/multi_stage/multi_stage.go
| func IPPoolLeaseForTest(profile ClusterProfile, branch string) (ret StepLease) { | ||
| if profile == "" { | ||
| return | ||
| } | ||
|
|
||
| if lt := profile.IPPoolLeaseType(); lt != "" { | ||
| if !profile.IPPoolLeaseShouldValidateBranch() || branchValidForIPPoolLease(branch) { | ||
| ret = StepLease{ | ||
| ResourceType: lt, | ||
| Env: DefaultIPPoolLeaseEnv, | ||
| Count: maxAddressesRequired, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return |
There was a problem hiding this comment.
Keep the old IPPoolLeaseForTest entrypoint until downstream repos migrate.
This exported signature change will break the vendored callers in openshift/release-controller and openshift/ci-chat-bot, which still compile against IPPoolLeaseForTest(s *MultiStageTestConfigurationLiteral, metadata Metadata). Please keep a compatibility wrapper and move the new logic behind a differently named helper, or land the downstream updates in lockstep.
As per coding guidelines, pkg/api/**: Changes here affect every repository that uses OpenShift CI.
🤖 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 `@pkg/api/leases.go` around lines 30 - 45, The exported function signature for
IPPoolLeaseForTest was changed and will break downstream callers; restore the
original exported entrypoint IPPoolLeaseForTest(s
*MultiStageTestConfigurationLiteral, metadata Metadata) as a compatibility
wrapper that adapts its parameters to the new ClusterProfile+branch form and
calls the new helper (e.g., ipPoolLeaseForTestFromProfile or
IPPoolLeaseForTestProfile) which contains the current logic that uses
profile.IPPoolLeaseType(), profile.IPPoolLeaseShouldValidateBranch(), and
branchValidForIPPoolLease(branch); keep the new logic in that helper and ensure
the wrapper returns the same StepLease type so existing vendored code continues
to compile.
| params := cfg.params | ||
|
|
||
| leases := api.LeasesForTest(c) | ||
| ipPoolLease := api.IPPoolLeaseForTest(test, cfg.CIConfig.Metadata) | ||
| if len(leases) != 0 || ipPoolLease.ResourceType != "" { | ||
| if len(leases) != 0 { | ||
| params = api.NewDeferredParameters(params) | ||
| } | ||
|
|
||
| var ret []api.Step | ||
| step := multi_stage.MultiStageTestStep(*c, cfg.CIConfig, params, cfg.podClient, cfg.JobSpec, leases, cfg.NodeName, cfg.TargetAdditionalSuffix, nil, cfg.GSMConfig != nil, cfg.GSMConfig, isLeaseProxyServerAvailable(cfg), retry.DefaultRetry) | ||
| if ipPoolLease.ResourceType != "" { | ||
| step = steps.IPPoolStep(cfg.LeaseClient, cfg.podClient, ipPoolLease, step, params, cfg.JobSpec.Namespace, cfg.MetricsAgent) | ||
| } | ||
| step = steps.IPPoolStep(cfg.LeaseClient, cfg.podClient, step, params, cfg.JobSpec.Namespace, cfg.MetricsAgent, test.ClusterProfile, cfg.CIConfig.Metadata.Branch) |
There was a problem hiding this comment.
Clone params for every multi-stage test now that IPPoolStep always provides a parameter.
IPPoolStep.Provides() now always registers IP_POOL_AVAILABLE, but this branch only creates a per-test DeferredParameters when standard leases exist. That lets later multi-stage tests overwrite the shared provider in cfg.params, so an AWS test without normal leases can read another test’s lease count instead of its own.
Suggested fix
if test := c.MultiStageTestConfigurationLiteral; test != nil {
- params := cfg.params
+ params := api.NewDeferredParameters(cfg.params)
leases := api.LeasesForTest(c)
- if len(leases) != 0 {
- params = api.NewDeferredParameters(params)
- }
var ret []api.StepAs per coding guidelines, pkg/steps/**: Changes here directly affect how CI jobs execute for all OpenShift repos.
📝 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.
| params := cfg.params | |
| leases := api.LeasesForTest(c) | |
| ipPoolLease := api.IPPoolLeaseForTest(test, cfg.CIConfig.Metadata) | |
| if len(leases) != 0 || ipPoolLease.ResourceType != "" { | |
| if len(leases) != 0 { | |
| params = api.NewDeferredParameters(params) | |
| } | |
| var ret []api.Step | |
| step := multi_stage.MultiStageTestStep(*c, cfg.CIConfig, params, cfg.podClient, cfg.JobSpec, leases, cfg.NodeName, cfg.TargetAdditionalSuffix, nil, cfg.GSMConfig != nil, cfg.GSMConfig, isLeaseProxyServerAvailable(cfg), retry.DefaultRetry) | |
| if ipPoolLease.ResourceType != "" { | |
| step = steps.IPPoolStep(cfg.LeaseClient, cfg.podClient, ipPoolLease, step, params, cfg.JobSpec.Namespace, cfg.MetricsAgent) | |
| } | |
| step = steps.IPPoolStep(cfg.LeaseClient, cfg.podClient, step, params, cfg.JobSpec.Namespace, cfg.MetricsAgent, test.ClusterProfile, cfg.CIConfig.Metadata.Branch) | |
| params := api.NewDeferredParameters(cfg.params) | |
| leases := api.LeasesForTest(c) | |
| var ret []api.Step | |
| step := multi_stage.MultiStageTestStep(*c, cfg.CIConfig, params, cfg.podClient, cfg.JobSpec, leases, cfg.NodeName, cfg.TargetAdditionalSuffix, nil, cfg.GSMConfig != nil, cfg.GSMConfig, isLeaseProxyServerAvailable(cfg), retry.DefaultRetry) | |
| step = steps.IPPoolStep(cfg.LeaseClient, cfg.podClient, step, params, cfg.JobSpec.Namespace, cfg.MetricsAgent, test.ClusterProfile, cfg.CIConfig.Metadata.Branch) |
🤖 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 `@pkg/defaults/defaults.go` around lines 358 - 367, The branch only wraps
cfg.params into a per-test DeferredParameters when leases exist, which lets
later multi-stage tests overwrite the shared provider; to fix, always clone
cfg.params for each multi-stage test by creating a per-test DeferredParameters
unconditionally (use api.NewDeferredParameters on cfg.params or otherwise assign
params := api.NewDeferredParameters(cfg.params)) before calling
multi_stage.MultiStageTestStep and steps.IPPoolStep so IPPoolStep's provided
IP_POOL_AVAILABLE cannot leak between tests; update the code around params,
api.NewDeferredParameters, multi_stage.MultiStageTestStep and steps.IPPoolStep
accordingly.
danilo-gemoli
force-pushed
the
fix/byoip/integrate-with-cluster-profile-set
branch
from
6ab95fd to
976745e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/defaults/defaults.go (1)
361-370:⚠️ Potential issue | 🟠 MajorBYOIP acquisition is skipped when no standard leases are requested
Conditioning
IPPoolSteponlen(leases) != 0removes the prior BYOIP path for tests that need an IP pool but do not request regular leases. That regresses the fallback behavior.Suggested fix
leases := api.LeasesForTest(c) - if len(leases) != 0 { + ipPoolLease := api.IPPoolLeaseForTest(test.ClusterProfile, cfg.CIConfig.Metadata.Branch) + needsIPPool := ipPoolLease.ResourceType != "" + if len(leases) != 0 || needsIPPool { params = api.NewDeferredParameters(params) } @@ - if len(leases) != 0 { + if len(leases) != 0 || needsIPPool { step = steps.IPPoolStep(cfg.LeaseClient, cfg.podClient, step, params, cfg.JobSpec.Namespace, cfg.MetricsAgent, test.ClusterProfile, cfg.CIConfig.Metadata.Branch) + } + if len(leases) != 0 { step = steps.LeaseStep(cfg.LeaseClient, leases, step, cfg.JobSpec.Namespace, cfg.MetricsAgent, cfg.kubeClient, cfg.ClusterProfileGetter) }🤖 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 `@pkg/defaults/defaults.go` around lines 361 - 370, The conditional wrapping that only adds IPPoolStep when len(leases) != 0 erroneously skips the BYOIP/IP pool fallback for tests that need a pool but request no regular leases; update the logic in defaults.go so that steps.IPPoolStep(cfg.LeaseClient, cfg.podClient, step, params, cfg.JobSpec.Namespace, cfg.MetricsAgent, test.ClusterProfile, cfg.CIConfig.Metadata.Branch) is applied regardless of leases length (or guard it with a specific “needs IP pool” condition), leaving steps.LeaseStep(...) still conditional on len(leases) != 0; locate the code around multi_stage.MultiStageTestStep, the variable leases, and the IPPoolStep/LeaseStep calls to make this change.
🤖 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.
Duplicate comments:
In `@pkg/defaults/defaults.go`:
- Around line 361-370: The conditional wrapping that only adds IPPoolStep when
len(leases) != 0 erroneously skips the BYOIP/IP pool fallback for tests that
need a pool but request no regular leases; update the logic in defaults.go so
that steps.IPPoolStep(cfg.LeaseClient, cfg.podClient, step, params,
cfg.JobSpec.Namespace, cfg.MetricsAgent, test.ClusterProfile,
cfg.CIConfig.Metadata.Branch) is applied regardless of leases length (or guard
it with a specific “needs IP pool” condition), leaving steps.LeaseStep(...)
still conditional on len(leases) != 0; locate the code around
multi_stage.MultiStageTestStep, the variable leases, and the
IPPoolStep/LeaseStep calls to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 049df236-0941-4ad1-b8d6-341f3631808b
📒 Files selected for processing (7)
pkg/api/leases.gopkg/api/leases_test.gopkg/api/types.gopkg/defaults/defaults.gopkg/steps/ip_pool.gopkg/steps/ip_pool_test.gopkg/steps/multi_stage/multi_stage.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/api/leases_test.go
- pkg/steps/multi_stage/multi_stage.go
- pkg/steps/ip_pool_test.go
- pkg/api/leases.go
- pkg/steps/ip_pool.go
- pkg/api/types.go
|
/test e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, jmguzik 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 |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/test e2e |
|
@danilo-gemoli: 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. |
openshift-merge-bot
Bot
merged commit 3dd1e85
into
openshift:main
2 participants
The "Bring Your Own IP Pool" feature assumes a cluster profile to be defined at configuration time:
but it doesn't work as intended when a Cluster Profile Set is used.
The following depicts a scenario in which a test uses the Cluster Profile Set functionality that breaks BYOIP.
ci-operatortest's configuration:ci-operatoracquires a lease of typeopenhishift-org-awsand it getsaws--us-east-1--quota-slice-001awsis loada. The cluster profile
awsis a legitimate profile for using BYOIPci-operatortries to acquire some leases for BYOIP but it considers the profileopenhishift-org-aws(statically defined) rather thanawsThis PR addresses the issue above by taking into account the cluster profile defined at runtime
awsrather than the static oneopenshift-org-aws.If no Cluster Profile Set is in use, it falls back to the old behaviour.
BYOIP Integration with Cluster Profile Sets
This PR fixes a BYOIP (Bring Your Own IP Pool) integration bug in ci-operator so IP pools are claimed using the runtime cluster profile when Cluster Profile Sets are used.
What changed, in practical terms
Key implementation points
Impact