-
Notifications
You must be signed in to change notification settings - Fork 275
Add support for issuing a container update to gcs #905
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
Add support for issuing a container update to gcs #905
Conversation
54fc9d3 to
570af7f
Compare
anmaxvl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. just one small question
internal/gcs/container.go
Outdated
| // UpdateContainerMemory makes a modify request on the given cow container to modify the memory size | ||
| // Internally, this will modify the container's job object memory limits | ||
| func UpdateContainerMemory(ctx context.Context, c cow.Container, newMemorySizeInBytes uint64) error { | ||
| newMemorySizeInMB := newMemorySizeInBytes / bytesPerMB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we allow this to be 0 or default to some minimum (e.g. 2MB)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I'm not sure what would happen if the memory size requested was 0. I can test and find out though.
| } | ||
|
|
||
| func (gc *GuestConnection) UpdateContainer(ctx context.Context, cid string, resources interface{}) (err error) { | ||
| ctx, span := trace.StartSpan(ctx, "gcs::GuestConnection::UpdateContainer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for new code the plan was to stop with the :: convention in our logs and use periods but my mind is hazy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevpar can you comment on this? I wanted to be consistent with the other guest connection traces
Signed-off-by: Kathryn Baldauf <[email protected]>
570af7f to
6e87c0c
Compare
Related work items: microsoft#173, microsoft#839, microsoft#856, microsoft#877, microsoft#881, microsoft#886, microsoft#887, microsoft#888, microsoft#889, microsoft#890, microsoft#893, microsoft#894, microsoft#896, microsoft#899, microsoft#900, microsoft#902, microsoft#904, microsoft#905, microsoft#906, microsoft#907, microsoft#908, microsoft#910, microsoft#912, microsoft#913, microsoft#914, microsoft#916, microsoft#918, microsoft#923, microsoft#925, microsoft#926, microsoft#928, microsoft#929, microsoft#932, microsoft#933, microsoft#934, microsoft#938, microsoft#939, microsoft#942, microsoft#943, microsoft#945, microsoft#946, microsoft#947, microsoft#949, microsoft#951, microsoft#952, microsoft#954
…ainer Add support for issuing a container update to gcs
This PR is part of the ongoing dynamic resources work. This PR adds new calls to allow hcsshim to issue container update requests to the running GCS.
Signed-off-by: Kathryn Baldauf [email protected]