Skip to content

🌱 (e2e): test: skip terminating pods in catalog image update test #3703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

🌱 (e2e): test: skip terminating pods in catalog image update test #3703

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions test/e2e/catalog_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"

"k8s.io/apimachinery/pkg/util/intstr"

"net"
Expand Down Expand Up @@ -1169,6 +1170,10 @@ var _ = Describe("Starting CatalogSource e2e tests", Label("CatalogSource"), fun
}

for _, pod := range podList.Items {
// Skip terminating pods
if pod.DeletionTimestamp != nil {
continue
}
ctx.Ctx().Logf("old image id %s\n new image id %s\n", registryPod.Items[0].Status.ContainerStatuses[0].ImageID,
pod.Status.ContainerStatuses[0].ImageID)
if pod.Status.ContainerStatuses[0].ImageID != registryPod.Items[0].Status.ContainerStatuses[0].ImageID {
Expand All @@ -1194,10 +1199,8 @@ var _ = Describe("Starting CatalogSource e2e tests", Label("CatalogSource"), fun
return false
}
By("await new catalog source and ensure old one was deleted")
registryPods, err = awaitPodsWithInterval(GinkgoT(), c, source.GetNamespace(), selector.String(), 30*time.Second, 10*time.Minute, podCheckFunc)
_, err = awaitPodsWithInterval(GinkgoT(), c, source.GetNamespace(), selector.String(), 30*time.Second, 10*time.Minute, podCheckFunc)
Expect(err).ShouldNot(HaveOccurred(), "error awaiting registry pod")
Expect(registryPods).ShouldNot(BeNil(), "nil registry pods")
Expect(registryPods.Items).To(HaveLen(1), "unexpected number of registry pods found")
Comment on lines -1199 to -1200
Copy link
Collaborator

@perdasilva perdasilva Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to remove these checks? Would it be reasonable to expect that there's at least one registry pod?

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because they are not part of the goal what we want to check
And in downstream we do not execute those in SERIAL so we can have more than 1 pod.

The check:

Expect(err).ShouldNot(HaveOccurred(), "error awaiting registry pod")

It is enough to validate what we want to validate.
By looking the failure it seems that I introduced a flake (downstream) with this test.
That was my motivation for the changes here.

Copy link
Member

@anik120 anik120 Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra checks never hurt.

And in downstream we do not execute those in SERIAL so we can have more than 1 pod.

These are namespace scoped right?

My ideal solution here would be to not remove these checks in this PR, observe if we're still getting flaky results, and then look into why we are (possibly) "still failing".

My main concern is that removing these checks reduces the sanctity of this test, at least the way it reads serially (and was intended to be read)

Copy link
Collaborator

@perdasilva perdasilva Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's ok to remove since the checks are embedded in the podCheck function. For awaitPodsWithInterval to succeed, podCheck needs to return true, which only happens when there's at least one pod in the new image.

Wdyt about using something like slices.DeleteFunc to filter out the terminating pods from the podList.Items and returning true when len == 1 and the pod image != registry image?

Then maybe just adding a comment that says the podCheck function checks the requirements. If it succeeds, so does the test.


By("update catalog source with annotation (to kick resync)")
Eventually(func() error {
Expand Down