feat: add v2 pod controller w/ workload lifecycle #2868
Conversation
DanStough
added
pr/no-changelog
control-plane/connect-inject/controllers/endpoints/consul_client_health_checks.go
Show resolved
Hide resolved
There was a problem hiding this comment.
This is looking great to me, and I really appreciate having your test setup as an example while I'm trying to write my own.
Some non-blocking comments about potential shared helpers, but overall what you have so far looks reasonable to me. Will look forward to seeing any other feedback around functional req's.
DanStough
force-pushed
the
dans/NET-5315/v2-pods-controller
branch
from
be40389
to
86f7cbb
Compare
DanStough
force-pushed
the
dans/NET-5315/v2-pods-controller
branch
2 times, most recently
from
6b7d186
to
cd88fa0
Compare
DanStough
force-pushed
the
dans/NET-5315/v2-pods-controller
branch
from
cd88fa0
to
ea4d003
Compare
|
|
||
| err = pc.writeWorkload(context.Background(), *tc.pod) | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
Does there need to be a ResourceHasPersisted check here before the read in L175?
There was a problem hiding this comment.
I think it's not needed here because we are doing a read after write, and in this case, we don't care if the Consul controller has reconciled the resource yet. In the other cases, we are overwriting/deleting resources, which had the CAS behavior at the time of testing.
There was a problem hiding this comment.
I could also totally be mistaken, so please correct me. I didn't see any flakes with the test, unlike the others.
There was a problem hiding this comment.
In the other cases, we are overwriting/deleting resources, which had the CAS behavior at the time of testing.
Ah ok, so the delete wasn't going through sometimes, unless you know consul has persisted? As opposed to in this case, the read is being done from some higher level client/cache?
I think that makes sense, I just wanted to understand the difference between this case and the others! If you ran something like a -count=100 without flakes I think it's fine!
This PR is part of a series of PRs that add functionality to the V2 Pod Controller. For the MVP task to be complete, there will be one additional PR to fill in the lifecycle of HealthStatus and ProxyConfiguration resources.
TODO(dans)comments are needed for the MVP tasks. OtherTODOsare captured in future stories.Changes proposed in this PR:
How I've tested this PR: Unit tests
How I expect reviewers to test this PR: 👓
Checklist: