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

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,16 @@ The following settings are optional and allow you to further configure your clus
* `settings.kubernetes.standalone-mode`: Whether to run the kubelet in standalone mode, without connecting to an API server. Defaults to `false`.
* `settings.kubernetes.authentication-mode`: Which authentication method the kubelet should use to connect to the API server, and for incoming requests. Defaults to `aws` for AWS variants, and `tls` for other variants.
* `settings.kubernetes.bootstrap-token`: The token to use for [TLS bootstrapping](https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet-tls-bootstrapping/). This is only used with the `tls` authentication mode, and is otherwise ignored.
* `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:
```
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this indent.

[settings.kubernetes.eviction-hard]
"memory.available" = "15%"
"nodefs.inodesFree" = "15Mi"
"pid.available" = "15%"
```


You can also optionally specify static pods for your node with the following settings.
Copy link
Member Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to that

Static pods can be particularly useful when running in standalone mode.
Expand Down
2 changes: 2 additions & 0 deletions packages/kubernetes-1.15/kubelet-config
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ authorization:
clusterDomain: {{settings.kubernetes.cluster-domain}}
clusterDNS:
- {{settings.kubernetes.cluster-dns-ip}}
evictionHard:
{{join_map ": " "\n " "no-fail-if-missing" settings.kubernetes.eviction-hard}}
resolvConf: "/etc/resolv.conf"
hairpinMode: hairpin-veth
cgroupDriver: systemd
Expand Down
2 changes: 2 additions & 0 deletions packages/kubernetes-1.16/kubelet-config
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ authorization:
clusterDomain: {{settings.kubernetes.cluster-domain}}
clusterDNS:
- {{settings.kubernetes.cluster-dns-ip}}
evictionHard:
{{join_map ": " "\n " "no-fail-if-missing" settings.kubernetes.eviction-hard}}
resolvConf: "/etc/resolv.conf"
hairpinMode: hairpin-veth
cgroupDriver: systemd
Expand Down
2 changes: 2 additions & 0 deletions packages/kubernetes-1.17/kubelet-config
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ authorization:
clusterDomain: {{settings.kubernetes.cluster-domain}}
clusterDNS:
- {{settings.kubernetes.cluster-dns-ip}}
evictionHard:
{{join_map ": " "\n " "no-fail-if-missing" settings.kubernetes.eviction-hard}}
resolvConf: "/etc/resolv.conf"
hairpinMode: hairpin-veth
cgroupDriver: systemd
Expand Down
2 changes: 2 additions & 0 deletions packages/kubernetes-1.18/kubelet-config
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ authorization:
clusterDomain: {{settings.kubernetes.cluster-domain}}
clusterDNS:
- {{settings.kubernetes.cluster-dns-ip}}
evictionHard:
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{{join_map ": " "\n " "no-fail-if-missing" settings.kubernetes.eviction-hard}}
resolvConf: "/etc/resolv.conf"
hairpinMode: hairpin-veth
cgroupDriver: systemd
Expand Down
2 changes: 2 additions & 0 deletions packages/kubernetes-1.19/kubelet-config
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ authorization:
clusterDomain: {{settings.kubernetes.cluster-domain}}
clusterDNS:
- {{settings.kubernetes.cluster-dns-ip}}
evictionHard:
{{join_map ": " "\n " "no-fail-if-missing" settings.kubernetes.eviction-hard}}
resolvConf: "/etc/resolv.conf"
hairpinMode: hairpin-veth
cgroupDriver: systemd
Expand Down
3 changes: 2 additions & 1 deletion sources/models/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ use crate::modeled_types::{
DNSDomain, ECSAgentLogLevel, ECSAttributeKey, ECSAttributeValue, FriendlyVersion, Identifier,
KubernetesAuthenticationMode, KubernetesBootstrapToken, KubernetesClusterName,
KubernetesLabelKey, KubernetesLabelValue, KubernetesTaintValue,
Lockdown, SingleLineString, SysctlKey, Url, ValidBase64,
Lockdown, SingleLineString, SysctlKey, Url, ValidBase64, EvictionHardKey, EvictionHardValue,
};

// Kubernetes static pod manifest settings
Expand All @@ -127,6 +127,7 @@ struct KubernetesSettings {
authentication_mode: KubernetesAuthenticationMode,
bootstrap_token: KubernetesBootstrapToken,
standalone_mode: bool,
eviction_hard: HashMap<EvictionHardKey, EvictionHardValue>,

// Settings where we generate a value based on the runtime environment. The user can specify a
// value to override the generated one, but typically would not.
Expand Down
117 changes: 117 additions & 0 deletions sources/models/src/modeled_types/kubernetes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,3 +437,120 @@ mod test_kubernetes_bootstrap_token {
}
}
}

// =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^=

/// EvictionHardKey represents a string that contains a valid Kubernetes eviction hard
/// signal. There are few valid eviction hard signals [memory.available], [nodefs.available],
Copy link
Contributor

Choose a reason for hiding this comment

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

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>

/// [imagefs.available], and [nodefs.inodesFree].
/// https://kubernetes.io/docs/tasks/administer-cluster/out-of-resource/


#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct EvictionHardKey {
inner: String,
}

impl TryFrom<&str> for EvictionHardKey {
type Error = error::Error;

fn try_from(input: &str) -> Result<Self, Self::Error> {
let evitionsignal = vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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! :)

Copy link
Contributor

@etungsten etungsten Feb 23, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

"memory.available","nodefs.available",
"nodefs.inodesFree","imagefs.available",
"imagefs.inodesFree","pid.available"
];

ensure!(
evitionsignal.contains(&input),
error::InvalideEvictionHard {
input,
msg: format!("must be one of designated signals"),
}
);

Ok(EvictionHardKey {
inner: input.to_string(),
})
}
}
string_impls_for!(EvictionHardKey, "EvictionHardKey");

#[cfg(test)]
mod test_kubernetes_eviction_hard_key {
use super::EvictionHardKey;
use std::convert::TryFrom;

#[test]
fn good_eviction_hard_key() {
for ok in &[
"memory.available",
"nodefs.available",
"nodefs.inodesFree",
"imagefs.available",
"imagefs.inodesFree",
"pid.available",
] {
EvictionHardKey::try_from(*ok).unwrap();
}
}

#[test]
fn bad_eviction_hard_key() {
for err in &["", "storage.available", ".bad", "bad.", &"a".repeat(64)] {
EvictionHardKey::try_from(*err).unwrap_err();
}
}
}

// =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^=

/// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

/// https://kubernetes.io/docs/tasks/administer-cluster/out-of-resource/


#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct EvictionHardValue {
inner: String,
}

impl TryFrom<&str> for EvictionHardValue {
type Error = error::Error;

fn try_from(input: &str) -> Result<Self, Self::Error> {

ensure!(
input.ends_with("Gi") || input.ends_with("Mi") || input.ends_with("%"),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

error::InvalideEvictionHard {
input,
msg: format!("must be ends with Gi, Mi, or %"),
Copy link
Contributor

Choose a reason for hiding this comment

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

"must end with ..."

}
);

Ok(EvictionHardValue {
inner: input.to_string(),
})
}
}
string_impls_for!(EvictionHardValue, "EvictionHardValue");

#[cfg(test)]
mod test_kubernetes_eviction_hard_value {
use super::EvictionHardValue;
use std::convert::TryFrom;

#[test]
fn good_eviction_hard_value() {
for ok in &["10Gi", "500Mi", "30%"] {
EvictionHardValue::try_from(*ok).unwrap();
}
}

#[test]
fn bad_eviction_hard_value() {
for err in &["", "bad", "100", &"a".repeat(64)] {
EvictionHardValue::try_from(*err).unwrap_err();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: new line

2 changes: 2 additions & 0 deletions sources/models/src/modeled_types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@etungsten etungsten Feb 23, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}

Expand Down