[Draft] Add new setting: eviction-hard #1341

gthao313 · 2021-02-23T00:41:10Z

Get some feedbacks from this draft, which can help me in implementing another three settings

Issue number:
#1140

Description of changes:
Add new setting eviction-hard to k8s. User can specify this setting in userdata.

example:
[settings.kubernetes.eviction-hard]
"memory.available" = "10Gi"
"nodefs.inodesFree" = “15%”

Testing done:
Build aws-eks-* images and launched instance.

Test step1:
Go to control container run apiclient -u /settings to check if expected setting is there.
eviction-hard":{"memory.available":"99.999%"}

Test step2:
ssh to admin contianer and look etc/kubernetes/kubelet/config to check if expected config is there.

evictionHard:
  memory.available: 15%

Test step3 - test effected/behavior:
set up eviction threshold to a high value for triggering eviction.

[evictionHard.memory]
"memory.available" = "99.99%" (high percentage to trigger eviction)

a) Go to EKS to check node condition

MemoryPressure	True	kubelet has insufficient memory available

b) In admin contianer, check kubelet log by journalctl -u kubelet

eviction_manager.go:335] eviction manager: attempting to reclaim memory
eviction_manager.go:346] eviction manager: must evict pod(s) to reclaim memory

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

gthao313 · 2021-02-23T00:42:13Z

README.md

+* `settings.kubernetes.eviction-hard`: The hard eviction Signal and Thresholds you set up to reclaim the associated starved resource.
+  Remember to quote keys (since they often contain ".") and to quote all values.
+  * Example user data for setting up eviction hard:
+  ```


I'll fix this indent.

gthao313 · 2021-02-23T00:47:28Z

README.md

+    "nodefs.inodesFree" = "15Mi"
+    "pid.available" = "15%"
+    ```
+

 You can also optionally specify static pods for your node with the following settings.


Question: There are four settings, and wonder if it's better to make four PRs for them (one PR for each setting.) Ben mentioned that we should make them in different commits and same PR. I think maybe it's better to have four PR, because all of them have different testing and implementation requirement. Can I get some suggestions on that? thanks!

If the commits have no inter-dependencies and can stand alone by themselves I don't think we have to have them in the same PR. That being said, if you already have them ready, and they're similar in scope to this commit you have out right now, it'd be nice to have them all in the PR and reviewers can just review commit by commit instead of having to jump through 4 PRs that might end up with similar review comments.

srgothi92

Nice ! 🎉

srgothi92 · 2021-02-23T14:35:28Z

sources/models/src/modeled_types/kubernetes.rs

+            EvictionHardValue::try_from(*err).unwrap_err();
+        }
+    }
+}


nit: new line

srgothi92 · 2021-02-23T14:40:09Z

sources/models/src/modeled_types/mod.rs

@@ -62,6 +62,8 @@ pub mod error {
            field: String,
            source: serde_plain::Error,
        },
+        #[snafu(display("Invalid eviction hard '{}': {}", input, msg))]
+        InvalideEvictionHard { input: String, msg: String },


we can create two different error enums, one for Key and other for value. So, that you don't have to specifically pass a msg string and we can include msg string as part of #[snafu(display to create string you want.
Did you refer it from somewhere, where we are using same enum ? I checked ECSAttributeKey and ECSAttributeValue we are using different error enum there.

Your suggestion could be a way as well. The reason why I make them as one is trying to make simple and clear. and I agree with you that different error enums could be a better way. Btw, I got this idea from KubernetesLableKey and KubernetesLabelValue.

The reason why we have error variants like this is because there might be a lot of special cases we check for and capturing each one as a separate error variant would be too much when all we want to convey is the value is invalid for so-and-so reason. An example of this is InvalidSysctlKey.

If we only return this error in one location I would use #[snafu(display)]. If there's more than one check that check for different things that might return this, then I think having it like this is fine.

srgothi92 · 2021-02-23T14:43:54Z

sources/models/src/modeled_types/kubernetes.rs

