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

better de/serialization for resourcelimits #535

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

pretentious7
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Jan 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plane ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 5:46pm

pub cpu_period: Option<DockerCpuPeriod>,

/// Proportion of period used by container
pub cpu_period_percent: Option<u8>,

/// Total cpu time allocated to container
#[serde_as(as = "Option<serde_with::DurationSeconds<u64>>")]
Copy link
Member

Choose a reason for hiding this comment

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

it feels weird having two time units on the JSON representation side. Thoughts on using a floating-point seconds representation for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, this is how docker takes it, that's how we'd justified it last time.

Not fp, too much stuff close to 0. but we could use nanos or micros consistently, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

What's a typical value for DockerCpuPeriod? I thought it would be millis at least, but maybe I misunderstand it.

I am ok straying from docker's way of doing things here, the docker API has a lot of vestigial stuff that I'm happy to paper over where we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100 milliseconds, but it reports in microseconds (https://docs.kernel.org/admin-guide/cgroup-v2.html#cgroup-v2-cpu)

you generally only go under that when you've got like super short lived loops and spinlocks and such.

Copy link
Member

@paulgb paulgb left a comment

Choose a reason for hiding this comment

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

thanks for the clarifications, lgtm

@pretentious7 pretentious7 merged commit 3292e9a into main Jan 5, 2024
5 checks passed
@pretentious7 pretentious7 deleted the abhi/better-serde-resource-limits branch January 5, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants