intra-step leasing: Add lease proxy server specs in prowgen#4877
intra-step leasing: Add lease proxy server specs in prowgen#4877danilo-gemoli wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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 an HTTP server port constant and a PodSpec mutator that injects an HTTP port (8080) and an env var exposing the pod IP; the mutator is applied in prow job builder tests, many test fixtures and generated job YAMLs updated, and a test updated to read UNRESOLVED_CONFIG from env vars. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/prowgen/podspec.go (1)
184-189: Consider adding duplicate check for consistency.The
addPorthelper always appends without checking for existing ports, unlike similar helpers (addEnvVar,addVolume,addVolumeMount) which prevent duplicates. While not an issue with current usage, adding a duplicate check would maintain consistency with the established pattern in this file.🔎 Optional: Add duplicate port check
func addPort(c *corev1.Container, name string, port int32) { + for _, p := range c.Ports { + if p.Name == name && p.ContainerPort == port { + return + } + } c.Ports = append(c.Ports, corev1.ContainerPort{ Name: name, ContainerPort: port, }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
pkg/api/constant.gopkg/prowgen/jobbase.gopkg/prowgen/podspec.gopkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_cluster_profile.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/api/constant.gopkg/prowgen/jobbase.gopkg/prowgen/podspec.gopkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_cluster_profile.yaml
🧬 Code graph analysis (2)
pkg/prowgen/jobbase.go (1)
pkg/prowgen/podspec.go (1)
LeaseProxyServer(498-504)
pkg/prowgen/podspec.go (1)
pkg/api/constant.go (1)
LeaseProxyServerPort(89-89)
🔇 Additional comments (7)
pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_cluster_profile.yaml (2)
19-23: LGTM!The environment variable correctly uses the downward API to expose the Pod IP, which is necessary for test steps to locate the lease proxy server.
27-29: LGTM!The container port configuration correctly exposes port 8082 as "lease-proxy", matching the constant defined in pkg/api/constant.go and used by the LeaseProxyServer() mutator.
pkg/api/constant.go (1)
88-89: LGTM!The constant definition is clear, follows existing naming conventions, and centralizes the port configuration for the lease proxy server.
pkg/prowgen/jobbase.go (2)
149-149: LGTM!The LeaseProxyServer() mutator is correctly added when a cluster profile is present, enabling ci-operator to run the lease proxy alongside the lease client configuration.
162-162: LGTM!The LeaseProxyServer() mutator is consistently added for the MultiStageTestConfiguration case, ensuring both configuration paths have the same lease proxy functionality.
pkg/prowgen/podspec.go (2)
470-478: LGTM!The environment variable correctly uses the Kubernetes downward API to expose the Pod's IP address, which test steps can use to connect to the lease proxy server.
498-504: LGTM!The LeaseProxyServer mutator correctly configures the pod specification to expose the lease proxy port and inject the Pod IP environment variable, enabling intra-step leasing functionality.
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/prowgen/podspec.go (1)
504-510: LGTM! Consider adding a doc comment.The implementation correctly adds the lease-proxy port and pod IP environment variable. The logic is sound and follows the same pattern as other mutators like
LeaseClient().📝 Optional: Add a doc comment for consistency
For consistency with similar functions like
LeaseClient(), consider adding a doc comment:+// LeaseProxyServer configures ci-operator to run an HTTP lease proxy server +// that forwards lease requests from workflow steps to Boskos, exposing the +// pod IP via CI_OPERATOR_POD_IP environment variable func LeaseProxyServer() PodSpecMutator {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/prowgen/podspec.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/prowgen/podspec.go
🧬 Code graph analysis (1)
pkg/prowgen/podspec.go (1)
pkg/api/constant.go (1)
LeaseProxyServerPort(89-89)
🔇 Additional comments (2)
pkg/prowgen/podspec.go (2)
184-195: LGTM!The
addPorthelper follows the established pattern of other helper functions in this file. The deduplication logic correctly prevents conflicts by checking both name and port number, and the implementation is clean and straightforward.
477-484: LGTM!The environment variable correctly uses the Kubernetes downward API to expose the pod's IP address. The field reference to
status.podIPis the standard way to inject pod metadata into containers.
|
Scheduling required tests: Scheduling tests matching the |
danilo-gemoli
force-pushed
the
feat/ci-operator-prowgen/intra-step-lease
branch
from
63e8431 to
3123d61
Compare
|
/test e2e |
|
/retest-required |
Prucek
left a comment
Prucek
left a comment
There was a problem hiding this comment.
/hold once this PR is merged, you would need to prepare a big PR changing all jobs in o/release. So whenever you are ready
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, Prucek 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 |
danilo-gemoli
force-pushed
the
feat/ci-operator-prowgen/intra-step-lease
branch
from
3123d61 to
3b411ab
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go`:
- Around line 513-519: The test checks for an unresolvedConfigEnv via
findUnresolvedConfigEnv and then immediately dereferences
unresolvedConfigEnv.Value; replace the t.Errorf call with t.Fatalf to abort the
test when unresolvedConfigEnv is nil (so you won't dereference a nil pointer).
Update the assertion on unresolvedConfigEnv in the test to use
t.Fatalf("UNRESOLVED_CONFIG not set on prowjob %s", pj.Spec.Job) (same
message/format) to stop execution, leaving the remainder of the test (reading
unresolvedConfigEnv.Value) unchanged.
| unresolvedConfigEnv := findUnresolvedConfigEnv(items[i].Spec.PodSpec.Containers[0].Env) | ||
| if unresolvedConfigEnv == nil { | ||
| t.Errorf("UNRESOLVED_CONFIG not set on prowjob %s", pj.Spec.Job) | ||
| } | ||
|
|
||
| unresolvedConfig := unresolvedConfigEnv.Value | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go | sed -n '500,540p'Repository: openshift/ci-tools
Length of output: 1496
Use t.Fatalf instead of t.Errorf to stop execution before dereferencing.
Line 515 uses t.Errorf, which logs an error but allows execution to continue. When unresolvedConfigEnv is nil, line 518 will dereference it and panic. Use t.Fatalf to stop execution immediately upon missing UNRESOLVED_CONFIG.
🛠️ Proposed fix
if unresolvedConfigEnv == nil {
- t.Errorf("UNRESOLVED_CONFIG not set on prowjob %s", pj.Spec.Job)
+ t.Fatalf("UNRESOLVED_CONFIG not set on prowjob %s", pj.Spec.Job)
}
unresolvedConfig := unresolvedConfigEnv.Value📝 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.
| unresolvedConfigEnv := findUnresolvedConfigEnv(items[i].Spec.PodSpec.Containers[0].Env) | |
| if unresolvedConfigEnv == nil { | |
| t.Errorf("UNRESOLVED_CONFIG not set on prowjob %s", pj.Spec.Job) | |
| } | |
| unresolvedConfig := unresolvedConfigEnv.Value | |
| unresolvedConfigEnv := findUnresolvedConfigEnv(items[i].Spec.PodSpec.Containers[0].Env) | |
| if unresolvedConfigEnv == nil { | |
| t.Fatalf("UNRESOLVED_CONFIG not set on prowjob %s", pj.Spec.Job) | |
| } | |
| unresolvedConfig := unresolvedConfigEnv.Value |
🤖 Prompt for AI Agents
In `@pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go` around lines 513 -
519, The test checks for an unresolvedConfigEnv via findUnresolvedConfigEnv and
then immediately dereferences unresolvedConfigEnv.Value; replace the t.Errorf
call with t.Fatalf to abort the test when unresolvedConfigEnv is nil (so you
won't dereference a nil pointer). Update the assertion on unresolvedConfigEnv in
the test to use t.Fatalf("UNRESOLVED_CONFIG not set on prowjob %s", pj.Spec.Job)
(same message/format) to stop execution, leaving the remainder of the test
(reading unresolvedConfigEnv.Value) unchanged.
There was a problem hiding this comment.
When unresolvedConfigEnv the loop skips the current cycle, preventing, in fact, a nil pointer dereference.
There was a problem hiding this comment.
@danilo-gemoli, you're absolutely right! I apologize for the confusion. Looking at the full file content, I can see there's a continue statement on line 516 right after the t.Errorf call:
if unresolvedConfigEnv == nil {
t.Errorf("UNRESOLVED_CONFIG not set on prowjob %s", pj.Spec.Job)
continue
}This continue statement indeed skips to the next iteration of the loop, preventing the nil pointer dereference on line 518. The code is safe as written. My initial review comment was based on an incomplete view of the code that didn't show the continue statement. Thank you for the clarification!
|
/test unit |
danilo-gemoli
force-pushed
the
feat/ci-operator-prowgen/intra-step-lease
branch
from
3b411ab to
af8d7f7
Compare
|
/test e2e |
|
@danilo-gemoli: 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. |
3 participants
This is the preparatory work that
ci-operatorrequires in order to run a small lease proxy server.We are working on the intra-step leasing capability that will allow a step from the
ci-operatorstep registry to acquire lease fromboskos.As of today,
ci-operatoracquire any leases required by a workflow and then run the test. With the new feature we are about to introduce, a step could also do that by sending requests toci-operatorthat will, in turn, forward them toboskos.A typical scenario looks like so:
ci-operatoris supposed to run the e2e-aws-ovn test:cluster_profile, thereforeci-operatorruns a small HTTP server that acts as a lease proxy server, actually forwarding any requests toboskos.openshift-e2e-awsexecutes.HTTP_SERVER_IPenvironment variable set, that it might uses to send requests to the proxy run byci-operator.ipi-install-installstep runs and tries to acquire a lease from the proxy.ci-operatorreceives the request, forwards it toboskosand finally returns the result back to theipi-install-installstep.