+    type Error = error::Error;
+
+    fn try_from(input: &str) -> Result<Self, Self::Error> {
+        let evitionsignal = vec![


we can also use enum here for example: https://github.com/bottlerocket-os/bottlerocket/blob/develop/sources/models/src/modeled_types/ecs.rs#L189
However, I don't know which aligns more to our existing style, I would let other comment on what we should use.

I was trying to use enum to handle this part; however, I think enum could be not that efficient and clear there because too many works on dealing with variables which contains dots like memory.available. I got some ideas that if we can use rename attributes for each enum variant #[serde(rename = "memory.available")] , and I'm not sure if that worths. Appreciate for the comments! :)

Another plus to just having it as an enum is being able to reuse it elsewhere (e.g. in your tests) for matching/validation etc.

I also wonder if we even need any of the traits here for converting to/from strings if EvictionHardKey itself was the enum. We've never tried that before but I don't see a clear reason why we can't just rely on serde's deserialization trait (with the appropriate attributes like you mentioned) for validation.

@gthao313 I think the rename attributes you mentioned above should allow us to use an enum. If so, that's probably the preferred route as other folks mentioned.

srgothi92 · 2021-02-23T14:47:57Z

packages/kubernetes-1.18/kubelet-config

@@ -29,6 +29,8 @@ authorization:
 clusterDomain: {{settings.kubernetes.cluster-domain}}
 clusterDNS:
 - {{settings.kubernetes.cluster-dns-ip}}
+evictionHard:


what would happen if user does not set this setting ? I guess this setting would still exist in kubelet config without any value. Should we wrap setting in if ? for example:

{{~#if settings.kubernetes.evictionHard }} evictionHard: {{join_map ": " "\n " "no-fail-if-missing" settings.kubernetes.eviction-hard}} {{~/if}}

I would let others weigh in, I am also learning in this commit :)

Correct. The eviction-hard setting would still there and would not effect kubelet start. Your example is a good suggestion to me. Thanks 👍

Yes - I think we should wrap this in an if.

zmrow · 2021-02-24T16:33:53Z

README.md

+    "nodefs.inodesFree" = "15Mi"
+    "pid.available" = "15%"
+    ```
+

 You can also optionally specify static pods for your node with the following settings.


zmrow · 2021-02-24T16:35:30Z

packages/kubernetes-1.18/kubelet-config

@@ -29,6 +29,8 @@ authorization:
 clusterDomain: {{settings.kubernetes.cluster-domain}}
 clusterDNS:
 - {{settings.kubernetes.cluster-dns-ip}}
+evictionHard:


Yes - I think we should wrap this in an if.

zmrow · 2021-02-24T16:37:39Z

sources/models/src/modeled_types/kubernetes.rs

+// =^..^=   =^..^=   =^..^=   =^..^=   =^..^=   =^..^=   =^..^=   =^..^=   =^..^=
+
+/// EvictionHardKey represents a string that contains a valid Kubernetes eviction hard
+/// signal. There are few valid eviction hard signals [memory.available], [nodefs.available],


nit: Rather than naming the currently available values, let's just point to the relevant k8s page so if more are added or if they change, we don't need to remember to update this comment.

Available eviction hard signals: <link here>

zmrow · 2021-02-24T16:39:52Z

sources/models/src/modeled_types/kubernetes.rs

+    type Error = error::Error;
+
+    fn try_from(input: &str) -> Result<Self, Self::Error> {
+        let evitionsignal = vec![


@gthao313 I think the rename attributes you mentioned above should allow us to use an enum. If so, that's probably the preferred route as other folks mentioned.

zmrow · 2021-02-24T16:41:24Z

sources/models/src/modeled_types/kubernetes.rs

+// =^..^=   =^..^=   =^..^=   =^..^=   =^..^=   =^..^=   =^..^=   =^..^=   =^..^=
+
+/// EvictionHardValue represents a string that contains a valid Kubernetes eviction threshold quantity
+/// An eviction threshold can be expressed as Gi/Mi or a percentage using the % token


Same down here - let's just point to the docs rather than reiterating them.

zmrow · 2021-02-24T16:42:50Z

sources/models/src/modeled_types/kubernetes.rs

+    fn try_from(input: &str) -> Result<Self, Self::Error> {
+
+        ensure!(
+            input.ends_with("Gi") || input.ends_with("Mi") || input.ends_with("%"),


Let's make sure that these are the only values that are possible (Gi, Mi, %) I took a peek at the doc page, but perhaps I missed the listing of acceptable values.

zmrow · 2021-02-24T16:44:51Z

sources/models/src/modeled_types/mod.rs

@@ -62,6 +62,8 @@ pub mod error {
            field: String,
            source: serde_plain::Error,
        },
+        #[snafu(display("Invalid eviction hard '{}': {}", input, msg))]
+        InvalideEvictionHard { input: String, msg: String },


s/InvalideEvictionHard/InvalidEvictionHard (There's an extra "e" after Invalid)

zmrow · 2021-02-24T16:46:12Z

sources/models/src/modeled_types/kubernetes.rs

+            input.ends_with("Gi") || input.ends_with("Mi") || input.ends_with("%"),
+            error::InvalideEvictionHard {
+                input,
+                msg: format!("must be ends with Gi, Mi, or %"),


"must end with ..."

Add new setting: eviction-hard

749134c

gthao313 requested review from tjkirch, bcressey, zmrow and etungsten February 23, 2021 00:41

gthao313 commented Feb 23, 2021

View reviewed changes

tjkirch marked this pull request as draft February 23, 2021 01:04

srgothi92 reviewed Feb 23, 2021

View reviewed changes

zmrow reviewed Feb 24, 2021

View reviewed changes

gthao313 closed this Feb 24, 2021

gthao313 deleted the new-setting branch February 24, 2021 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Add new setting: eviction-hard #1341

[Draft] Add new setting: eviction-hard #1341

gthao313 commented Feb 23, 2021

gthao313 Feb 23, 2021

gthao313 Feb 23, 2021

etungsten Feb 23, 2021

zmrow Feb 24, 2021

srgothi92 left a comment

srgothi92 Feb 23, 2021

srgothi92 Feb 23, 2021

gthao313 Feb 23, 2021

etungsten Feb 23, 2021 •

edited

Loading

srgothi92 Feb 23, 2021

gthao313 Feb 23, 2021

etungsten Feb 23, 2021 •

edited

Loading

zmrow Feb 24, 2021

srgothi92 Feb 23, 2021

gthao313 Feb 23, 2021

zmrow Feb 24, 2021

zmrow Feb 24, 2021

zmrow Feb 24, 2021

zmrow Feb 24, 2021

zmrow Feb 24, 2021

zmrow Feb 24, 2021

zmrow Feb 24, 2021

zmrow Feb 24, 2021

zmrow Feb 24, 2021

[Draft] Add new setting: eviction-hard #1341

[Draft] Add new setting: eviction-hard #1341

Conversation

gthao313 commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srgothi92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etungsten Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etungsten Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etungsten Feb 23, 2021 •

edited

Loading

etungsten Feb 23, 2021 •

edited

Loading