ppc64le: Fix hotplug issue by nitkon · Pull Request #1186 · kata-containers/runtime

nitkon · 2019-01-28T12:36:06Z

ppc64le qemu does not need threadID and
socketID parameters when hotplugging.

Fixes: #1155

Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

nitkon · 2019-01-28T12:54:22Z

/test

grahamwhaley

lgtm

jodh-intel · 2019-01-28T13:31:07Z

 		coreID := fmt.Sprintf("%d", hc.Properties.Core)
 		threadID := fmt.Sprintf("%d", hc.Properties.Thread)
+
+		// If CPU type pseries, we do not set socketID and threadID


Nit: I'd say "IBM pSeries" as wikipedia tells me there are atleast two other hardware platforms with this name so it doesn't hurt to be specific ;)

@jodh-intel : Fixed.

jodh-intel · 2019-01-28T13:31:29Z

lgtm

But let's get feedback from @markdryan on his chosen approach...

markdryan · 2019-01-28T13:38:33Z

I'm happy with the govmm patch and will merge as soon as the travis builds pass. Note you should probably revendor govmm in this PR, otherwise the PR isn't actually going to fix #1155

nitkon · 2019-01-28T17:35:46Z

Updated my PR.
/retest

devimc · 2019-01-28T17:38:38Z

    "github.com/containernetworking/plugins/pkg/ns",
    "github.com/dlespiau/covertool/pkg/cover",
    "github.com/docker/go-units",
-    "github.com/firecracker-microvm/firecracker-go-sdk",


uhmm I wonder why this was removed

cc @mcastelino

@nitkon did this get removed automatically. Is it possible the vendoring tool is not picking up the firecracker dependency properly.

We do use the swagger generated files from the firecracker-go-sdk

Ok. I think I have an explanation for this but still need to test this PR.

In fc.go we only use the generated files

"github.com/firecracker-microvm/firecracker-go-sdk/client" models "github.com/firecracker-microvm/firecracker-go-sdk/client/models" ops "github.com/firecracker-microvm/firecracker-go-sdk/client/operations"

The files that are getting deleted are under github.com/firecracker-microvm/firecracker-go-sdk, which I think are not used by fc.go or its dependencies. (I will verify).

Somehow the vendoring tool did not do the right thing when I vendored the code and is now doing the right set of files.

Let me retest and confirm.

Sure @mcastelino

@nitkon verified. So LGTM

devimc · 2019-01-28T17:40:16Z

  packages = ["qemu"]
  pruneopts = "NUT"
-  revision = "737f03de595e216116264cc74a58e5f2a1df789a"
+  revision = "78d079db6d1f3e32e3ed7578a54baa6257b058a7"


missing short log in commit message

@devimc : Done.

ppc64le qemu does not need threadID and socketID parameters when hotplugging. Fixes: kata-containers#1155 Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

Shortlog: 78d079d Merge pull request kata-containers#84 from nitkon/master 4692f6b qmp: Conditionally pass threadID and socketID when CPU device add b9c8f76 Merge pull request kata-containers#85 from markdryan/fix-travis 1f51b43 Update the versions of Go used to build GoVMM ad310f9 Fix staticcheck S1023 932fdc7 Fix staticcheck S1023 cb2ce93 Fix staticcheck S1008 f0172cd Fix staticcheck (S1002) 5f2e630 Fix staticcheck (S1025) 4beea51 Fix staticcheck (ST1005) errors Fixes: kata-containers#1155 Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

nitkon · 2019-01-28T18:14:28Z

/retest

nitkon · 2019-01-29T06:25:10Z

Jenkins metrics is failing ...

00:27:32 Report Summary:
00:27:32 +-----+----------------------+-------+--------+--------+-------+--------+--------+------+------+-----+
00:27:32 | P/F |         NAME         |  FLR  |  MEAN  |  CEIL  |  GAP  |  MIN   |  MAX   | RNG  | COV  | ITS |
00:27:32 +-----+----------------------+-------+--------+--------+-------+--------+--------+------+------+-----+
00:27:32 | P   | boot-times           | 93.0% | 102.5% | 107.0% | 14.0% | 101.6% | 105.1% | 3.5% | 0.8% |  20 |
00:27:32 | *F* | memory-footprint     | 85.0% | 117.9% | 115.0% | 30.0% | 117.9% | 117.9% | 0.0% | 0.0% |   1 |
00:27:32 | P   | memory-footprint-ksm | 95.0% | 98.1%  | 105.0% | 10.0% | 98.1%  | 98.1%  | 0.0% | 0.0% |   1 |
00:27:32 +-----+----------------------+-------+--------+--------+-------+--------+--------+------+------+-----+
00:27:32 Fails: 1, Passes 2
00:27:32 Failed
00:27:32 checkmetrics FAILED (1)

cc @grahamwhaley

ARM CI is failing due to permission denied error.

jodh-intel · 2019-01-29T09:38:32Z

Adding dnm label as we need to get input from @mcastelino about the firecracker files being deleted here. They were added on #1044 fwics.

grahamwhaley · 2019-01-29T10:12:40Z

Metrics CI we are aware of - I'm working on why the values have 'shifted'. I'll adjust the bounds soon so we stop failing (but we still need to understand why things moved). See: kata-containers/ci#102
ARM CI permission error we hope we've just fixed. See: kata-containers/ci#30 (comment)

nitkon · 2019-01-30T10:51:48Z

@grahamwhaley @Pennyzct : ARM CI still fails ...

grahamwhaley · 2019-01-30T10:57:41Z

ARM CI looks like it may be failing for a different reason?

