Add Synthetic lease-proxy-server Step#4948
Add Synthetic lease-proxy-server Step#4948danilo-gemoli wants to merge 1 commit intoopenshift:mainfrom
lease-proxy-server Step#4948Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThis pull request introduces a lease proxy server feature to the CI operator. It adds configuration flags, new step link types for dependency modeling, a lease proxy step implementation, and integrates these components into the multi-stage test infrastructure through additional dependencies and test updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
| ret = append(ret, api.ReleaseImagesLink(releaseName)) | ||
| } | ||
|
|
||
| ret = append(ret, api.LeaseProxyServerLink()) |
There was a problem hiding this comment.
Here we declare the new dependency.
|
[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
| func (*stepLeaseProxyServer) Description() string { return "" } | ||
| func (*stepLeaseProxyServer) Requires() []api.StepLink { return nil } | ||
| func (*stepLeaseProxyServer) Creates() []api.StepLink { | ||
| return []api.StepLink{api.LeaseProxyServerReadyLink()} |
There was a problem hiding this comment.
And here we resolve the dependency.
danilo-gemoli
changed the title
lease-proxy-server Steplease-proxy-server Step
|
/test e2e |
![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
🤖 Fix all issues with AI agents
In `@pkg/api/graph.go`:
- Around line 285-316: LeaseProxyServerReadyLink.SatisfiedBy currently returns
true for any StepLink, which incorrectly marks any requirement satisfied; change
the implementation of leaseProxyServerReadyLink.SatisfiedBy to only return true
when the other link is either *leaseProxyServerLink or
*leaseProxyServerReadyLink (mirroring the pattern used by
imagesReadyLink/rpmRepoLink) so that HasAllLinks only treats the actual lease
proxy link (or itself) as satisfying the requirement.
In `@pkg/steps/multi_stage/multi_stage.go`:
- Around line 328-330: The code always appends api.LeaseProxyServerLink() in the
multi-stage step construction, which creates an unsatisfied dependency when no
lease client is present; modify the multiStageTestStep construction to consult
the lease-client availability (e.g., add or use a LeaseClientEnabled boolean on
the multiStageTestStep struct) and only append api.LeaseProxyServerLink() when
that flag is true in the function that currently does "ret = append(ret,
api.LeaseProxyServerLink())" (or alternatively ensure the lease-proxy step
itself is always added so the link can be satisfied); update any
callers/constructors of multiStageTestStep to thread the flag through.
🧹 Nitpick comments (1)
pkg/steps/lease_proxy.go (1)
39-45: Consider a cleaner linter workaround or removing the unused import.The
if 1 == 2block is unconventional. Iffmtis needed for future implementation, a common pattern is to use a blank identifier assignment likevar _ = fmt.Errorfat package level, or simply remove the import until actual implementation. Since this is a temporary stub, this is a minor point.♻️ Alternative approach
-func (s *stepLeaseProxyServer) run(context.Context) error { - if 1 == 2 { - return fmt.Errorf("unreachable code to make the linter happy. Will be removed soon.") - } - s.logger.Info("TODO - Not implemented") - return nil -} +func (s *stepLeaseProxyServer) run(context.Context) error { + s.logger.Info("TODO - Not implemented") + return nil +}And remove the
"fmt"import until it's needed.
| func LeaseProxyServerLink() StepLink { | ||
| return &leaseProxyServerLink{} | ||
| } | ||
|
|
||
| type leaseProxyServerLink struct{} | ||
|
|
||
| func (*leaseProxyServerLink) SatisfiedBy(other StepLink) bool { | ||
| switch other.(type) { | ||
| case *leaseProxyServerReadyLink: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| func (*leaseProxyServerLink) UnsatisfiableError() string { | ||
| return "" | ||
| } | ||
|
|
||
| func LeaseProxyServerReadyLink() StepLink { | ||
| return &leaseProxyServerReadyLink{} | ||
| } | ||
|
|
||
| type leaseProxyServerReadyLink struct{} | ||
|
|
||
| func (*leaseProxyServerReadyLink) SatisfiedBy(StepLink) bool { | ||
| return true | ||
| } | ||
|
|
||
| func (*leaseProxyServerReadyLink) UnsatisfiableError() string { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*.go" -type f | xargs grep -l "lease_proxy\|LeaseProxy" | head -20Repository: openshift/ci-tools
Length of output: 217
🏁 Script executed:
rg -n "LeaseProxyServerReadyLink|LeaseProxyServerLink|Creates\(" -g '!vendor' -A 3 | head -100Repository: openshift/ci-tools
Length of output: 5779
🏁 Script executed:
rg -n "HasAllLinks\|SatisfiedBy" pkg/api/graph.go -A 5 | head -50Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
cat -n pkg/steps/lease_proxy.go | head -100Repository: openshift/ci-tools
Length of output: 1871
🏁 Script executed:
rg -n "Creates\(\)" pkg/steps/lease_proxy.go -A 2Repository: openshift/ci-tools
Length of output: 184
🏁 Script executed:
rg -n "HasAllLinks\|func.*SatisfiedBy" pkg/api/ -A 10Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
sed -n '320,340p' pkg/steps/multi_stage/multi_stage.goRepository: openshift/ci-tools
Length of output: 767
🏁 Script executed:
rg -n "HasAllLinks" . -g '!vendor' -A 5Repository: openshift/ci-tools
Length of output: 1357
🏁 Script executed:
rg -n "SatisfiedBy" . -g '!vendor' -B 3 -A 3Repository: openshift/ci-tools
Length of output: 6113
🏁 Script executed:
rg -n "ImagesReadyLink\|ImagesLink" pkg/api/graph.go -B 2 -A 8Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
sed -n '602,612p' pkg/api/graph.goRepository: openshift/ci-tools
Length of output: 300
🏁 Script executed:
rg -n "ImagesReadyLink\|RpmRepoLink" pkg/api/graph.go | grep -A 5 "SatisfiedBy"Repository: openshift/ci-tools
Length of output: 44
Fix LeaseProxyServerReadyLink.SatisfiedBy to prevent incorrect dependency satisfaction.
Line 310 returns true for any StepLink, which will incorrectly satisfy all requirements in HasAllLinks. This breaks dependency ordering—any step's requirement will appear satisfied once the lease proxy step runs, even if the actual required link wasn't created. Fix by restricting SatisfiedBy to only match LeaseProxyServerLink and itself, following the pattern used by other ready links like imagesReadyLink and rpmRepoLink.
Fix
-func (*leaseProxyServerReadyLink) SatisfiedBy(StepLink) bool {
- return true
-}
+func (*leaseProxyServerReadyLink) SatisfiedBy(other StepLink) bool {
+ switch other.(type) {
+ case *leaseProxyServerLink, *leaseProxyServerReadyLink:
+ return true
+ default:
+ return false
+ }
+}📝 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.
| func LeaseProxyServerLink() StepLink { | |
| return &leaseProxyServerLink{} | |
| } | |
| type leaseProxyServerLink struct{} | |
| func (*leaseProxyServerLink) SatisfiedBy(other StepLink) bool { | |
| switch other.(type) { | |
| case *leaseProxyServerReadyLink: | |
| return true | |
| default: | |
| return false | |
| } | |
| } | |
| func (*leaseProxyServerLink) UnsatisfiableError() string { | |
| return "" | |
| } | |
| func LeaseProxyServerReadyLink() StepLink { | |
| return &leaseProxyServerReadyLink{} | |
| } | |
| type leaseProxyServerReadyLink struct{} | |
| func (*leaseProxyServerReadyLink) SatisfiedBy(StepLink) bool { | |
| return true | |
| } | |
| func (*leaseProxyServerReadyLink) UnsatisfiableError() string { | |
| return "" | |
| } | |
| func LeaseProxyServerLink() StepLink { | |
| return &leaseProxyServerLink{} | |
| } | |
| type leaseProxyServerLink struct{} | |
| func (*leaseProxyServerLink) SatisfiedBy(other StepLink) bool { | |
| switch other.(type) { | |
| case *leaseProxyServerReadyLink: | |
| return true | |
| default: | |
| return false | |
| } | |
| } | |
| func (*leaseProxyServerLink) UnsatisfiableError() string { | |
| return "" | |
| } | |
| func LeaseProxyServerReadyLink() StepLink { | |
| return &leaseProxyServerReadyLink{} | |
| } | |
| type leaseProxyServerReadyLink struct{} | |
| func (*leaseProxyServerReadyLink) SatisfiedBy(other StepLink) bool { | |
| switch other.(type) { | |
| case *leaseProxyServerLink, *leaseProxyServerReadyLink: | |
| return true | |
| default: | |
| return false | |
| } | |
| } | |
| func (*leaseProxyServerReadyLink) UnsatisfiableError() string { | |
| return "" | |
| } |
🤖 Prompt for AI Agents
In `@pkg/api/graph.go` around lines 285 - 316,
LeaseProxyServerReadyLink.SatisfiedBy currently returns true for any StepLink,
which incorrectly marks any requirement satisfied; change the implementation of
leaseProxyServerReadyLink.SatisfiedBy to only return true when the other link is
either *leaseProxyServerLink or *leaseProxyServerReadyLink (mirroring the
pattern used by imagesReadyLink/rpmRepoLink) so that HasAllLinks only treats the
actual lease proxy link (or itself) as satisfying the requirement.
|
|
||
| ret = append(ret, api.LeaseProxyServerLink()) | ||
| return |
There was a problem hiding this comment.
Guard LeaseProxyServerLink on lease-client availability.
Line 328 adds LeaseProxyServerLink() for every multi-stage test, but the lease-proxy step is only injected when LeaseClientEnabled is true. That leaves multi-stage steps with an unsatisfied dependency in jobs without a lease client and can fail graph construction. Please gate this requirement on lease-client availability (e.g., thread a flag into multiStageTestStep) or always add the lease-proxy step so the link is satisfiable.
🤖 Prompt for AI Agents
In `@pkg/steps/multi_stage/multi_stage.go` around lines 328 - 330, The code always
appends api.LeaseProxyServerLink() in the multi-stage step construction, which
creates an unsatisfied dependency when no lease client is present; modify the
multiStageTestStep construction to consult the lease-client availability (e.g.,
add or use a LeaseClientEnabled boolean on the multiStageTestStep struct) and
only append api.LeaseProxyServerLink() when that flag is true in the function
that currently does "ret = append(ret, api.LeaseProxyServerLink())" (or
alternatively ensure the lease-proxy step itself is always added so the link can
be satisfied); update any callers/constructors of multiStageTestStep to thread
the flag through.
|
@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. |
2 participants
Introduce a fake
lease-proxy-serverstep that I will implement with several upcoming PRs.As of now, the step is required by the
multistagestep onlyThe code is dumb: a new step is added to the execution graph and it always succeeds. The
lease-proxy-serverexists only when a lease client is available. It wouldn't make any sense otherwise.