Skip to content
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.

Sign up for GitHub

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

Add default requests and limits to the init containers #2186

Merged
merged 3 commits into from
Dec 4, 2019
Merged

Add default requests and limits to the init containers #2186

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2026a0
Added limits to initpod
Nov 28, 2019
8053444
Better defaults
Dec 4, 2019
ddb41ba
Same limit as request
Dec 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pkg/controller/elasticsearch/initcontainer/prepare_fs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
esvolume "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/volume"
"github.com/elastic/cloud-on-k8s/pkg/utils/stringsutil"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
)

const (
Expand Down Expand Up @@ -77,6 +78,17 @@ var (
},
},
}
//defaultResources are the default request and limits for the init container.
sanderma marked this conversation as resolved.
Show resolved Hide resolved
defaultResources = corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("500Mi"),
corev1.ResourceCPU: resource.MustParse("500m"),
},
Limits: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("1Gi"),
corev1.ResourceCPU: resource.MustParse("1"),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picking good default numbers is pretty hard. I did a few tests, here are my observations.

ES Pod consideration

ES Pod defaults are currently set to 2Gi RAM (requests and limits). And no CPU limitation.
We could set the same defaults for the init container that runs before the Pod since we need those resources anyway, but I think it's cleaner if we aim for lower more realistic values.
Even though no CPU is specified, we can reasonably say "0.1" CPU seems to be a fair strict minimum for an Elasticsearch Pod (I expect "1" to be more realistic) .

CPU requirements

Assigning both request and limit of "100m" CPU to the init container still gives pretty good performance on a n1-standard-8 GKE machine. The initContainer took about a second from start to end. It obviously depend on the underlying CPU, but I think we could aim for a lower request here. "1" seems to be a generally acceptable resource limit, since the value is relative to the underlying CPU. I suggest we lower the request to "100Mi".

Memory requirements

I set both requests and limits to "10Mi" RAM, everything worked fine with a fresh clean cluster. So far the init container only does filesystem operations, there should be no hard RAM requirement. I think there's no reason not to use this limit: if it the initContainer breaks we'll notice it and realize we probably did something wrong.

Suggestion

I think we could use the following values:

defaultResources = corev1.ResourceRequirements{
		Requests: map[corev1.ResourceName]resource.Quantity{
			corev1.ResourceMemory: resource.MustParse("10Mi"),
			corev1.ResourceCPU:    resource.MustParse("0.1"),
		},
		Limits: map[corev1.ResourceName]resource.Quantity{
			corev1.ResourceMemory: resource.MustParse("10Mi"),
			corev1.ResourceCPU:    resource.MustParse("1"),
		},
	}

Let me know if that makes sense to you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 overall but I think we want to make sure requests == limits, otherwise the init containers default settings will keep the pod from having guaranteed QoS even if the user sets the elasticsearch requests == limits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'll change it now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanderma I think Anya is right here, we should probably aim for requests==limits.
I think it's fine to request "0.1" CPU for both requests and limits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree. It is usually better to set it that way. It's only useful to have a higher limit if you expect infrequent bursts in resource consumption.
I'll set them to 0.1.

}
)

// NewPrepareFSInitContainer creates an init container to handle things such as:
Expand Down Expand Up @@ -117,6 +129,7 @@ func NewPrepareFSInitContainer(
esvolume.DefaultDataVolumeMount,
esvolume.DefaultLogsVolumeMount,
),
Resources: defaultResources,
}

return container, nil
Expand Down