-
Notifications
You must be signed in to change notification settings - Fork 10.2k
[Antithesis] Add the client part for k8s setup #20668
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,3 +101,55 @@ spec: | |
resources: | ||
requests: | ||
storage: 200Mi | ||
--- | ||
apiVersion: apps/v1 | ||
kind: Deployment | ||
metadata: | ||
name: client | ||
labels: | ||
app: client | ||
spec: | ||
replicas: 1 | ||
selector: | ||
matchLabels: | ||
app: client | ||
template: | ||
metadata: | ||
labels: | ||
app: client | ||
spec: | ||
containers: | ||
- name: etcd-client | ||
image: etcd-client:latest | ||
command: ["/opt/antithesis/entrypoint/entrypoint"] | ||
imagePullPolicy: Never | ||
env: | ||
- name: ETCD_ROBUSTNESS_SERVER_ENDPOINTS | ||
value: "etcd-0.etcd.default.svc.cluster.local:2379,etcd-1.etcd.default.svc.cluster.local:2379,etcd-2.etcd.default.svc.cluster.local:2379" | ||
- name: ETCD_ROBUSTNESS_DATA_PATHS | ||
value: "/var/etcddata-0,/var/etcddata-1,/var/etcddata-2" | ||
- name: ETCD_ROBUSTNESS_REPORT_PATH | ||
value: "/var/report/" | ||
volumeMounts: | ||
- name: etcd-0 | ||
mountPath: /var/etcddata-0 | ||
- name: etcd-1 | ||
mountPath: /var/etcddata-1 | ||
- name: etcd-2 | ||
mountPath: /var/etcddata-2 | ||
- name: report | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use hostvolume for report? What's the benefit of PVC here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to |
||
mountPath: /var/report/ | ||
volumes: | ||
- name: etcd-0 | ||
persistentVolumeClaim: | ||
claimName: data-etcd-0 | ||
- name: etcd-1 | ||
persistentVolumeClaim: | ||
claimName: data-etcd-1 | ||
- name: etcd-2 | ||
persistentVolumeClaim: | ||
claimName: data-etcd-2 | ||
- name: report | ||
hostPath: | ||
path: /var/report/ | ||
type: DirectoryOrCreate |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ package common | |
import ( | ||
"fmt" | ||
"os" | ||
"strings" | ||
) | ||
|
||
const ( | ||
|
@@ -34,8 +35,12 @@ const ( | |
localetcd2 = "127.0.0.1:32379" | ||
// used by default when running the client locally | ||
defaultetcdLocalDataPath = "/tmp/etcddata%d" | ||
localetcdDataPathEnv = "ETCD_ROBUSTNESS_DATA_PATH" | ||
localetcdDataPathEnv = "ETCD_ROBUSTNESS_LOCAL_DATA_PATH" | ||
localReportPath = "report" | ||
|
||
serverEndpointEnv = "ETCD_ROBUSTNESS_SERVER_ENDPOINTS" | ||
dataPathEnv = "ETCD_ROBUSTNESS_DATA_PATHS" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the benefit of separating There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
reportPathEnv = "ETCD_ROBUSTNESS_REPORT_PATH" | ||
) | ||
|
||
func DefaultPaths(cfg *Config) (hosts []string, reportPath string, dataPaths map[string]string) { | ||
|
@@ -64,3 +69,25 @@ func etcdDataPaths(dir string, amount int) map[string]string { | |
} | ||
return dataPaths | ||
} | ||
|
||
func PathsFromEnv() (hosts []string, reportPath string, dataPaths map[string]string) { | ||
if h, ok := os.LookupEnv(serverEndpointEnv); ok { | ||
hosts = strings.Split(h, ",") | ||
} | ||
var data []string | ||
if d, ok := os.LookupEnv(dataPathEnv); ok { | ||
data = strings.Split(d, ",") | ||
} | ||
|
||
if len(hosts) != len(data) { | ||
panic("The number of endpoints and data paths must be equal") | ||
} | ||
|
||
dataPaths = make(map[string]string) | ||
for i := range hosts { | ||
dataPaths[hosts[i]] = data[i] | ||
} | ||
reportPath = os.Getenv(reportPathEnv) | ||
|
||
return hosts, reportPath, dataPaths | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below. It's for distinguishing the existing local data path and all paths.