[Antithesis] Add the client part for k8s setup #20668
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nwnt The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Also, what I didn't include in this PR was also my refactoring of how the current |
nwnt
force-pushed
the
k8s-antithesis-client
branch
4 times, most recently
from
1fbe07e to
6a2985d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 14 files with indirect coverage changes @@ Coverage Diff @@
## main #20668 +/- ##
==========================================
- Coverage 69.16% 69.14% -0.03%
==========================================
Files 420 420
Lines 34794 34794
==========================================
- Hits 24065 24058 -7
- Misses 9334 9337 +3
- Partials 1395 1399 +4 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| - '${GROUP_ID:-root}' | ||
| volumes: | ||
| - ${ETCD_ROBUSTNESS_DATA_PATH:-/tmp/etcddata}0:/var/etcddata0 | ||
| - ${ETCD_ROBUSTNESS_LOCAL_DATA_PATH:-/tmp/etcddata}0:/var/etcddata0 |
There was a problem hiding this comment.
What's the value of this rename? This only complicates the review. Please move it to separate PR.
There was a problem hiding this comment.
See below. It's for distinguishing the existing local data path and all paths.
| mountPath: /var/etcddata-1 | ||
| - name: etcd-2 | ||
| mountPath: /var/etcddata-2 | ||
| - name: report |
There was a problem hiding this comment.
Why not use hostvolume for report? What's the benefit of PVC here?
There was a problem hiding this comment.
I was thinking it might be better to keep things consistent than mixing between hostPath and PVC. Let me know if you want it to be hostPath instead.
There was a problem hiding this comment.
I prefer clear and understandable. While we need to use PVCs for etcd as there are 3 replicas, I don't see a reason why we cannot have a static path for client report.
There was a problem hiding this comment.
Changed to hostPath for report now.
tests/antithesis/README.md
Outdated
| It should show something like: | ||
|
|
||
| ``` | ||
| ```bash |
There was a problem hiding this comment.
Please avoid random changes unrelated to the task. They can be done in separate PR.
There was a problem hiding this comment.
Normally I would. I got a linting error from the checks and it said I needed to address this, that's why it's here.
There was a problem hiding this comment.
Still please send a separate PR as it can get merged in seconds.
There was a problem hiding this comment.
Removed these changes from the current PR.
| localReportPath = "report" | ||
|
|
||
| serverEndpointEnv = "ETCD_ROBUSTNESS_SERVER_ENDPOINTS" | ||
| dataPathEnv = "ETCD_ROBUSTNESS_DATA_PATHS" |
There was a problem hiding this comment.
What's the benefit of separating ETCD_ROBUSTNESS_DATA_PATH and ETCD_ROBUSTNESS_LOCAL_DATA_PATH? From binary point of view both are local.
There was a problem hiding this comment.
How the existing local data path is used is different from the new one. The existing one takes a path (more like a prefix) and then add counts to it. The new one no longer assumes such patterns and requires that all paths are provided as a comma separated value.
Also, I do not want to rework how the existing data path works in this PR to keep things simple. I can send a follow up PR to refactor and consolidate them.
There was a problem hiding this comment.
We should avoid over-complicating the existing code, especially avoid introducing two mechanism for the same thing. If the PR is too complicated it means it should be split.
There was a problem hiding this comment.
Will be reworking how the existing code currently handles the paths it uses as I mentioned here then. Right now all the paths are baked into the code. I will be sending a PR for that change before continuing this one.
Signed-off-by: Nont <[email protected]>
nwnt
force-pushed
the
k8s-antithesis-client
branch
from
6a2985d to
641f796
Compare
I'm keeping this simple for now by assuming a fixed number of replicas and assuming the etcd data volumes are known beforehand. I have a somewhat working code that can deal with dynamic etcd replicas, but I'll add that once we make this work on antithesis.
@serathius