intra-step leasing: Add lease proxy server specs in prowgen #4877
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds a lease-proxy port and pod IP env: a new exported constant (8082), a PodSpec mutator that injects a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing touches
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli 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 |
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: 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. |
2 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.CI_OPERATOR_POD_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.