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.

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

Ignore 0% and 100% eviction thresholds #59681

Merged
merged 1 commit into from Feb 13, 2018

Conversation

@mtaufen
Copy link
Contributor

mtaufen commented Feb 9, 2018

Primarily, this gives a way to explicitly disable eviction, which is
necessary to use omitempty on EvictionHard.
See: #53833 (comment)

As justification for this approach, neither 0% nor 100% make sense as
eviction thresholds; in the "less-than" case, you can't have less than
0% of a resource and 100% perpetually evicts; in the
"greater-than" case (assuming we ever add a resource with this
semantic), the reasoning is the reverse (not more than 100%, 0%
perpetually evicts).

Eviction thresholds set to 0% or 100% are now ignored.
@dashpole
Copy link
Contributor

dashpole left a comment

Not sure about soft evictions

@@ -145,6 +145,10 @@ func parseThresholdStatements(statements map[string]string) ([]evictionapi.Thres
if len(statements) == 0 {
return nil, nil
}
// explicit none means no thresholds
if _, ok := statements[string(evictionapi.SignalNone)]; ok {
return nil, nil

This comment has been minimized.

@dashpole

dashpole Feb 10, 2018

Contributor

I think we want an empty list rather than nil here. We use the resulting list like this: results = append(results, hardThresholds...), where hardThresholds is the list of thresholds returned by this function.

This comment has been minimized.

@mtaufen

mtaufen Feb 10, 2018

Author Contributor

Why should the return value for 'none' be any different than len(statements) == 0?
Appending nil has the same effect as appending an empty slice.

This comment has been minimized.

@dashpole

dashpole Feb 10, 2018

Contributor

Oh, neat. TIL

allErrors = append(allErrors, fmt.Errorf("invalid configuration: EvictionHard (--eviction-hard) %v may not contain additional thresholds when '%s' is specified", evictionapi.SignalNone))
}
}
// we do the same for EvictionSoft, for consistent behavior

This comment has been minimized.

@dashpole

dashpole Feb 10, 2018

Contributor

I think we should either have eviction-soft-grace-period accept a none value as well, or skip soft eviction for now. This is because you need to specify a soft eviction grace period for each soft eviction threshold: https://kubernetes.io/docs/tasks/administer-cluster/out-of-resource/#soft-eviction-thresholds

This comment has been minimized.

@mtaufen

mtaufen Feb 10, 2018

Author Contributor

If you set 'none' in EvictionSoft, softThresholds will be nil, and we won't require a corresponding grace period. It doesn't make sense to me to require 'none' to be specified in two places, when the second place's semantics depend on the first's.

@mtaufen mtaufen force-pushed the mtaufen:kc-empty-eviction-hard branch from 3847cab to a198202 Feb 10, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Feb 10, 2018

@mtaufen mtaufen referenced this pull request Feb 10, 2018

Merged

Graduate kubeletconfig API group to beta #53833

33 of 33 tasks complete
@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented Feb 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 10, 2018