09:55:00 cli/kata-check_arm64.go:129:9:warning: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (golint)
09:55:02 virtcontainers/qemu_arm64_test.go:43:23:warning: unused variable or constant undeclared name: virt (varcheck)
09:55:02 virtcontainers/qemu_arm64_test.go:50:11:warning: unused variable or constant undeclared name: defaultMemSlots (varcheck)
09:55:02 virtcontainers/qemu_arm64_test.go:54:40:warning: unused variable or constant too few arguments in call to arm64.memoryTopology (varcheck)
09:55:02 virtcontainers/qemu_arm64_test.go:29:23:warning: unused variable or constant undeclared name: virt (varcheck)
09:55:02 cli/kata-check_test.go:141:113:warning: unused variable or constant undeclared name: TestDataa (varcheck)
09:55:03 virtcontainers/qemu_arm64_test.go:43:23:warning: unused struct field undeclared name: virt (structcheck)
09:55:03 virtcontainers/qemu_arm64_test.go:50:11:warning: unused struct field undeclared name: defaultMemSlots (structcheck)
09:55:03 virtcontainers/qemu_arm64_test.go:54:40:warning: unused struct field too few arguments in call to arm64.memoryTopology (structcheck)
09:55:03 virtcontainers/qemu_arm64_test.go:29:23:warning: unused struct field undeclared name: virt (structcheck)
09:55:03 cli/kata-check_test.go:141:113:warning: unused struct field undeclared name: TestDataa (structcheck)
09:55:03 cli/kata-check_test.go:141:113:warning: undeclared name: TestDataa (unused)
09:55:04 cli/kata-check_test.go:141:113:warning: undeclared name: TestDataa (staticcheck)
09:55:05 Build step 'Execute shell' marked build as failure

grahamwhaley · 2019-01-30T11:41:37Z

I've added the ARM CI breaking to the CI status page

mcastelino · 2019-01-30T19:49:40Z

LGTM from a firecracker point of view. It must have been a vendoring issue previously.

devimc · 2019-01-30T19:57:59Z

LGTM

devimc · 2019-01-30T19:58:13Z

@nitkon can we merge this?

nitkon · 2019-01-31T00:45:49Z

@devimc: Sure.

caoruidong

LGTM

Pennyzct · 2019-01-31T01:47:32Z

@grahamwhaley @nitkon Sorry for the keeping failing, there exited some bugs on unit tests under runtime/ repository, and I will fire a PR and issue to fix it asap. During the time, In case ARM CI blocks other PRs ,we could bring it down temporarily.

jodh-intel · 2019-01-31T09:26:21Z

I'm going to merge this as the change is ppc64le-specific but we also have CI assurances it works for atleast one other arch...

grahamwhaley · 2019-01-31T10:27:28Z

Thanks @Pennyzct - let's keep the ARM CI active for the moment and see if we can fix the errors. The ARM CI is listed as failing on the CI Status page, so folks can check its status etc.

Pennyzct · 2019-02-01T09:59:04Z

Hi~ @grahamwhaley thanks for updating the ARM CI status. I have already fired a PR to fix related issues. ;).

grahamwhaley approved these changes Jan 28, 2019

View reviewed changes

jodh-intel reviewed Jan 28, 2019

View reviewed changes

devimc reviewed Jan 28, 2019

View reviewed changes

nitkon force-pushed the Hotplug branch from 336cada to ca22f73 Compare January 28, 2019 18:10

nitkon added 2 commits January 28, 2019 23:42

ppc64le: Fix vCPU hotplug issue

b0986a5

ppc64le qemu does not need threadID and socketID parameters when hotplugging. Fixes: kata-containers#1155 Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

nitkon force-pushed the Hotplug branch from ca22f73 to b1c6667 Compare January 28, 2019 18:13

jodh-intel added the do-not-merge label Jan 29, 2019

devimc removed the do-not-merge label Jan 30, 2019

caoruidong approved these changes Jan 31, 2019

View reviewed changes

jodh-intel merged commit 530360d into kata-containers:master Jan 31, 2019

egernst added the stable-candidate label Feb 12, 2019

egernst mentioned this pull request Feb 26, 2019

Stable 1.5.1 rc #1281

Merged

Conversation

nitkon commented Jan 28, 2019

Uh oh!

nitkon commented Jan 28, 2019

Uh oh!

grahamwhaley left a comment

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!

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

Uh oh!

Uh oh!

markdryan commented Jan 28, 2019

Uh oh!

nitkon commented Jan 28, 2019

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!

nitkon commented Jan 28, 2019

Uh oh!

nitkon commented Jan 29, 2019

Uh oh!

jodh-intel commented Jan 29, 2019

Uh oh!

grahamwhaley commented Jan 29, 2019

Uh oh!

nitkon commented Jan 30, 2019

Uh oh!

grahamwhaley commented Jan 30, 2019

Uh oh!

grahamwhaley commented Jan 30, 2019

Uh oh!

mcastelino commented Jan 30, 2019 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devimc commented Jan 30, 2019 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devimc commented Jan 30, 2019

Uh oh!

nitkon commented Jan 31, 2019

Uh oh!

caoruidong left a comment

Choose a reason for hiding this comment

Uh oh!

Pennyzct commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jodh-intel commented Jan 31, 2019

Uh oh!

grahamwhaley commented Jan 31, 2019

Uh oh!

Pennyzct commented Feb 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jodh-intel commented Jan 28, 2019 •

edited by amshinde

Loading

mcastelino commented Jan 30, 2019 •

edited by amshinde

Loading

devimc commented Jan 30, 2019 •

edited by amshinde

Loading

Pennyzct commented Jan 31, 2019 •

edited

Loading