static-build: build the real static binary for clh by likebreath · Pull Request #1099 · kata-containers/packaging

likebreath · 2020-07-21T23:02:24Z

This patch reuses the 'dev_cli.sh' from cloud-hypervisor repo to build a
static binary of clh without creating our own docker image nor
installing extra dependencies.

Fixes: #1033

Signed-off-by: Bo Chen chen.bo@intel.com

Note: As we don't have full CI coverage in this repo, I am using a dummy PR from the tests repo (kata-containers/tests#2733) to verify this change is passing all CI jobs related to cloud-hypervisor.

likebreath · 2020-07-21T23:08:38Z

/test-ubuntu

likebreath · 2020-07-21T23:08:46Z

/test-clh

likebreath · 2020-07-21T23:22:22Z

As pointed by @jcvenegas, the clh CI now is broken and is being fixed by two pending PRs (#1098 and kata-containers/runtime#2843). We will need to land those two PRs before we can merge this one.

amshinde · 2020-07-22T00:42:50Z

 git fetch || true
 git checkout "${cloud_hypervisor_version}"
-"${script_dir}/docker-build/build.sh"
+./scripts/dev_cli.sh build --release --libc musl


I dont see the dev_cli.sh file in this repo. Are we adding it in this PR?
Also, prefer to use ${script_dir} instead os ./scripts since this script could be invoked from another directory.

This patch reuses the building script from clh repo: https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/scripts/dev_cli.sh. It relies on the container image created/maintained as a part of the clh project. I think it is better to leverage their efforts, instead of maintaining another docker image to build clh in Kata.

Also, using the hard-coded path to /scripts/dev_cli.sh should be good given the context of this build-static-clh.sh . I assume this build-static-clh.sh will work independent of where it is invoked. Please correct me if I am wrong. Any other thoughts, please also let me know. :)

/cc @sboeuf

This seems reasonable to me. The relative path is within the repo we just pulled -- it's handled by the cd $repo_dir.

We just have to hope that they don't reorganize within their repos structure. If they do, we'd need to redo our build logic anyway. lgtm.

amshinde · 2020-07-22T00:44:11Z

+./scripts/dev_cli.sh build --release --libc musl
 rm -f cloud-hypervisor
-cp ./target/release/cloud-hypervisor .
+cp build/cargo_target/x86_64-unknown-linux-musl/release/cloud-hypervisor .


I see that the docker file is removed below. Are the builds no longer performed inside a container?

Yes, the build is inside a container, but with the cloud-hypervisor source folder mounted. The final build artifact (static binary) will be in that location (build/cargo_target/x86_64-unknown-linux-musl/release/cloud-hypervisor relative to the clh src root). I chose to keep the cp after the build assuming this will make this change transparent to the rest of the CI/building flow.

Can you replace that hard-coded architecture with uname -m?

@jodh-intel Good point. Updated the PR.

likebreath · 2020-07-22T19:08:13Z

/test-clh

likebreath · 2020-07-22T19:11:55Z

/test-clh

likebreath · 2020-07-22T19:13:43Z

/test

Again this is a dummy PR to the test repo, and is used to validate the changes to another PR from the packaging PR related to cloud-hypervisor. The following linked issue is just a place holder. Depends-on: github.com/kata-containers/packaging#1099 Fixes: kata-containers#2546 Signed-off-by: Bo Chen <chen.bo@intel.com>

amshinde · 2020-08-12T22:32:34Z

/test-clh
/test-ubuntu
/AzurePipelines run

Again this is a dummy PR to the test repo, and is used to validate the changes to another PR from the packaging PR related to cloud-hypervisor. The following linked issue is just a place holder. Depends-on: github.com/kata-containers/packaging#1099 Fixes: kata-containers#2546 Signed-off-by: Bo Chen <chen.bo@intel.com>

jcvenegas · 2020-08-13T21:30:39Z

/AzurePipelines run

azure-pipelines · 2020-08-13T21:30:54Z

Azure Pipelines successfully started running 2 pipeline(s).

Again this is a dummy PR to the test repo, and is used to validate the changes to another PR from the packaging PR related to cloud-hypervisor. The following linked issue is just a place holder. Depends-on: github.com/kata-containers/packaging#1099 Fixes: kata-containers#2546 Signed-off-by: Bo Chen <chen.bo@intel.com>

likebreath · 2020-08-14T00:05:58Z

/test-ubuntu

likebreath · 2020-08-14T00:06:17Z

/test-clh

Again this is a dummy PR to the test repo, and is used to validate the changes to another PR from the packaging PR related to cloud-hypervisor. The following linked issue is just a place holder. Depends-on: github.com/kata-containers/packaging#1099 Fixes: kata-containers#2546 Signed-off-by: Bo Chen <chen.bo@intel.com>

likebreath · 2020-08-14T16:44:19Z

/test-clh
/test-ubuntu
/AzurePipelines run

jcvenegas · 2020-08-14T18:12:36Z

/AzurePipelines run

azure-pipelines · 2020-08-14T18:13:02Z

Azure Pipelines successfully started running 2 pipeline(s).

likebreath · 2020-08-14T19:56:21Z

Although the PR itself passed all necessary checks, we are still facing a metrics CI failure as tested by the dummy PR from the test repo (kata-containers/tests#2733). So I marked it as DNM for now.

The metrics CI is failing again on the check over the minimal memory footprint (and only for KSM enabled instances): 11:35:00 time="2020-08-14T18:35:00Z" level=warning msg="Failed Minval ( 269742.458500(minimal mean expected) > 269741.400000(minimal mean on dataset)) for [memory-footprint-ksm]". Full log is here: Log is here: http://jenkins.katacontainers.io/job/kata-metrics-tests-ubuntu-clh-18-04-PR/85/console.

One interesting observation is that the numbers are very close (1 out of 269000). The question is: how do we want to adjust the metrics CI to make sure it is working with static build of cloud-hypervisor? We either can 1) remove the check over the minimal memory footprint, or 2) adjust the reference number from the metrics CI. WDYT? @GabyCT @jcvenegas @amshinde @egernst

egernst · 2020-08-14T20:05:44Z

Once we understand the reason for the increased footprint, it would make sense to update the reference value.

We have more details on why? @likebreath @sameo?

likebreath · 2020-08-14T20:33:57Z

Once we understand the reason for the increased footprint, it would make sense to update the reference value.

We have more details on why? @likebreath @sameo?

@egernst Thanks for the prompt response. My understanding is that the checks is failing not because of increased memory footprint, but the opposite: the actual minimal memory footprint now is smaller than the reference value. This is how I understand the error message: 11:35:00 time="2020-08-14T18:35:00Z" level=warning msg="Failed Minval ( 269742.458500(minimal mean expected) > 269741.400000(minimal mean on dataset)) for [memory-footprint-ksm]. Please correct me if I am wrong.

likebreath · 2020-08-20T21:35:21Z

@kata-containers/packaging Please comment on the above discussions about how we want to proceed with the metrics CI failure, where the current minimum memory footprint is slightly smaller than the reference value.

Also, as we are preparing for next release, do we want to have it as a part of that release?

Thanks.

amshinde · 2020-08-20T22:04:27Z

@likebreath Is the decrease in memory footprint only in case of KSM?
I thought that you were seeing an increase in memory footprint for static binary, which makes sense to me.

In any case, I feel we should remove the check on the min value, we should'nt really trigger a failure when we are seeing lower memory overhead. I would also readjust the min limits.
@egernst @kata-containers/packaging Any thoughts on this?

likebreath · 2020-08-20T22:10:51Z

@likebreath Is the decrease in memory footprint only in case of KSM?

Right.

I thought that you were seeing an increase in memory footprint for static binary, which makes sense to me.

That is also what I thought when we got the initial metrics CI failure with this PR. For now (after several changes from the metrics CI/runtime/packaging), check in minval is the only failure.

In any case, I feel we should remove the check on the min value, we should'nt really trigger a failure when we are seeing lower memory overhead. I would also readjust the min limits.

This makes sense to me. Any pointers/help to adjust this in the CI?

amshinde · 2020-08-20T22:12:52Z

This makes sense to me. Any pointers/help to adjust this in the CI?

@GabyCT would be the best person to help you here.

chavafg · 2020-08-21T02:29:53Z

This is the file that needs to be updated: https://github.com/kata-containers/tests/blob/master/cmd/checkmetrics/ci_slaves/checkmetrics-json-clh-baremetal-kata-metric3.toml

likebreath · 2020-08-24T20:10:57Z

FYI: the pending PR kata-containers/tests#2809 to loos the check on minval, which should resolve the metrics CI failure we have with this PR.

Again this is a dummy PR to the test repo, and is used to validate the changes to another PR from the packaging PR related to cloud-hypervisor. The following linked issue is just a place holder. Depends-on: github.com/kata-containers/packaging#1099 Fixes: kata-containers#2546 Signed-off-by: Bo Chen <chen.bo@intel.com>

likebreath · 2020-08-25T16:08:02Z

Please help re-run the AzurePipelines. Just Rebased on master.
I will run the CLH related CI jobs from the dummy PR kata-containers/tests#2733 to make sure all CI jobs are passing (especially the metrics CI).
/test

This patch reuses the 'dev_cli.sh' from cloud-hypervisor repo to build a static binary of clh without creating our own docker image nor installing extra dependencies. Fixes: kata-containers#1033 Signed-off-by: Bo Chen <chen.bo@intel.com>

Again this is a dummy PR to the test repo, and is used to validate the changes to another PR from the packaging PR related to cloud-hypervisor. The following linked issue is just a place holder. Depends-on: github.com/kata-containers/packaging#1099 Fixes: kata-containers#2546 Signed-off-by: Bo Chen <chen.bo@intel.com>

likebreath · 2020-08-25T19:29:34Z

I see some instability of our clh-docker CI, which mostly can be resolved by restarting the CI. After talking with @jcvenegas, we believe it might be related to the seccomp filters defined in clh.

The static build of cloud-hypervisor is using a different set of syscalls from its dynamic build. The static build of clh is less tested in our kata CI, so we may see more instabilities when using the static build of clh. I am setting up a local environment to manually run the clh-docker CI tests, from which I am hoping to find the missing syscalls from the seccomp list in clh if any.

Meanwhile, for the sake of stability, we may need to keep using the dynamic build of clh in our current beta release of Kata.

Also, I am thinking we may want to disable the seccomp functionality of clh in Kata for now while we are completing the seccomp filter list based on Kata's CI jobs/workload.

/cc @egernst @jcvenegas @amshinde @sameo @sboeuf

Again this is a dummy PR to the test repo, and is used to validate the changes to another PR from the packaging PR related to cloud-hypervisor. The following linked issue is just a place holder. Depends-on: github.com/kata-containers/packaging#1099 Depends-on: github.com/kata-containers/runtime#2899 Fixes: kata-containers#2546 Signed-off-by: Bo Chen <chen.bo@intel.com>

Again this is a dummy PR to the test repo, and is used to validate the changes to another PR from the packaging PR related to cloud-hypervisor. The following linked issue is just a place holder. Depends-on: github.com/kata-containers/packaging#1099 Depends-on: github.com/kata-containers/runtime#2900 Fixes: kata-containers#2546 Signed-off-by: Bo Chen <chen.bo@intel.com>

sboeuf · 2020-08-26T06:51:26Z

@likebreath thanks for the summary about your investigations. I agree disabling seccomp while you try to find out the missing syscalls is acceptable.

likebreath · 2020-08-26T18:14:57Z

@egernst @amshinde Please run the AzurePipeline and I think the PR is ready for being merged.

