test: Eviction E2E #996
test: Eviction E2E #996
Conversation
Arvindthiru
force-pushed
the
rolloutEvictionE2E
branch
from
4ff6b3b to
028709b
Compare
|
Addressed comment from #959 (comment) |
Arvindthiru
force-pushed
the
rolloutEvictionE2E
branch
2 times, most recently
from
70f5a4f to
bbb380e
Compare
Arvindthiru
force-pushed
the
rolloutEvictionE2E
branch
2 times, most recently
from
e104b7d to
5abf54a
Compare
Arvindthiru
force-pushed
the
rolloutEvictionE2E
branch
from
2fb1fc8 to
c754100
Compare
| } | ||
| Expect(hubClient.Create(ctx, &crpdb)).To(Succeed(), "Failed to create CRP Disruption Budget %s", crpName) | ||
|
|
||
| ensureCRPDisruptionBudgetExists := ensureCRPDisruptionBudgetExists(crpName) |
There was a problem hiding this comment.
Hi Arvind! Just a nit here: since we have switched to uncached reader here; there might no longer be any need to check for DB existence here any more. (Successful creation should guarantee successful DB reads)
There was a problem hiding this comment.
I'm thinking of leaving it there, to ensure the E2E test itself is not the cause for any future flaky behavior
There was a problem hiding this comment.
I concur with Chen, this is a guaranteed behavior by k8s, no need to check regardless.
michaelawyu
left a comment
michaelawyu
left a comment
There was a problem hiding this comment.
Added some comments, PTAL.
|
A note for the uncached reader change: when we have more time on hand it might be interesting to explore a bit why the DB objects are being watched and cached by the clients. |
| type Reconciler struct { | ||
| client.Client | ||
| // UncachedReader is only used to read disruption budget objects directly from the API server to ensure we can enforce the disruption budget for eviction. | ||
| UncachedReader client.Reader |
There was a problem hiding this comment.
i am not sure why we need this
There was a problem hiding this comment.
I noticed flaky behavior when I was writing E2Es where I create an PDB followed by an eviction, but the eviction would successfully execute saying no PDB exists. After ruling all possible E2E related flaky behavior scenarios @michaelawyu helped me figure out it's a cache miss hence the uncached reader to ensure disruption budget is found by the eviction controller by reading from API server when reconciling an eviction object
| } | ||
| Expect(hubClient.Create(ctx, &crpdb)).To(Succeed(), "Failed to create CRP Disruption Budget %s", crpName) | ||
|
|
||
| ensureCRPDisruptionBudgetExists := ensureCRPDisruptionBudgetExists(crpName) |
There was a problem hiding this comment.
I concur with Chen, this is a guaranteed behavior by k8s, no need to check regardless.
3 participants
Description of your changes
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
Added uncached reader for eviction controller to ensure PDB can be read directly from API server if it exists to avoid cache miss