-
Notifications
You must be signed in to change notification settings - Fork 5k
Bump min required memory to 3GB and Fix KVM driver (tests) timeouts #20852
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
Changes from all commits
acb9d57
7484b51
5c6f7db
5b0b753
2b81ce2
6a0613f
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 |
---|---|---|
|
@@ -145,7 +145,7 @@ func getLabels(containerRuntime string) *stackdriver.Labels { | |
func minikubeStartTime(ctx context.Context, projectID, minikubePath, containerRuntime string) (float64, error) { | ||
defer deleteMinikube(ctx, minikubePath) | ||
|
||
cmd := exec.CommandContext(ctx, minikubePath, "start", "--driver=docker", "-p", profile, "--memory=2048", "--trace=gcp", fmt.Sprintf("--container-runtime=%s", containerRuntime)) | ||
cmd := exec.CommandContext(ctx, minikubePath, "start", "--driver=docker", "-p", profile, "--memory=3072", "--trace=gcp", fmt.Sprintf("--container-runtime=%s", containerRuntime)) | ||
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. btw this is for the Docker Driver, do we even need this ? (this is benchmarking) 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. it's definitelly not "critical", but i think that now that we know we can expect issues with less than 3gb mem, my thinking is trying to avoid potential flakiness, including here |
||
cmd.Env = append(os.Environ(), fmt.Sprintf("%s=%s", pkgtrace.ProjectEnvVar, projectID)) | ||
cmd.Stdout = os.Stderr | ||
cmd.Stderr = os.Stderr | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,16 +27,16 @@ const domainTmpl = ` | |
<acpi/> | ||
<apic/> | ||
<pae/> | ||
{{if .Hidden}} | ||
{{- if .Hidden}} | ||
<kvm> | ||
<hidden state='on'/> | ||
</kvm> | ||
{{end}} | ||
{{- end}} | ||
</features> | ||
<cpu mode='host-passthrough'> | ||
{{if gt .NUMANodeCount 1}} | ||
{{- if gt .NUMANodeCount 1}} | ||
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. was this not working in past ? seems to be a new syntax ? 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're using that syntax to avoid printing empty lines where conditions are not met - we use that in a number of other templates already - eg: v1beta4.go |
||
{{.NUMANodeXML}} | ||
{{end}} | ||
{{- end}} | ||
</cpu> | ||
<os> | ||
<type machine='virt-4.2' arch='aarch64'>hvm</type> | ||
|
@@ -75,12 +75,12 @@ const domainTmpl = ` | |
<rng model='virtio'> | ||
<backend model='random'>/dev/random</backend> | ||
</rng> | ||
{{if .GPU}} | ||
{{- if .GPU}} | ||
{{.DevicesXML}} | ||
{{end}} | ||
{{if gt .ExtraDisks 0}} | ||
{{- end}} | ||
{{- if gt .ExtraDisks 0}} | ||
{{.ExtraDisksXML}} | ||
{{end}} | ||
{{- end}} | ||
</devices> | ||
</domain> | ||
` |
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.
I believe this means, we are bumping the mininum required memory for minikube ? if yes and if needed how about be done in an explicit separate PR that can get attention (since affects all drivers)
all other refactors seems to be doing good by unifying the error message wording
Uh oh!
There was an error while loading. Please reload this page.
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.
i think that bumping minimum required memory now, after we bumped several components starting from buildroot, became mandatory and also core to fix this issue, but i don't mind opening a separate pr for just memory bump if we think that would help, although it's coupled with few other improvements here on how we wait machine to get an ip
btw, we're also seeing other examples of failures with less than 3gb mem - like this one
i was able to replicate the issue with at least these ten kvm driver tests consistently failing (whilst all having in common originally using 2gb mem):
here's couple console capture examples of kernel panic due to oom:


i've also added the ability to capture and log the vm console outputs in this pr - that should help future debugging as well, eg: