Skip to content

Conversation

@katiewasnothere
Copy link

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]

@katiewasnothere katiewasnothere requested a review from a team as a code owner December 10, 2020 01:36
Copy link
Contributor

@anmaxvl anmaxvl left a 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

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

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

Copy link
Author

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")
Copy link
Contributor

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.

Copy link
Author

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

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.

3 participants