(fix) registry pods do not come up again after node failure

[anik120]

Description of the change:

PR 3201 attempted to solve for the issue by deleting the pods stuck in
Terminating due to unreachable node. However, the logic to do that was
included in EnsureRegistryServer, which only gets executed if polling in
requested by the user.

This PR moves the logic of checking for dead pods out of EnsureRegistryServer,
and puts it in CheckRegistryServer instead. This way, if there are any dead pods
detected during CheckRegistryServer, the value of healthy is returned false,
which inturn triggers EnsureRegistryServer.

Motivation for the change:

Architectural changes:

Testing remarks:

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Bug fixes are accompanied by regression test(s)
• e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
• tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
• Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
• Docs updated or added to /doc
• Commit messages sensible and descriptive
• Tests marked as [FLAKE] are truly flaky and have an issue
• Code is properly formatted

[perdasilva]

So far it looks pretty good =D should we add any unit tests for the new behaviour? and maybe the test we missed in the original bug fix?

[anik120]

So far it looks pretty good =D should we add any unit tests for the new behaviour? and maybe the test we missed in the original bug fix?

I was scared you'll ask this. :p

I did think about it, looked into it, and it looks like it's going to be a little bit of effort to write a test for this. Mainly because it's not really unit testable so an e2e test is the viable option. However, I'm not sure we can mimic a node going down from within our tests.

