-
Notifications
You must be signed in to change notification settings - Fork 827
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
feat: make vm memory overhead configurable #1953
Changes from all commits
be45f35
29a40e8
74c15d1
5c153df
95db488
a5f5db1
a743baa
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 |
---|---|---|
@@ -0,0 +1,52 @@ | ||
package main | ||
|
||
import ( | ||
"flag" | ||
"fmt" | ||
"log" | ||
"os" | ||
"strings" | ||
|
||
"github.com/aws/karpenter/pkg/utils/options" | ||
) | ||
|
||
func main() { | ||
if len(os.Args) != 2 { | ||
log.Printf("Usage: %s path/to/markdown.md", os.Args[0]) | ||
os.Exit(1) | ||
} | ||
outputFileName := os.Args[1] | ||
mdFile, err := os.ReadFile(outputFileName) | ||
if err != nil { | ||
log.Printf("Can't read %s file: %v", os.Args[1], err) | ||
os.Exit(2) | ||
} | ||
|
||
genStart := "[comment]: <> (the content below is generated from hack/docs/configuration_gen_docs.go)" | ||
genEnd := "[comment]: <> (end docs generated content from hack/docs/configuration_gen_docs.go)" | ||
startDocSections := strings.Split(string(mdFile), genStart) | ||
if len(startDocSections) != 2 { | ||
log.Fatalf("expected one generated comment block start but got %d", len(startDocSections)-1) | ||
} | ||
endDocSections := strings.Split(string(mdFile), genEnd) | ||
if len(endDocSections) != 2 { | ||
log.Fatalf("expected one generated comment block end but got %d", len(endDocSections)-1) | ||
} | ||
topDoc := fmt.Sprintf("%s%s\n\n", startDocSections[0], genStart) | ||
bottomDoc := fmt.Sprintf("\n%s%s", genEnd, endDocSections[1]) | ||
|
||
opts := options.New() | ||
|
||
envVarsBlock := "| Environment Variable | CLI Flag | Description |\n" | ||
envVarsBlock += "|--|--|--|\n" | ||
opts.VisitAll(func(f *flag.Flag) { | ||
envVarsBlock += fmt.Sprintf("| %s | %s | %s |\n", strings.ReplaceAll(strings.ToUpper(f.Name), "-", "_"), "\\-\\-"+f.Name, f.Usage) | ||
}) | ||
|
||
log.Println("writing output to", outputFileName) | ||
f, err := os.Create(outputFileName) | ||
if err != nil { | ||
log.Fatalf("unable to open %s to write generated output: %v", outputFileName, err) | ||
} | ||
f.WriteString(topDoc + envVarsBlock + bottomDoc) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,20 @@ func WithDefaultInt(key string, def int) int { | |
return i | ||
} | ||
|
||
// WithDefaultFloat64 returns the float64 value of the supplied environment variable or, if not present, | ||
// the supplied default value. If the float64 conversion fails, returns the default | ||
func WithDefaultFloat64(key string, def float64) float64 { | ||
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. Is there a generics way to do this? 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 don't think so. The float64 func uses
I think they're better as separate funcs :) |
||
val, ok := os.LookupEnv(key) | ||
if !ok { | ||
return def | ||
} | ||
f, err := strconv.ParseFloat(val, 64) | ||
if err != nil { | ||
return def | ||
} | ||
return f | ||
} | ||
|
||
// WithDefaultString returns the string value of the supplied environment variable or, if not present, | ||
// the supplied default value. | ||
func WithDefaultString(key string, def string) string { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,11 @@ limitations under the License. | |
package options | ||
|
||
import ( | ||
"errors" | ||
"flag" | ||
"fmt" | ||
"net/url" | ||
"os" | ||
|
||
"go.uber.org/multierr" | ||
|
||
|
@@ -31,43 +33,58 @@ const ( | |
ResourceName AWSNodeNameConvention = "resource-name" | ||
) | ||
|
||
func MustParse() Options { | ||
opts := Options{} | ||
flag.StringVar(&opts.ClusterName, "cluster-name", env.WithDefaultString("CLUSTER_NAME", ""), "The kubernetes cluster name for resource discovery") | ||
flag.StringVar(&opts.ClusterEndpoint, "cluster-endpoint", env.WithDefaultString("CLUSTER_ENDPOINT", ""), "The external kubernetes cluster endpoint for new nodes to connect with") | ||
flag.StringVar(&opts.KarpenterService, "karpenter-service", env.WithDefaultString("KARPENTER_SERVICE", ""), "The Karpenter Service name for the dynamic webhook certificate") | ||
flag.IntVar(&opts.MetricsPort, "metrics-port", env.WithDefaultInt("METRICS_PORT", 8080), "The port the metric endpoint binds to for operating metrics about the controller itself") | ||
flag.IntVar(&opts.HealthProbePort, "health-probe-port", env.WithDefaultInt("HEALTH_PROBE_PORT", 8081), "The port the health probe endpoint binds to for reporting controller health") | ||
flag.IntVar(&opts.WebhookPort, "port", 8443, "The port the webhook endpoint binds to for validation and mutation of resources") | ||
flag.IntVar(&opts.KubeClientQPS, "kube-client-qps", env.WithDefaultInt("KUBE_CLIENT_QPS", 200), "The smoothed rate of qps to kube-apiserver") | ||
flag.IntVar(&opts.KubeClientBurst, "kube-client-burst", env.WithDefaultInt("KUBE_CLIENT_BURST", 300), "The maximum allowed burst of queries to the kube-apiserver") | ||
flag.StringVar(&opts.AWSNodeNameConvention, "aws-node-name-convention", env.WithDefaultString("AWS_NODE_NAME_CONVENTION", string(IPName)), "The node naming convention used by the AWS cloud provider. DEPRECATION WARNING: this field may be deprecated at any time") | ||
flag.BoolVar(&opts.AWSENILimitedPodDensity, "aws-eni-limited-pod-density", env.WithDefaultBool("AWS_ENI_LIMITED_POD_DENSITY", true), "Indicates whether new nodes should use ENI-based pod density") | ||
flag.StringVar(&opts.AWSDefaultInstanceProfile, "aws-default-instance-profile", env.WithDefaultString("AWS_DEFAULT_INSTANCE_PROFILE", ""), "The default instance profile to use when provisioning nodes in AWS") | ||
flag.BoolVar(&opts.AWSEnablePodENI, "aws-enable-pod-eni", env.WithDefaultBool("AWS_ENABLE_POD_ENI", false), "If true then instances that support pod ENI will report a vpc.amazonaws.com/pod-eni resource") | ||
flag.Parse() | ||
if err := opts.Validate(); err != nil { | ||
panic(err) | ||
} | ||
return opts | ||
} | ||
|
||
// Options for running this binary | ||
type Options struct { | ||
*flag.FlagSet | ||
ClusterName string | ||
ClusterEndpoint string | ||
KarpenterService string | ||
MetricsPort int | ||
HealthProbePort int | ||
WebhookPort int | ||
KubeClientQPS int | ||
KubeClientBurst int | ||
VMMemoryOverhead float64 | ||
AWSNodeNameConvention string | ||
AWSENILimitedPodDensity bool | ||
AWSDefaultInstanceProfile string | ||
AWSEnablePodENI bool | ||
} | ||
|
||
// New creates an Options struct and registers CLI flags and environment variables to fill-in the Options struct fields | ||
func New() Options { | ||
opts := Options{} | ||
f := flag.NewFlagSet("karpenter", flag.ContinueOnError) | ||
opts.FlagSet = f | ||
f.StringVar(&opts.ClusterName, "cluster-name", env.WithDefaultString("CLUSTER_NAME", ""), "The kubernetes cluster name for resource discovery") | ||
f.StringVar(&opts.ClusterEndpoint, "cluster-endpoint", env.WithDefaultString("CLUSTER_ENDPOINT", ""), "The external kubernetes cluster endpoint for new nodes to connect with") | ||
f.IntVar(&opts.MetricsPort, "metrics-port", env.WithDefaultInt("METRICS_PORT", 8080), "The port the metric endpoint binds to for operating metrics about the controller itself") | ||
f.IntVar(&opts.HealthProbePort, "health-probe-port", env.WithDefaultInt("HEALTH_PROBE_PORT", 8081), "The port the health probe endpoint binds to for reporting controller health") | ||
f.IntVar(&opts.KubeClientQPS, "kube-client-qps", env.WithDefaultInt("KUBE_CLIENT_QPS", 200), "The smoothed rate of qps to kube-apiserver") | ||
f.IntVar(&opts.KubeClientBurst, "kube-client-burst", env.WithDefaultInt("KUBE_CLIENT_BURST", 300), "The maximum allowed burst of queries to the kube-apiserver") | ||
f.Float64Var(&opts.VMMemoryOverhead, "vm-memory-overhead", env.WithDefaultFloat64("VM_MEMORY_OVERHEAD", 0.075), "The VM memory overhead as a percent that will be subtracted from the total memory for all instance types") | ||
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 remember you doing some testing on this - did we want to tune 0.075 any further or is that still the best approximation we've got by default? Have we also given up trying to do this per instance type? 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. Testing found I gave up on per instance type, there's too much variability and we could be wrong as new ones are released, the percentage is a conservative, safe value. 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. As a customer, how do I know what value to be setting here? I'd probably see that 0.075 is too high and therefore my nodes aren't as densely packed as they are with just nodegroups + k8s scheduler, but how do I tune this to say 0.072 etc? Is it just guesswork? If I choose a really bad number, the worst that will happen is that Karpenter may launch an incorrect number of nodes to satisfy my pending pods but it should eventually converge. Is my understanding correct? |
||
f.StringVar(&opts.AWSNodeNameConvention, "aws-node-name-convention", env.WithDefaultString("AWS_NODE_NAME_CONVENTION", string(IPName)), "The node naming convention used by the AWS cloud provider. DEPRECATION WARNING: this field may be deprecated at any time") | ||
f.BoolVar(&opts.AWSENILimitedPodDensity, "aws-eni-limited-pod-density", env.WithDefaultBool("AWS_ENI_LIMITED_POD_DENSITY", true), "Indicates whether new nodes should use ENI-based pod density") | ||
f.StringVar(&opts.AWSDefaultInstanceProfile, "aws-default-instance-profile", env.WithDefaultString("AWS_DEFAULT_INSTANCE_PROFILE", ""), "The default instance profile to use when provisioning nodes in AWS") | ||
f.BoolVar(&opts.AWSEnablePodENI, "aws-enable-pod-eni", env.WithDefaultBool("AWS_ENABLE_POD_ENI", false), "If true then instances that support pod ENI will report a vpc.amazonaws.com/pod-eni resource") | ||
return opts | ||
} | ||
|
||
// MustParse reads the user passed flags, environment variables, and default values. | ||
// Options are valided and panics if an error is returned | ||
func (o Options) MustParse() Options { | ||
err := o.Parse(os.Args[1:]) | ||
|
||
if errors.Is(err, flag.ErrHelp) { | ||
os.Exit(0) | ||
} | ||
if err != nil { | ||
panic(err) | ||
} | ||
if err := o.Validate(); err != nil { | ||
panic(err) | ||
} | ||
return o | ||
} | ||
|
||
func (o Options) Validate() (err error) { | ||
err = multierr.Append(err, o.validateEndpoint()) | ||
if o.ClusterName == "" { | ||
|
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.
:O What's this?
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.
just dummy params to allow for flag parsing to take place. This allowed me to plug in other params that could change the generated instance types resources like max-pods and vm overhead easily. Figured I'd leave in how I did it so that if we need to or want to change some of those parameters, it's a simple
os.Setenv(...