I think we can remove the TODO in line 164 now.

That TODO refers to an improvement made possible by ControllerRef, but we are not attempting to implement that improvement here, so the TODO still stands.

There is only one value in controllerScale, why use a loop?

@CaoShuFeng yes will fix once i fix the tests

This comment needs update.

This comment needs update.

That TODO refers to an improvement made possible by ControllerRef, but we are not attempting to implement that improvement here, so the TODO still stands.

Please retain the comment explaining why we are ignoring the error (returning nil).

Specifically: the only possible error when fetching from the local cache is NotFound. If the referenced ReplicaSet does not exist, we consider that as successfully finding that the Pod is not controlled by any ReplicaSets.

In Go style, we capitalize acronyms: newPodOwnedByRC. Same for RS, SS, etc.

I think this may be why the test is failing, at least for RS. If controllerRef == nil, we actually want to return the RS without any further checking.

The idea here is, if the RS is a top-level controller, we return it. If the RS is controlled by a Deployment, we don't return it, because we instead want the Deployment itself to be returned by getPodDeployments. If the RS is controlled by something that isn't a Deployment, we should still return it because we don't know the semantics of that unknown thing.

There's an opportunity now to simplify and optimize this whole process. We can actually get rid of the finders and have one function called something like getPodControllerScale. It would check the Pod's ControllerRef and figure out the scale somehow depending on the type of controller. Let me know if you want to pursue that longer-term fix and we can talk about it. It's also fine if you just want to finish this intermediate fix first and leave the refactoring for later.

In Go style, we try to keep the "error-free" path as unindented as possible. In this case, that means:

controllerRef := ...
if controllerRef == nil {
  return cas, nil
}
deployment, err := ...

Similarly here:

if err != nil {
  return cas, nil
}
``

statefulset is part of the apps group.

rc is part of the core legacy group

var casSlice []controllerAndScale

if controllerRef == nil || controllerRef.Kind != controllerKindDep.Kind {

var ...

Please change this for all instances.

After fetching the RS by name, check that the UID matches controllerRef.UID. If it doesn't match, the RS you fetched doesn't actually own the pod; it just has the same name as a previously-deleted RS that owned the pod. In that case, you can return nil, nil since the pod has no RS owner.

Please add back the comment that explains why we ignore err: It's because the only possible error returned by the cache is NotFound, and that means there is no RS for this pod.

Go style suggestion: This line is the end of the "happy path" (everything went as expected - we found an RS). Ideally it should be as unindented as possible, and unexpected things should result in early return statements. This way the "happy path" flows straight down rather than becoming more deeply nested at each step.

As an example, this function could be structured like:

...
if controllerRef == nil {
  return nil, nil
}
if controllerRef.Kind != controllerKindRS.Kind {
  return nil, nil
}
rs, err := ...
if err != nil {
  // The only possible error is NotFound, which is ok here.
  return nil, nil
}
if rs.UID != controllerRef.UID {
  return nil, nil
}
...
if controllerRef != nil && controllerRef.Kind == controllerKindDep.Kind {
  // Skip RS if it's controlled by a Deployment.
  return nil, nil
}
return []controllerAndScale{...}

Notice there's no nesting and everything flows straight down.

Check UID after Get.

Add back comment about NotFound error being ok.

Check UID after Get.

Remove raw print or convert to t.Log().

Can this test be split into multiple test cases, so we don't modify pods that were already added to the pod store? In my experience, that can lead to flaky/racy tests because you are changing things you already gave to the pod store, which it is not designed to support.

If these need to be in one test case/func, is it possible to add new pods rather than changing existing ones?

took care of all other comments. will try this and see how it works. Not very familiar with this framework

This PR LGTM other than the potential flakiness here. If you want, we can just try to measure whether this is going to be flaky and submit it as is if it passes.

One way to do that is something like go test -count 1000 or go test -race -count 1000. You might want to start with a smaller count and work up to what your machine can handle. It seems that Go sometimes runs the tests concurrently and I've locked up my workstation before by putting a count that was too high. :)

go test ./pkg/controller/disruption/... --count 300 --race
race: limit on 8192 simultaneously alive goroutines is exceeded, dying
FAIL k8s.io/kubernetes/pkg/controller/disruption 16.577s

go test ./pkg/controller/disruption/... --count 250 --race
ok k8s.io/kubernetes/pkg/controller/disruption 16.575s

I cant go beyond 250 on my mac. Is there a way to request this count with the prbot ?

Try running higher counts, but without -race.

Thanks. Let me know when you've resolved the merge conflicts and I will LGTM.

@enisoc resolved conflicts.

Remove or convert to t.Log().

Can you use the vars you already defined in the other file?

We also have Deployments in the apps group. I think you need to use both but I am not fairly certain.

Hm, you just compare UID and Kind so it may not be needed after all

Since it's only one parent that you are returning now, switch all these helpers to return a single object.

@Kargakis i was hoping to do that change in a subsequent PR, since that would also involve modifying the callers and understanding that logic. Is that ok ?

sure