The seccomp option of clh in kata-runtime is now disabled temperately in kata-runtime, I no longer see the instability of the clh-docker CI (tested in kata-containers/tests#2733). So I removed the DNM from this PR.

jodh-intel · 2020-08-27T08:21:34Z

/AzurePipelines run

azure-pipelines · 2020-08-27T08:21:48Z

Azure Pipelines successfully started running 2 pipeline(s).

jcvenegas approved these changes Jul 21, 2020

View reviewed changes

likebreath mentioned this pull request Jul 21, 2020

Add support for statically building cloud-hypervisor #1089

Closed

amshinde reviewed Jul 22, 2020

View reviewed changes

likebreath force-pushed the fix_1033 branch from 39d8b4b to 86d9bd6 Compare July 22, 2020 19:11

likebreath mentioned this pull request Jul 22, 2020

[DNM] dummy: clh: test PR #1099 from kata-containers/packaging kata-containers/tests#2733

Closed

egernst approved these changes Aug 8, 2020

View reviewed changes

amshinde approved these changes Aug 12, 2020

View reviewed changes

amshinde closed this Aug 12, 2020

amshinde reopened this Aug 12, 2020

likebreath force-pushed the fix_1033 branch from 86d9bd6 to b93949c Compare August 13, 2020 17:13

likebreath force-pushed the fix_1033 branch from b93949c to ece696b Compare August 14, 2020 16:43

likebreath added the do-not-merge PR has problems or depends on another label Aug 14, 2020

likebreath mentioned this pull request Aug 24, 2020

metrics: clh: Failing on decreased memory footprint. kata-containers/tests#2808

Closed

jodh-intel approved these changes Aug 25, 2020

View reviewed changes

likebreath force-pushed the fix_1033 branch from ece696b to 1e3c577 Compare August 25, 2020 16:05

likebreath removed the do-not-merge PR has problems or depends on another label Aug 26, 2020

likebreath merged commit 20a89c7 into kata-containers:master Aug 27, 2020

likebreath added the port-to-2.0 PRs that need to be ported to kata 2.0-dev branch label Aug 27, 2020

Conversation

likebreath commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

likebreath commented Jul 21, 2020

Uh oh!

likebreath commented Jul 21, 2020

Uh oh!

likebreath commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

likebreath commented Jul 22, 2020

Uh oh!

likebreath commented Jul 22, 2020

Uh oh!

likebreath commented Jul 22, 2020

Uh oh!

amshinde commented Aug 12, 2020

Uh oh!

jcvenegas commented Aug 13, 2020

Uh oh!

azure-pipelines Bot commented Aug 13, 2020

Uh oh!

likebreath commented Aug 14, 2020

Uh oh!

likebreath commented Aug 14, 2020

Uh oh!

likebreath commented Aug 14, 2020

Uh oh!

jcvenegas commented Aug 14, 2020

Uh oh!

azure-pipelines Bot commented Aug 14, 2020

Uh oh!

likebreath commented Aug 14, 2020

Uh oh!

egernst commented Aug 14, 2020

Uh oh!

likebreath commented Aug 14, 2020

Uh oh!

likebreath commented Aug 20, 2020

Uh oh!

amshinde commented Aug 20, 2020

Uh oh!

likebreath commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amshinde commented Aug 20, 2020

Uh oh!

chavafg commented Aug 21, 2020

Uh oh!

likebreath commented Aug 24, 2020

Uh oh!

likebreath commented Aug 25, 2020

Uh oh!

likebreath commented Aug 25, 2020

Uh oh!

sboeuf commented Aug 26, 2020

Uh oh!

likebreath commented Aug 26, 2020

Uh oh!

jodh-intel commented Aug 27, 2020

Uh oh!

azure-pipelines Bot commented Aug 27, 2020

Uh oh!

Reviewers

likebreath commented Jul 21, 2020 •

edited

Loading

likebreath commented Aug 20, 2020 •

edited

Loading