// when this option is specified, no other thresholds should be present
for signal, _ := range kc.EvictionHard {
if len(kc.EvictionHard) > 1 && evictionapi.Signal(signal) == evictionapi.SignalNone {
allErrors = append(allErrors, fmt.Errorf("invalid configuration: EvictionHard (--eviction-hard) %v may not contain additional thresholds when '%s' is specified", evictionapi.SignalNone))

This comment has been minimized.

@dashpole

dashpole Feb 10, 2018

Contributor

I think you forgot an arg to fmt.Errorf. I see %v and %s, but only evictionapi.SignalNone.

This comment has been minimized.

@mtaufen

mtaufen Feb 10, 2018

Author Contributor

done

@mtaufen mtaufen force-pushed the mtaufen:kc-empty-eviction-hard branch from a198202 to b257f99 Feb 10, 2018

@k8s-github-robot k8s-github-robot removed the lgtm label Feb 10, 2018

@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented Feb 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 10, 2018

@mtaufen mtaufen changed the title Allow 'none' signal in eviction thresholds Allow 'none' signal in hard eviction thresholds Feb 10, 2018

@mtaufen mtaufen force-pushed the mtaufen:kc-empty-eviction-hard branch from b257f99 to c3cad96 Feb 10, 2018

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 10, 2018

fixed linter

@k8s-github-robot k8s-github-robot removed the lgtm label Feb 10, 2018

@mtaufen mtaufen added the lgtm label Feb 10, 2018

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 12, 2018

Were we defaulting missing Eviction{Hard,Soft} to something?

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/apis/kubeletconfig/v1alpha1/defaults.go#L179-L186

Is there another way to express "don't evict", like setting the limits to 100% or something?

This is a great idea, I'll ask @dashpole if that'll have the same effect.

@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented Feb 12, 2018

This is a great idea, I'll ask @dashpole if that'll have the same effect.

I think you would want to set it to 0%. In theory they are the same, but in terms of which code paths are run, they are different. For example, if we find a bug in evictions, users may want to disable them entirely. Setting a 0% threshold wouldn't work for this, as it would still run the eviction codepaths. But it would work in most cases. We could even explicitly ignore 0% thresholds.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 12, 2018

We could even explicitly ignore 0% thresholds.

That seems like it might be a good idea... a sensible expression of "don't mess with this" would translate to bypassing the code path

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 12, 2018

We could even explicitly ignore 0% thresholds.

That seems like it might be a good idea... a sensible expression of "don't mess with this" would translate to bypassing the code path

We could do this for now, since all thresholds have an implicit < semantic, but it could be weird if we get a threshold with a > semantic in the future. OTOH neither 0% nor 100% really make sense as eviction thresholds, so maybe we just explicitly ignore both?

@mtaufen mtaufen force-pushed the mtaufen:kc-empty-eviction-hard branch from c3cad96 to 79b9122 Feb 12, 2018

@k8s-github-robot k8s-github-robot removed the lgtm label Feb 12, 2018

@mtaufen mtaufen changed the title Allow 'none' signal in hard eviction thresholds Ignore 0% and 100% eviction thresholds Feb 12, 2018

@mtaufen mtaufen force-pushed the mtaufen:kc-empty-eviction-hard branch from 79b9122 to 39b1778 Feb 12, 2018

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 12, 2018

Updated to ignore 0% and 100%

@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented Feb 12, 2018

/lgtm
This is as intuitive of a zero value as I think we are going to find.

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 12, 2018

@mtaufen mtaufen force-pushed the mtaufen:kc-empty-eviction-hard branch from 39b1778 to a43a96d Feb 12, 2018

@k8s-github-robot k8s-github-robot removed the lgtm label Feb 12, 2018

@mtaufen mtaufen added the lgtm label Feb 12, 2018

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 12, 2018

re-add label after vet fix

Ignore 0% and 100% eviction thresholds
Primarily, this gives a way to explicitly disable eviction, which is
necessary to use omitempty on EvictionHard.
See: #53833 (comment)

As justification for this approach, neither 0% nor 100% make sense as
eviction thresholds; in the "less-than" case, you can't have less than
0% of a resource and 100% perpetually evicts; in the
"greater-than" case (assuming we ever add a resource with this
semantic), the reasoning is the reverse (not more than 100%, 0%
perpetually evicts).

@mtaufen mtaufen force-pushed the mtaufen:kc-empty-eviction-hard branch from a43a96d to 21dbbe1 Feb 12, 2018

@k8s-github-robot k8s-github-robot removed the lgtm label Feb 12, 2018

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 12, 2018

re-add label after verify-bazel fix

@mtaufen mtaufen added the lgtm label Feb 12, 2018

@dchen1107

This comment has been minimized.

Copy link
Member

dchen1107 commented Feb 13, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107, mtaufen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these OWNERS Files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 13, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 13, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 9de5839 into kubernetes:master Feb 13, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation mtaufen authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request Feb 15, 2018

Kubernetes Submit Queue
Merge pull request #53833 from mtaufen/kubeletconfig-to-beta
Automatic merge from submit-queue (batch tested with PRs 59353, 59905, 53833). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Graduate kubeletconfig API group to beta

Regarding kubernetes/enhancements#281, this PR moves the kubeletconfig API group to beta. 

After #53088, the KubeletConfiguration type should not contain any deprecated or experimental fields, and we should not have to remove any more fields from the type before graduating it to beta. 

We need the community to double check for two things, however:
1. Are there any fields currently in the KubeletConfiguration type that you were going to mark deprecated this quarter, but haven't yet?
2. Are there any fields currently in the KubeletConfiguration type that are experimental or alpha, but were not explicitly denoted as such?

Please comment on this PR if you can answer "yes" to either of those two questions. Please cc anyone with a stake in the kubeletconfig API, so we get as much coverage as possible.

/cc @thockin @dchen1107 @Random-Liu @yujuhong @dashpole @tallclair @vishh @abw @freehan @dnardo @bowei @MrHohn @luxas @liggitt @ncdc @derekwaynecarr @mikedanese 

@kubernetes/sig-network-pr-reviews, @kubernetes/sig-node-pr-reviews 

```release-note
action required: The `kubeletconfig` API group has graduated from alpha to beta, and the name has changed to `kubelet.config.k8s.io`. Please use `kubelet.config.k8s.io/v1beta1`, as `kubeletconfig/v1alpha1` is no longer available. 
```

**TODO:**
- [x] Move experimental/non-gated-alpha/soon-to-be-deprecated fields to `KubeletFlags`
  - [x] #53088
  - [x] #54154
  - [x] #54160
  - [x] #55562
  - [x] #55983
  - [x] #57851
- [x] Lift embedded structure out of strings
  - [x] #53025
  - [x] #54643
  - [x] #54823
  - [x] #55254
- [x] Resolve relative paths against the location config files are loaded from
  - [x] #55648 
- [x] Rename to `kubelet.config.k8s.io`
- [x] Comments
  - [x] Make sure existing comments at least read sensibly.
  - [x] Note default values in comments on the versioned struct.
  - [x] Remove any reference to default values in comments on the internal struct.
- [x] Most fields should be `+optional` and `omitempty`. Add where necessary. ~Where omitted, explicitly comment.~ Edit: We should not distinguish between nil and empty, see below items.
- [x] Ensure defaults are specified via `pkg/kubelet/apis/kubelet.config.k8s.io/v1beta1/defaults.go`, not `cmd/kubelet/app/options/options.go`.
  - [x] #57770
- [x] Ensure kubeadm does not persist v1alpha1 KubeletConfiguration objects (or feature-gates this functionality)
- [x] Don't make a distinction between empty and nil, because of #43203.
  - [x] #59515
  - [x] #59681
- [x] Take the opportunity to fix insecure Kubelet defaults @tallclair 
  - [x] #59666
- [x] Remove CAdvisorPort from KubeletConfiguration wrt #56523.
  - [x] #59580
- [x] Hide `ConfigTrialDuration` until we're more sure what to do with it.
   - [x] #59628
- [x] Fix `// default: x` comments after rebasing on recent changes.
@jberkus

This comment has been minimized.

Copy link

jberkus commented Feb 20, 2018

Did I miss something? I'm still not seeing a way to express "don't evict" after this change.

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 20, 2018

@jberkus if you specify an arbitrary 0% threshold, you'll override the default set of thresholds, and the 0% threshold will be ignored. No thresholds == no evictions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.