agent: add support for online memory and cpu separately by cedriccchen · Pull Request #331 · kata-containers/agent

cedriccchen · 2018-08-23T08:56:29Z

agent: add support for online memory and cpu separately.

In func OnlineCPUMem, cpu is always onlined,
which is not expected. So we add "CpuOnly"
field to support separating memory and cpu online.

Fixes #332

Signed-off-by: Clare Chen clare.chenhui@huawei.com
Signed-off-by: Zichang Lin linzichang@huawei.com

cedriccchen · 2018-08-23T09:00:32Z

This PR is related to kata-containers/runtime#624

bergwolf · 2018-08-23T09:17:18Z

grpc.go


 func (a *agentGRPC) onlineCPUMem(req *pb.OnlineCPUMemRequest) error {
-	if req.NbCpus <= 0 {
+	if req.NbCpus < 0 {


Good catch!

jodh-intel · 2018-08-23T09:51:18Z

Hi @clarecch - thanks for raising!

It might be worth changing the commit message a little. Previously -- as you have said -- CPU onlining was always attempted (even when zero cpus were specified). But also, it looks like because of that initial test, previously memory was only ever onlined when CPUs were also onlined? With this change, we online memory "always", not only when the number of cpus is zero I think?

It might even be clearer to change the code to online the memory first - we could then keep the original req.NbCpus test?

lgtm

devimc · 2018-08-23T12:31:15Z

CI is not happy, @clarecch can you fix it?

grpc.go:196:5:warning: unsigned values are never < 0 (SA4003) (staticcheck)

cedriccchen · 2018-08-23T12:39:25Z

@devimc I will fix it as soon as possible.

linzichang · 2018-08-23T13:11:57Z

@jodh-intel @devimc I think there're three solutions:

The modify in this PR. It's very simple. But maybe not so graceful.
Adding member to "req *pb.OnlineCPUMemRequest" to decide online cpu, mem or both.
Separating onlineCPUMem to two function, onlineCPU and onlineMem.

devimc · 2018-08-23T13:21:26Z

@linzichang I like the number 3

Separating onlineCPUMem to two function, onlineCPU and onlineMem.

devimc · 2018-08-23T13:22:18Z

@clarecch what do you think ?

jodh-intel · 2018-08-23T13:23:31Z

Hi @linzichang - I agree and I think for clarity it makes sense to have separate OnlineCPUs() and OnlineMemory() rpc calls.

However, presumably we created the OnlineCPUMem() for efficiency, so that a single gRPC call could handle both? Is that right @sboeuf, @bergwolf, @gnawux?

Aside: on a general point, how about renaming OnlineCPUMem() to OnlineResources()?

cedriccchen · 2018-08-23T13:24:54Z

@devimc I like number 3 too, but it seems to need a lot of modification

jodh-intel · 2018-08-23T13:43:21Z

Yep - I think option 3 is the "cleanest", but let's get some more input from others....

codecov · 2018-08-23T13:55:19Z

Codecov Report

Merging #331 into master will decrease coverage by 0.23%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
- Coverage   44.92%   44.68%   -0.24%     
==========================================
  Files          15       15              
  Lines        2393     2354      -39     
==========================================
- Hits         1075     1052      -23     
+ Misses       1176     1167       -9     
+ Partials      142      135       -7

devimc · 2018-08-23T14:05:44Z

yep, I think hyper guys wrote this protocol
cc @bergwolf @laijs @gnawux

bergwolf · 2018-08-24T00:10:05Z

@devimc @jodh-intel @linzichang @clarecch We put onlinecpu and onlinememory into one call since they are mostly called together, and we save one grpc call when they actually are. Also we did want to only use a small set of grpc APIs to keep it simple.

And if we split/rename the onlinecpumem API, it will break grpc backward compatibility. So I would suggest we go with option 2 and name the new field CpuOnly so that it defaults to false and old client can still work properly.

cedriccchen · 2018-08-24T02:22:11Z

@bergwolf If we want to keep grpc backward compatibility from being broken, I think the option 2 is the only choice. There are two solutions I think:

Add a new field named CpuOnly, use NbCpus to judge online cpu&memory or online memory only. In this case, we cannot keep the orign comparation, which is that if we want to online cpu while NbCpus=0 we should return an error.
Add two new field named CpuOnline and MemOnline so that we can judge online cpu&memory or memory only or cpu only. We can keep the orign comparation at the same time.

bergwolf · 2018-08-24T02:40:51Z

@clarecch, I don't think we need an extra field. The new grpc onlinecpumem handler can:

NbCpus > 0: online CPU
NbCpus == 0 && CpuOnly: return error
!CpuOnly: online memory

And all cases are covered. WDYT?

cedriccchen · 2018-08-24T03:24:26Z

@bergwolf Yes, this solution is quite good, which covers most cases, except one case NbCpus=0 and CpuOnly=false. In this case we don't know NbCpus=0 is an input error or really want hotplug memory only. Is that matters？ ^^

bergwolf · 2018-08-24T03:41:06Z

@clarecch I think we can rely on NbCpus>0 to say that we want to online CPU, and CpuOnly=false to say that we want to online memory, and do not specially care about the cpu/memory only cases. The combination of NbCpus<=0 && CpuOnly == true means we do not want to online cpu nor memory, in which case caller should not call the API in the first place, and we return error for it.

So to answer your question, NbCpus == 0 && CpuOnly == false means we only online memory. Maybe we should rename CpuOnly to SkipMemory to make it non-relevant to CPU ;). I wanted to make online memory the default behavior so I picked CpuOnly but it seems to cause more confusion than I expected.

jodh-intel · 2018-08-24T07:42:15Z

Adding a CpuOnly bool wfm...

diff --git a/protocols/grpc/agent.proto b/protocols/grpc/agent.proto
index b930d94..865fe0f 100644
--- a/protocols/grpc/agent.proto
+++ b/protocols/grpc/agent.proto
@@ -330,11 +330,13 @@ message ListRoutesRequest {
 message OnlineCPUMemRequest {
        // Wait specifies if the caller waits for the agent to online all resources.
        // If true the agent returns once all resources have been connected, otherwise all
        // resources are connected asynchronously and the agent returns immediately.
        bool wait = 1;
 
        // NbCpus specifies the number of CPUs that were added and the agent has to online.
        uint32 nb_cpus = 2;
+
+       bool cpu_only = 3;
 }

... as long as we document in that file every possible scenario (as we have done on this issue) to maximise understanding.

Related: #150.

linzichang · 2018-08-27T01:28:12Z

I have no idea why the ubuntu-ci failed. May someone give a help?

bergwolf · 2018-08-27T01:35:05Z

grpc.go


 func (a *agentGRPC) onlineCPUMem(req *pb.OnlineCPUMemRequest) error {
-	if req.NbCpus <= 0 {
+	if req.NbCpus == 0 && req.CpuOnly {


What about NbCpus < 0? We should fail in such case.

NbCpus is uint32. I think there is no need to consider NbCpus <0.

heh, good point! I thought it was int32 as we had <= there ;)

bergwolf · 2018-08-27T06:26:21Z

CI failed with

not ok 2 Running within memory constraints
# (from function `waitForProcess' in file ../../.ci/lib.sh, line 161,
#  in test file k8s-memory.bats, line 47)
#   `waitForProcess "$wait_time" "$sleep_time" "$pod_status_cmd"' failed

cedriccchen · 2018-08-27T06:33:30Z

@bergwolf I have no idea what happen when Ci run k8s-memory.bats and failed. Could you please help to find out the reason, if convenient?

caoruidong · 2018-08-27T06:46:44Z

This PR kata-containers/runtime#643 also fails on this test. But it just bumps version of golang. Kind of wired. I think there is something wrong with CI.

bergwolf · 2018-08-27T10:58:15Z

LGTM!

bergwolf · 2018-08-27T10:59:55Z

CI green now. It needs one more approval on the protocol change though.

caoruidong · 2018-08-27T11:09:23Z

Hold on. @clarecch Do you generate pb.go file by proto-gen-go tool?

devimc · 2018-08-27T14:05:18Z

good catch @caoruidong! @clarecch please run hack/update-generated-agent-proto.sh to generate the pb.go file

In func OnlineCPUMem, cpu is always onlined, which is not expected. So we add "CpuOnly" field to support separating memory and cpu online. Fixes #332 Signed-off-by: Clare Chen <clare.chenhui@huawei.com> Signed-off-by: Zichang Lin <linzichang@huawei.com>

cedriccchen · 2018-08-28T07:41:42Z

@devimc @caoruidong @bergwolf Commit is updated, and CI is green. PTAL！

bergwolf · 2018-08-28T08:01:09Z

Thanks @clarecch ! Still need one more approval on the protocol change though. ping @kata-containers/agent

caoruidong · 2018-08-28T08:03:28Z

LGTM

jodh-intel · 2018-08-28T09:51:57Z

Thanks @clarecch!

lgtm

sboeuf · 2018-09-04T14:56:23Z

grpc.go

@@ -193,21 +193,25 @@ func updateContainerCpuset(cgroupPath string, newCpuset string, cookies cookie)
 }

 func (a *agentGRPC) onlineCPUMem(req *pb.OnlineCPUMemRequest) error {


@linzichang @bergwolf
For sure this patch works, but why don't we simply define two different functions for CPU and memory ?
This way, no need for specific flags, we would simply call the appropriate function:

onlineCPU

onlineMemory

We did discuss this a bit above (#331 (comment)) and I agree it would be a lot clearer (but potentially slower as multiple calls might be required).

Yes I can see that!
@bergwolf I understand the concern about multiple calls, but I think it's definitely negligible compared to the whole stack.
And about the API compatibility breakage, we better break it now to make it right, as the project is still not completely stable.

bergwolf approved these changes Aug 23, 2018

View reviewed changes

cedriccchen mentioned this pull request Aug 23, 2018

add support for online memory only #332

Closed

caoruidong mentioned this pull request Aug 25, 2018

versions: Bump golang from 1.8.3 to 1.9.7 kata-containers/runtime#643

Merged

cedriccchen changed the title ~~agent: add support for online memory only.~~ agent: add support for online memory and cpu separately. Aug 25, 2018

cedriccchen changed the title ~~agent: add support for online memory and cpu separately.~~ agent: add support for online memory and cpu separately Aug 25, 2018

bergwolf reviewed Aug 27, 2018

View reviewed changes

caoruidong added the do-not-merge label Aug 27, 2018

caoruidong removed the do-not-merge label Aug 28, 2018

jodh-intel merged commit a0136e7 into kata-containers:master Aug 28, 2018

sboeuf reviewed Sep 4, 2018

View reviewed changes

linzichang mentioned this pull request Sep 5, 2018

Shall we separate onlineCPUMem to onlineCPU and onlineMem #357

Closed

		@@ -193,21 +193,25 @@ func updateContainerCpuset(cgroupPath string, newCpuset string, cookies cookie)
		}

		func (a agentGRPC) onlineCPUMem(req pb.OnlineCPUMemRequest) error {

Conversation

cedriccchen commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cedriccchen commented Aug 23, 2018

Uh oh!

bergwolf Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Aug 23, 2018 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devimc commented Aug 23, 2018

Uh oh!

cedriccchen commented Aug 23, 2018

Uh oh!

linzichang commented Aug 23, 2018

Uh oh!

devimc commented Aug 23, 2018

Uh oh!

devimc commented Aug 23, 2018

Uh oh!

jodh-intel commented Aug 23, 2018

Uh oh!

cedriccchen commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jodh-intel commented Aug 23, 2018

Uh oh!

codecov bot commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devimc commented Aug 23, 2018

Uh oh!

bergwolf commented Aug 24, 2018

Uh oh!

cedriccchen commented Aug 24, 2018

Uh oh!

bergwolf commented Aug 24, 2018

Uh oh!

cedriccchen commented Aug 24, 2018

Uh oh!

bergwolf commented Aug 24, 2018

Uh oh!

jodh-intel commented Aug 24, 2018

Uh oh!

linzichang commented Aug 27, 2018

Uh oh!

bergwolf Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

linzichang Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

bergwolf Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

bergwolf commented Aug 27, 2018

Uh oh!

cedriccchen commented Aug 27, 2018

Uh oh!

caoruidong commented Aug 27, 2018

Uh oh!

bergwolf commented Aug 27, 2018 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergwolf commented Aug 27, 2018

Uh oh!

caoruidong commented Aug 27, 2018

Uh oh!

devimc commented Aug 27, 2018

Uh oh!

cedriccchen commented Aug 28, 2018

Uh oh!

bergwolf commented Aug 28, 2018

Uh oh!

caoruidong commented Aug 28, 2018 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cedriccchen commented Aug 23, 2018 •

edited

Loading

jodh-intel commented Aug 23, 2018 •

edited by amshinde

Loading

cedriccchen commented Aug 23, 2018 •

edited

Loading

codecov bot commented Aug 23, 2018 •

edited

Loading

bergwolf commented Aug 27, 2018 •

edited by amshinde

Loading

caoruidong commented Aug 28, 2018 •

edited by amshinde

Loading

jodh-intel commented Aug 28, 2018 •

edited by amshinde

Loading