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

Jump to bottom

Make kubelet topology-manager-policy configurable #8833

Merged
merged 3 commits into from
Apr 5, 2020
Merged

Make kubelet topology-manager-policy configurable #8833

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf42d2b
Make topologymanagerpolicy configurable
Apr 2, 2020
cb8f3e5
Return errors early if kubernetesVersion cannot be determined
Apr 5, 2020
4000988
Apply suggestions from code review
olemarkus Apr 5, 2020
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
8 changes: 8 additions & 0 deletions k8s/crds/kops.k8s.io_clusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2069,6 +2069,10 @@ spec:
tlsPrivateKeyFile:
description: 'TODO: Remove unused TLSPrivateKeyFile'
type: string
topologyManagerPolicy:
description: TopologyManagerPolicy determines the allocation policy
for the topology manager
type: string
volumePluginDirectory:
description: The full path of the directory in which to search for
additional third party volume plugins (this path must be writeable,
Expand Down Expand Up @@ -2427,6 +2431,10 @@ spec:
tlsPrivateKeyFile:
description: 'TODO: Remove unused TLSPrivateKeyFile'
type: string
topologyManagerPolicy:
description: TopologyManagerPolicy determines the allocation policy
for the topology manager
type: string
volumePluginDirectory:
description: The full path of the directory in which to search for
additional third party volume plugins (this path must be writeable,
Expand Down
4 changes: 4 additions & 0 deletions k8s/crds/kops.k8s.io_instancegroups.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,10 @@ spec:
tlsPrivateKeyFile:
description: 'TODO: Remove unused TLSPrivateKeyFile'
type: string
topologyManagerPolicy:
description: TopologyManagerPolicy determines the allocation policy
for the topology manager
type: string
volumePluginDirectory:
description: The full path of the directory in which to search for
additional third party volume plugins (this path must be writeable,
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/kops/componentconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ type KubeletConfigSpec struct {
RegistryPullQPS *int32 `json:"registryPullQPS,omitempty" flag:"registry-qps"`
//RegistryBurst Maximum size of a bursty pulls, temporarily allows pulls to burst to this number, while still not exceeding registry-qps. Only used if --registry-qps > 0 (default 10)
RegistryBurst *int32 `json:"registryBurst,omitempty" flag:"registry-burst"`
//TopologyManagerPolicy determines the allocation policy for the topology manager
olemarkus marked this conversation as resolved.
Show resolved Hide resolved
TopologyManagerPolicy string `json:"topologyManagerPolicy,omitempty" flag:"topology-manager-policy"`

// rotateCertificates enables client certificate rotation.
RotateCertificates *bool `json:"rotateCertificates,omitempty" flag:"rotate-certificates"`
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/kops/v1alpha2/componentconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ type KubeletConfigSpec struct {
RegistryPullQPS *int32 `json:"registryPullQPS,omitempty" flag:"registry-qps"`
//RegistryBurst Maximum size of a bursty pulls, temporarily allows pulls to burst to this number, while still not exceeding registry-qps. Only used if --registry-qps > 0 (default 10)
RegistryBurst *int32 `json:"registryBurst,omitempty" flag:"registry-burst"`
//TopologyManagerPolicy determines the allocation policy for the topology manager
olemarkus marked this conversation as resolved.
Show resolved Hide resolved
TopologyManagerPolicy string `json:"topologyManagerPolicy,omitempty" flag:"topology-manager-policy"`

// rotateCertificates enables client certificate rotation.
RotateCertificates *bool `json:"rotateCertificates,omitempty" flag:"rotate-certificates"`
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/kops/v1alpha2/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

124 changes: 49 additions & 75 deletions pkg/apis/kops/validation/legacy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,15 @@ func ValidateCluster(c *kops.Cluster, strict bool) field.ErrorList {
fieldSpec := field.NewPath("spec")
allErrs := field.ErrorList{}

// kubernetesRelease is the version with only major & minor fields
// We initialize to an arbitrary value, preferably in the supported range,
// in case the value in c.Spec.KubernetesVersion is blank or unparseable.
kubernetesRelease := semver.Version{Major: 1, Minor: 15}

// KubernetesVersion
// This is one case we return error because a large part of the rest of the validation logic depend on a valid kubernetes version.
olemarkus marked this conversation as resolved.
Show resolved Hide resolved

if c.Spec.KubernetesVersion == "" {
allErrs = append(allErrs, field.Required(fieldSpec.Child("kubernetesVersion"), ""))
} else {
sv, err := util.ParseKubernetesVersion(c.Spec.KubernetesVersion)
if err != nil {
allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubernetesVersion"), c.Spec.KubernetesVersion, "unable to determine kubernetes version"))
} else {
kubernetesRelease = semver.Version{Major: sv.Major, Minor: sv.Minor}
}
return allErrs
} else if _, err := util.ParseKubernetesVersion(c.Spec.KubernetesVersion); err != nil {
allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubernetesVersion"), c.Spec.KubernetesVersion, "unable to determine kubernetes version"))
return allErrs
}

if c.ObjectMeta.Name == "" {
Expand Down Expand Up @@ -440,7 +434,7 @@ func ValidateCluster(c *kops.Cluster, strict bool) field.ErrorList {

// KubeAPIServer
if c.Spec.KubeAPIServer != nil {
if kubernetesRelease.GTE(semver.MustParse("1.10.0")) {
if c.IsKubernetesGTE("1.10") {
if len(c.Spec.KubeAPIServer.AdmissionControl) > 0 {
if len(c.Spec.KubeAPIServer.DisableAdmissionPlugins) > 0 {
allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("kubeAPIServer", "disableAdmissionPlugins"),
Expand All @@ -451,68 +445,8 @@ func ValidateCluster(c *kops.Cluster, strict bool) field.ErrorList {
}

// Kubelet
if c.Spec.Kubelet != nil {
kubeletPath := fieldSpec.Child("kubelet")

{
// Flag removed in 1.6
if c.Spec.Kubelet.APIServers != "" {
allErrs = append(allErrs, field.Invalid(
kubeletPath.Child("apiServers"),
c.Spec.Kubelet.APIServers,
"api-servers flag was removed in 1.6"))
}
}

if kubernetesRelease.GTE(semver.MustParse("1.10.0")) {
// Flag removed in 1.10
if c.Spec.Kubelet.RequireKubeconfig != nil {
allErrs = append(allErrs, field.Invalid(
kubeletPath.Child("requireKubeconfig"),
*c.Spec.Kubelet.RequireKubeconfig,
"require-kubeconfig flag was removed in 1.10. (Please be sure you are not using a cluster config from `kops get cluster --full`)"))
}
}

if c.Spec.Kubelet.BootstrapKubeconfig != "" {
if c.Spec.KubeAPIServer == nil {
allErrs = append(allErrs, field.Required(fieldSpec.Child("kubeAPIServer"), "bootstrap token require the NodeRestriction admissions controller"))
}
}

if c.Spec.Kubelet.APIServers != "" && !isValidAPIServersURL(c.Spec.Kubelet.APIServers) {
allErrs = append(allErrs, field.Invalid(kubeletPath.Child("apiServers"), c.Spec.Kubelet.APIServers, "Not a valid apiServer URL"))
}
}

// MasterKubelet
if c.Spec.MasterKubelet != nil {
masterKubeletPath := fieldSpec.Child("masterKubelet")

{
// Flag removed in 1.6
if c.Spec.MasterKubelet.APIServers != "" {
allErrs = append(allErrs, field.Invalid(
masterKubeletPath.Child("apiServers"),
c.Spec.MasterKubelet.APIServers,
"api-servers flag was removed in 1.6"))
}
}

if kubernetesRelease.GTE(semver.MustParse("1.10.0")) {
// Flag removed in 1.10
if c.Spec.MasterKubelet.RequireKubeconfig != nil {
allErrs = append(allErrs, field.Invalid(
masterKubeletPath.Child("requireKubeconfig"),
*c.Spec.MasterKubelet.RequireKubeconfig,
"require-kubeconfig flag was removed in 1.10. (Please be sure you are not using a cluster config from `kops get cluster --full`)"))
}
}

if c.Spec.MasterKubelet.APIServers != "" && !isValidAPIServersURL(c.Spec.MasterKubelet.APIServers) {
allErrs = append(allErrs, field.Invalid(masterKubeletPath.Child("apiServers"), c.Spec.MasterKubelet.APIServers, "Not a valid apiServers URL"))
}
}
allErrs = append(allErrs, validateKubelet(c.Spec.Kubelet, c, fieldSpec.Child("kubelet"))...)
allErrs = append(allErrs, validateKubelet(c.Spec.MasterKubelet, c, fieldSpec.Child("masterKubelet"))...)

// Topology support
if c.Spec.Topology != nil {
Expand Down Expand Up @@ -731,3 +665,43 @@ func DeepValidate(c *kops.Cluster, groups []*kops.InstanceGroup, strict bool) er

return nil
}

func validateKubelet(k *kops.KubeletConfigSpec, c *kops.Cluster, kubeletPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if k != nil {

{
// Flag removed in 1.6
if k.APIServers != "" {
allErrs = append(allErrs, field.Forbidden(
kubeletPath.Child("apiServers"),
"api-servers flag was removed in 1.6"))
}
}

if c.IsKubernetesGTE("1.10") {
Copy link
Member

Choose a reason for hiding this comment

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

This will panic if the version in the cluster spec is unparseable. Not something that should happen in the validation code. It should keep the old logic of checking kubernetesRelease.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I think that getting this far with an unparseable kubernetes version is undefined behaviour though. While I like outputting all errors (or as many as possible) in one go, I think an unparseable kubernetes version should just return the error immediately.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't agree it counts as "undefined behavior". It is, admittedly, a hard thing to unit test for. I could see us deciding that an unparseable Kubernetes version is one error we don't try to combine with others.

Copy link
Member

Choose a reason for hiding this comment

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

Given how much validation logic depends on a parseable Kubernetes version, I would be in favor of returning a single error early if it isn't parseable.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I read the code correctly, the old code just continued on with 1.15 as the kubernetes version. Changed this to returning early now.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose another approach would be to DeepCopy the Cluster and put 1.15 in the copy's KubernetesVersion.

// Flag removed in 1.10
if k.RequireKubeconfig != nil {
allErrs = append(allErrs, field.Forbidden(
kubeletPath.Child("requireKubeconfig"),
"require-kubeconfig flag was removed in 1.10. (Please be sure you are not using a cluster config from `kops get cluster --full`)"))
}
}

if k.BootstrapKubeconfig != "" {
rifelpet marked this conversation as resolved.
Show resolved Hide resolved
if c.Spec.KubeAPIServer == nil {
allErrs = append(allErrs, field.Required(kubeletPath.Root().Child("spec").Child("kubeAPIServer"), "bootstrap token require the NodeRestriction admissions controller"))
}
}

if k.TopologyManagerPolicy != "" {
allErrs = append(allErrs, IsValidValue(kubeletPath.Child("topologyManagerPolicy"), &k.TopologyManagerPolicy, []string{"none", "best-effort", "restricted", "single-numa-node"})...)
if !c.IsKubernetesGTE("1.18") {
allErrs = append(allErrs, field.Forbidden(kubeletPath.Child("topologyManagerPolicy"), "topologyManagerPolicy requires at least Kubernetes 1.18"))
}
}

}
return allErrs
}