Fix scale up status processor overriding default one with proactive s… #7275
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @abdelrahman882. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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. |
k8s-ci-robot
added
the
size/M
| opts.Processors.ScaleUpStatusProcessor = podinjection.NewFakePodsScaleUpStatusProcessor(podInjectionBackoffRegistry) | ||
| // FakePodsScaleUpStatusProcessor processor needs to be the first processor in ScaleUpStatusProcessor before the default processor | ||
| // As it filters out fake pods from Scale Up status so that we don't emit events. | ||
| opts.Processors.ScaleUpStatusProcessor = status.NewCombinedScaleUpStatusProcessor([]status.ScaleUpStatusProcessor{podinjection.NewFakePodsScaleUpStatusProcessor(podInjectionBackoffRegistry), opts.Processors.ScaleUpStatusProcessor}) |
There was a problem hiding this comment.
I don't think that it is nill, but can we have a nullability check either here or (preferably) in the combined processor?
There was a problem hiding this comment.
opts.Processors.ScaleUpStatusProcessor can't be null as it's initialized from default processors, but I agree that it's a better practice to check processors existence in the combinedProcessor implementation in case other processors added in the future
abdelrahman882
force-pushed
the
fix_scaleup_status_issue
branch
from
0cbf788
to
38ce500
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abdelrahman882, x13n The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| func NewCombinedScaleUpStatusProcessor(processors []ScaleUpStatusProcessor) *CombinedScaleUpStatusProcessor { | ||
| var scaleUpProcessors []ScaleUpStatusProcessor | ||
| for _, processor := range processors { | ||
| if processor != nil { |
There was a problem hiding this comment.
When you check nil for an interface, you have to do this:
processor != nil && !reflect.ValueOf(processor).IsNil()
There was a problem hiding this comment.
In fact, doing it using the suggestion might also break if the interface value is a struct and not a pointer.
I think this is better discussed in bigger scale, so no action required here.
|
|
||
| // AddProcessor append processor to the list. | ||
| func (p *CombinedScaleUpStatusProcessor) AddProcessor(processor ScaleUpStatusProcessor) { | ||
| if processor != nil { |
There was a problem hiding this comment.
When you check nil for an interface, you have to do this:
processor != nil && !reflect.ValueOf(processor).IsNil()
There was a problem hiding this comment.
In fact, doing it using the suggestion might also break if the interface value is a struct and not a pointer.
I think this is better discussed in bigger scale, so no action required here.
[Let's leave the discussion to the other comment since they are the same]
…caleup enabled
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes a bug in recent introduced feature ( proactive scale-up) , scale-up status processor was overriding the default one instead of running before it
Which issue(s) this PR fixes:
non
Special notes for your reviewer:
original feature pr : #7145
Does this PR introduce a user-facing change?
no