I just figured the squeeze might not be worth the juice, however if someone has a better idea (or really insist we include a test for this even if it'll require some effort), I'm happy to include one in this PR.

[perdasilva]

What's the signal

So far it looks pretty good =D should we add any unit tests for the new behaviour? and maybe the test we missed in the original bug fix?

I was scared you'll ask this. :p

I did think about it, looked into it, and it looks like it's going to be a little bit of effort to write a test for this. Mainly because it's not really unit testable so an e2e test is the viable option. However, I'm not sure we can mimic a node going down from within our tests.

I just figured the squeeze might not be worth the juice, however if someone has a better idea (or really insist we include a test for this even if it'll require some effort), I'm happy to include one in this PR.

What's the signal we get in the code that a node has gone down? Are some pods unreachable?

[anik120]

What's the signal we get in the code that a node has gone down? Are some pods unreachable?

We don't really get any signal that a node's gone down, we just discover pod/s that have been Deleted, but they still showed up in the result of list pods action. So we say "something's awry, let's force clean this area and move on".

And that "discovery" is done here. We could add a test for that, but it's a pretty simple function.

The real useful test would have been if we could mimic pods "hanging around" in an e2e setting. In fact, the entire syncRegistryServer functionality is mostly e2e tested, this is the only unit test that's there.

[anik120]

I may have found a way to e2e test this......working on it now....

[perdasilva]

t discover pod/s that have been Deleted

Would it be possible to mock those client responses? e.g. list says they are there, get says they aren't?

[anik120]

Okay I have some tests now.

I may have found a way to e2e test this......working on it now....

This did not work out. I was thinking about using gracefulDeletionPeriod as a hack, but that's not going to be a very reliable test (since gracefulDeletionPeriod doesn't necessarily force the pod to hang around for the period specified after it's been deleted, like I was hoping it would. Instead, it looks like as soon as the container in the pod stops, the GC comes and deletes it, which in our case is very quick. ie the registry server is shut down immediately so the pod will be cleaned up immediately too.)

Would it be possible to mock those client responses? e.g. list says they are there, get says they aren't?

I went with the unit tests route, that mocks these interactions. So the entire feature is tested by 3 components of tests:

1. The CleanRegistryServer for both Grpc and ConfigMap is tested individually in the reconciler package
   note: it's more clear that the CleanRegistryServer is getting called because of the presence of a "deleted" pod when the test is run in verbose mode:

go test ./pkg/controller/registry/reconciler -run TestGrpcRegistryCleaner -v
=== RUN   TestGrpcRegistryCleaner
=== RUN   TestGrpcRegistryCleaner/Grpc/ExistingRegistry/DeletedPod
time="2024-08-19T16:42:20-04:00" level=info msg="force deleting dead pod" pod.name= pod.namespace=testns
--- PASS: TestGrpcRegistryCleaner (0.10s)
    --- PASS: TestGrpcRegistryCleaner/Grpc/ExistingRegistry/DeletedPod (0.10s)
PASS
ok  	github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/reconciler	0.939s

force deleting dead pod is logged.

2. A level above, in the operator package (that uses the reconciler implementations), a unit test is put in place to check that reconciler.CleanRegistryServer is called.

This is the absolute best we can do. e2e isn't possible without much more digging in, and frankly at this point not necessary at all.

[anik120]

Pivoted the PR to include the logic inside CheckRegistryServer, instead of adding a CleanRegistryServer component to the interface.

[joelanford]

Do we need to return an error from this function when we find a wedged pod and successfully delete it? My initial thought is that we would only error from this function if:

• we failed to determine healthy
• we failed to delete wedged pods.

[anik120]

I started out thinking the same, but then realized that that'd be an "artificial" error. If we include both of the scenarios you mentioned in the definition of "error", we're sort of sullying the definition which is "something went wrong". In the case of "we failed to determine healthy", we are already sending that signal through the boolean variable anyway. So decided to not include both, and only include the second scenario of "we failed to delete the wedged pods".

[joelanford]

This is trickier than I originally thought. If we have:

1. at least one alive pod
2. the alive pods are otherwise deemed healthy
3. we successfully delete the dead pods

What should we return from CheckRegistryServer? Seems like we could say healthy in that case?

[anik120]

Yea this is a bit confusing, I'm thinking if we detect even one dead pod, we just force delete all the pods, and let EnsureRegistryServer recreate everything. Otherwise we're in a very non-deterministic state...

[joelanford]

That seems like that could have significant performance implications. There's a lot of disk/cpu/memory costs that are paid when catalog pods are started, so we should minimized that as much as possible.

I think it is okay to force delete the dead pods, and if there are any alive pods, we just move forward as if those were the only pods that were there to begin with.

[anik120]

That's fair too. I was a bit confused about there being multiple pods in the first place, since my impression is that we only create one registry pod per catalog, but turns out we have pods mainly because we use the lister to list via label selector. In any case this should return one pod, unless I'm unaware of some feature that allows us to spin up multiple registry pods per catalog.

Changed it back to just deleting dead pods.

[joelanford]

There are definitely multiple pods when we're polling. In that case, we spin up a new pod to see what the digest is, and we compare the digests to see if we need to use the new pod (if it has a new digest) or keep using the old pod (when the digests match)

There may also be multiple pods when the pod spec changes (e.g. when the catalog source image is changed).

[anik120]

Right, both are transitory states though, and we're in that state because EnsureRegistryServer has already been called. But yes I think this is in a good state now.

[tmshort]

@joelanford are you good with this?

[joelanford]

I still think we should return true, nil from this function in the case where:

• There is at least one healthy pod
• There are multiple "dead" pods
• We deleted all of the "dead" pods successfully.

That sort of behavior would mean that we would short circuit and avoid calling EnsureRegistryServer, which would:

• end up making some no-op calls to the apiserver:
  • e.g. https://github.com/anik120/operator-lifecycle-manager/blob/20a171690cf96fcea177e5fa90426e7c234a14d3/pkg/controller/registry/reconciler/grpc.go#L459
• (maybe, not sure) have some strange issues related to caching, where subsequent calls to currentPods would still return the dead pods because the deletion has no propagated back to our informer cache yet (I'm assuming c.Lister is backed by a cache. If it is not, then we'd be making more apiserver calls unnecessarily)

[joelanford]

Essentially, I think both of the following should result in identical behavior:

1. There are 1 or more alive pods, and no dead pods
2. There are 1 or more alive pods, and we deleted all of the dead pods (hence: there are no dead pods, so this is actually a variant of (1))

[anik120]

@joelanford thanks for the discussion yesterday. Thought about it a little more and we can do this. I've had to add an additional change though, highlighting it in the next comment...