Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Enable Kata container on ppc64le arch #286

Merged
merged 4 commits into from
Jun 1, 2018
Merged

Enable Kata container on ppc64le arch #286

merged 4 commits into from
Jun 1, 2018

Conversation

nitkon
Copy link
Contributor

@nitkon nitkon commented May 7, 2018

This patch enables kata container using
initrd approach on ppc64le architecture.

Fixes #302

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

@nitkon
Copy link
Contributor Author

nitkon commented May 7, 2018

For testing purpose, the kernel for running kata on ppc64le using the initrd approach can be taken from: https://github.com/nitkon/kata/blob/master/vmlinux

@devimc
Copy link

devimc commented May 7, 2018

@nitkon please file and issue and add fixes #XXX in the commit message, where XXX is the number of the issue

@@ -0,0 +1,21 @@
# Copyright (c) 2018 Intel Corporation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Intel Corporation/IBM/ ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use next style for all licence headers

// Copyright (c) 2018 XXXXXXXXXXXXXXXXX
//
// SPDX-License-Identifier: Apache-2.0
//

@@ -0,0 +1,110 @@
// Copyright (c) 2018 Intel Corporation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Intel Corporation/IBM/ ?

@@ -0,0 +1,199 @@
//
// Copyright (c) 2018 Intel Corporation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Intel Corporation/IBM/ ?

MACHINETYPE := pseries
KERNELPARAMS :=
MACHINEACCELERATORS :=
KERNELPATH := /usr/share/kata-containers/vmlinux.container
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line really required? If so, please can you add a comment stating that this architecture must use an uncompressed kernel.

A better approach might be to define a new variable so that you could say something like:

KERNEL_NAME := vmlinux.container

... or even:

KERNEL_TYPE := uncompressed

ifeq (uncompressed,$(KERNEL_TYPE))
    KERNEL_NAME = vmlinux.container
else
    KERNEL_NAME = vmlinuz.container
endif

// hostIsVMContainerCapable checks to see if the host is theoretically capable
// of creating a VM container.

func hostIsVMContainerCapable(details vmContainerCapableDetails) error {
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 this change will break the ARM64 build. Maybe you could leave this function in kata-check.go but rename it to something like genericHostIsVMContainerCapable() and then call it from kata-check_amd64.go and (a new) kata-check_arm64.go?

This also raises a larger issue: our CI currently only tests x86_64. Do you have any thoughts on how we could add PPC64 to our CI to ensure this code doesn't break at some point in the future? If you could handle the ongoing testing of PPC64 that would be great.

/cc @egernst, @bergwolf, @chavafg.

@@ -458,6 +458,10 @@ func RunningOnVMM(cpuInfoPath string) (bool, error) {
return false, nil
}

if runtime.GOARCH == "ppc64le" {
virtLog.Debugf("Unable to know if the system is running inside a VM")
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 this might be better as an info or warning message. Note that you don't need to use the "f" version of the API as there are no additional args (so virtLog.Info() or virtLog.Warn()).

@@ -127,6 +127,9 @@ const (

// QemuVirt is the QEMU virt machine type for aarch64
QemuVirt = "virt"

// QemuPpc64le is a QEMU virt machine type for for ppc64le
QemuPpc64le = "pseries"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This camel case looks a bit odd to my eyes. If the golang compiler agrees could you make this QemuPPC64le (same comment to some of the code below)?


// kvmIsUsable determines if it will be possible to create a full virtual machine
// by creating a minimal VM and then deleting it.
func kvmIsUsable() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated function. Please can you move the version in cli/kata-check_amd64.go into cli/kata-check.go and maybe prefix with generic.

}

func (q *qemuPpc64le) cpuModel() string {
//fmt.Println("HELLLLO")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this line please? 😄

return cpuModel
}

func (q *qemuPpc64le) memoryTopology(memoryMb, hostMemoryMb uint64) govmmQemu.Memory {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost identical to the same function name in virtcontainers/qemu_amd64.go so this could be moved to virtcontainers/qemu.go maybe?


memory := govmmQemu.Memory{
Size: mem,
//Slots: defaultMemSlots,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Almost identical because of these two lines. Please remove the commented out code if these values should not be set.

return nil, err
}

//govmmQemu.SCSIController{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out line.

// See the License for the specific language governing permissions and
// limitations under the License.
//

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @markdryan - any comments on the qemu code here?

@jodh-intel
Copy link
Contributor

Thanks for raising @nitkon! There are a few issues, but most are easily resolved I think.

// hostIsVMContainerCapable checks to see if the host is theoretically capable
// of creating a VM container.
func hostIsVMContainerCapable(details vmContainerCapableDetails) error {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add comments here explaining why this is nil.

var caps capabilities

// Only pc machine type supports hotplugging drives
if q.machineType == QemuPpc64le {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment and the implementation are not in sync. Could you change the comment if the pseries machine type supports this capability.

}

devices = append(devices,
govmmQemu.BridgeDevice{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other functions for appendBridges assign an address to the bridge as well. Please do that here as well.
Also since the code is identical, can you move it to a common function prefixed with "generic" as @jodh-intel has suggested for other cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you add some unit tests for this architecture?

// hostIsVMContainerCapable checks to see if the host is theoretically capable
// of creating a VM container.
func hostIsVMContainerCapable(details vmContainerCapableDetails) error {
return nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double tab

@@ -454,7 +454,12 @@ func getHostMemorySizeKb(memInfoPath string) (uint64, error) {
// RunningOnVMM checks if the system is running inside a VM.
func RunningOnVMM(cpuInfoPath string) (bool, error) {
if runtime.GOARCH == "arm64" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| runtime.GOARCH == "ppc64le"

@@ -127,6 +127,9 @@ const (

// QemuVirt is the QEMU virt machine type for aarch64
QemuVirt = "virt"

// QemuPPC64le is a QEMU virt machine type for for ppc64le
QemuPPC64le = "pseries"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable name should be QemuPseries

qemuArchBase
}

const defaultQemuPath = "/usr/libexec/qemu-kvm"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean /usr/bin/qemu-kvm ?

{"iommu", "off"},
{"cryptomgr.notests", ""},
{"net.ifnames", "0"},
{"pci", "lastbus=0"},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this improves boot time of PCIe machines types, does QEMU ppc have PCIe machine types?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nitkon you didn't answer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devimc : Thanks for pointing out. pci=lastbus=0 is an intel only feature. POWER does not use it. So we can remove this option for ppc64le.
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/kernel-parameters.txt#n3058
{"iommu", "off"} can also be removed.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/kernel-parameters.txt#n1699
{"cryptomgr.notests", ""}, and {"net.ifnames", "0"}, should improve the boot times.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the parameters analysis, this was a very complete answer 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodh-intel : Is it fine to raise a PR to optimize the kernel parameters for ppc64le.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nitkon - Sure - please do! ;)

// NVDIMM device needs memory space 1024MB
memoryOffset := 1024

// Align hostMemoryMb to 256 MB multiples
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uhhmm this is weird, why 256?, have you tried with 128 MB?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default LMB (Logical Memory Blocksize) in ppc64le arch is 256MB. If we do not adhere to that we get the error: "docker: Error response from daemon: OCI runtime create failed: qemu-system-ppc64: Can't support memory configuration where RAM size 0x80000000 or maxmem size 0x3017400000 isn't aligned to 256 MB: unknown."

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, thanks @nitkon

@@ -0,0 +1,192 @@
// Copyright (c) 2018 IBM
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to run gofmt -s -w on this file

for idx, b := range bridges {
t := govmmQemu.PCIBridge
if b.Type == pcieBridge {
t = govmmQemu.PCIEBridge
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does pseries support pcie ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devimc : Yes .

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, have you compared the boot time with/without pci=lastbus=0 in the kernel command line? probably you will need it


fieldLogger.WithField("device", kvmDevice).Info("device available")

vm, _, errno := syscall.Syscall(syscall.SYS_IOCTL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this patch, but is there any possibility that we have /dev/kvm device but kvm is not enabled? I wonder if the extra step of checking kvm ioctl is necessary. If not, we can remove the only piece of C go from kata runtime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, yes (kind of). This code was added to protect against the scenario where the user has a different hypervisor running (VMWare, VirtualBox, etc) since they stop qemu from using kvm. See:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also why kata-check is specified twice in the README, one with sudo and one without since kata-check needs root access to create a KVM VM (which we then destroy), but you can run the command without root (but we can't then detect if another hypervisor is using it):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fair enough. I guess next possibility is to either push the kvm ioctl number to go runtime, or define it in govmm. Then we can still remove the c go code.

QemuPPC64le: defaultQemuPath,
}

var kernelParams = []Param{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these duplicate with kernelParams in qemu_amd64.go, which means we should define a kernelParamsBase in qemu_arch_base.go.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree we should define a base of common params being shared across different architectures.

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating @nitkon!

Aside from the inline comments, I think it would be good to create atleast minimal qemu_ppc64el_test.go and qemu_arm64_test.go files.

Makefile Outdated
@@ -345,6 +352,7 @@ $(GENERATED_FILES): %: %.in Makefile VERSION
-e "s|@QEMUPATH@|$(QEMUPATH)|g" \
-e "s|@RUNTIME_NAME@|$(TARGET)|g" \
-e "s|@MACHINETYPE@|$(MACHINETYPE)|g" \
-e "s|@KERNELTYPE@|$(KERNELTYPE)|g" \
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 this line is redundant?

# SPDX-License-Identifier: Apache-2.0
#

# Power Ppc64le settings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The capitalisation looks a bit odd here - should it be "PPC64le"?


// variables rather than consts to allow tests to modify them
var (
kvmDevice = "/dev/kvm"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be defined in kata-check.go only.


// hostIsVMContainerCapable checks to see if the host is theoretically capable
// of creating a VM container.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove blank line.

return false
}
// hostIsVMContainerCapable checks to see if the host is theoretically capable
// of creating a VM container.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove blank line.

QemuPseries: defaultQemuPath,
}

var kernelParams = []Param{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all these options really used by ppc64el? If so, I'd suggest moving this to qemu.go so they can be shared by amd64+ppc64el.

kernelParamsDebug: kernelParamsDebug,
kernelParams: kernelParams,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is missing the block the other arches have:

if config.ImagePath != "" {
    ...
}

In fact, since that block of code should I think be common to all architectures, you could make a new function in qemu.go so that all the newQemuArch() functions can call something like:

return handleImagePath(config HypervisorConfig, q qemuArch)

return caps
}

func (q *qemuPPC64le) bridges(number uint32) []Bridge {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is quite a bit of commonality in the following functions between amd64+ppc64el:

  • bridges()
  • memoryTopology()
  • appendBridges()

I'd say it's "borderline refactoring territory" but wdyt @sboeuf?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @sboeuf.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nitkon yes you could factorize more for those functions here. Correct me if I'm wrong but the code for ppc64 is a copy of the implementation for amd64 (for the 3 functions @jodh-intel mentioned). bridges() is slightly different because of the type itself, but the semantic is the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the only outstanding issue stopping this from landing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodh-intel : Thanks for pointing it out. Will update my PR accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodh-intel : @sboeuf :
The code for memoryTopology() is not exactly the same for ppc64 and x86. The addition being in ppc64le

hostMemoryMb -= (hostMemoryMb % 256)

The code for appendBridges() has already been refactored.

// appendBridges appends to devices the given bridges
func (q *qemuPPC64le) appendBridges(devices []govmmQemu.Device, bridges []Bridge) []govmmQemu.Device {
        return genericAppendBridges(devices, bridges, q.machineType)
}

bridges() can be refactored furthure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nitkon - yes. I think all we really need now is something like the following (untested! :) that the arch-specific qemu functions can call:

func genericMakeBridges(number uint32, bt bridgeType) []Bridge {
        var bridges []Bridge

	for i := uint32(0); i < number; i++ {
		bridges = append(bridges, Bridge{
			Type:    bt,
			ID:      fmt.Sprintf("%s-bridge-%d", bt, i),
			Address: make(map[uint32]string),
		})
	}

        return bridges
}


func genericMemoryTopology(memoryMb, hostMemoryMb uint64, alignMB uint32) govmmQemu.Memory {
	// NVDIMM device needs memory space 1024MB
	memoryOffset := 1024

        if alignMB > 0 {
                hostMemoryMb -= (hostMemoryMb % alignMB)
        }

	// add 1G memory space for nvdimm device (vm guest image)
	memMax := fmt.Sprintf("%dM", hostMemoryMb+uint64(memoryOffset))

	mem := fmt.Sprintf("%dM", memoryMb)

	memory := govmmQemu.Memory{
		Size:   mem,
		Slots:  defaultMemSlots,
		MaxMem: memMax,
	}

	return memory
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodh-intel : Thnx for review comment. I saw this post updating my PR. If you find my changes acceptable then we can keep them else I shall revert to the way u have suggested it.

@jodh-intel
Copy link
Contributor

Hi @nitkon - please could you add a note when you've updated the branch again so we can try to get this landed as soon as possible?

@codecov
Copy link

codecov bot commented May 19, 2018

Codecov Report

Merging #286 into master will increase coverage by 0.02%.
The diff coverage is 89.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   63.74%   63.77%   +0.02%     
==========================================
  Files          87       87              
  Lines        8731     8749      +18     
==========================================
+ Hits         5566     5580      +14     
- Misses       2569     2572       +3     
- Partials      596      597       +1
Impacted Files Coverage Δ
cli/kata-env.go 98.57% <0%> (-1.43%) ⬇️
virtcontainers/hypervisor.go 72.78% <0%> (ø) ⬆️
cli/kata-check_amd64.go 100% <100%> (+6.25%) ⬆️
virtcontainers/qemu_amd64.go 92% <100%> (-1.55%) ⬇️
virtcontainers/qemu_arch_base.go 77.52% <100%> (+0.52%) ⬆️
virtcontainers/qemu.go 19.34% <91.66%> (+7.32%) ⬆️
cli/kata-check.go 97.24% <93.54%> (-1.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b0612...e14eab0. Read the comment docs.

@nitkon
Copy link
Contributor Author

nitkon commented May 19, 2018

Hi @jodh-intel : I have updated the PR.

@devimc
Copy link

devimc commented May 22, 2018

@jodh-intel please take a look
@nitkon please rebase it

@katabuilder
Copy link

PSS Measurement:
Qemu: 141956 KB
Proxy: 4565 KB
Shim: 10871 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2012952 KB

@nitkon
Copy link
Contributor Author

nitkon commented May 23, 2018

Rebased my PR.

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. My only concerns are:

  • the duplication in the qemu_${arch}.go.
  • how this changes affects ARM64.

/cc @sboeuf, @bergwolf.


const defaultQemuMachineType = QemuPseries

const defaultQemuMachineOptions = "accel=kvm,usb=off"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USB is also specified by arm64. I wonder if we should also disable for x86_64 @devimc, @mcastelino, @grahamwhaley?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - looks like -usb is required to explicitly enable on x86_64. But we disable it anyway 😄:

return caps
}

func (q *qemuPPC64le) bridges(number uint32) []Bridge {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @sboeuf.

@jodh-intel
Copy link
Contributor

/cc @WeiZhang555.

@jodh-intel
Copy link
Contributor

Hi @nitkon - please can you try running the unit tests for this PR on a PPC64el system:

$ cd $GOPATH/src/github.com/kata-containers/runtime
$ go get -d github.com/kata-containers/tests
$ script -fec 'KATA_DEV_MODE=true ../tests/.ci/go-test.sh'

I think you'll find that you need to atleast create a cli/kata-check_data_ppc64le_test.go.

@jodh-intel
Copy link
Contributor

@nitkon - once you've got the tests passing, and we've agreed on whether to do that bit of refactor (let's wait for @sboeuf's view), I'm happy to ack this btw ;)

@nitkon
Copy link
Contributor Author

nitkon commented May 25, 2018

@jodh-intel : Thanks for the review. I have updated my PR.

=== RUN   TestWriteFileErrNoPath
--- PASS: TestWriteFileErrNoPath (0.00s)
=== RUN   TestVersion
--- PASS: TestVersion (0.00s)
PASS
coverage: 85.3% of statements
ok  	github.com/kata-containers/runtime/cli	3.460s	coverage: 85.3% of statements_

@katabuilder
Copy link

PSS Measurement:
Qemu: 141846 KB
Proxy: 4605 KB
Shim: 10847 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2011092 KB

Copy link

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nitkon nice PR, but I have a request about the format. Could you split this PR into 3 distincts commits so that it is easier to review and easier to understand all changes that have been done.
First commit being the factorization of some functions for the purpose of being reused later by ppc64 arch.
Second commit introducing the qemu ppc64 implementation in virtcontainers, and relying on the new factorized functions.
Last commit being the CLI part leveraging the new support for ppc64.

@nitkon
Copy link
Contributor Author

nitkon commented May 28, 2018

@sboeuf : I have updated my PR as suggested.

@katabuilder
Copy link

PSS Measurement:
Qemu: 145972 KB
Proxy: 6779 KB
Shim: 10691 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2013296 KB

@jodh-intel
Copy link
Contributor

Thanks for fixing the tests @nitkon.

@bergwolf, @sboeuf - ptal.

@devimc
Copy link

devimc commented May 29, 2018

nice patch @nitkon !! lgtm 😄

Copy link

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nitkon this looks better ! Still a few comments/questions not addressed I think.
Also, would be good to factorize the implementation duplication between amd64 and ppc64.

@jodh-intel
Copy link
Contributor

Hi @nitkon - there is still the issue of how we ensure the PPC64el build doesn't break after this PR lands.

Can you help the Kata Project in any way with PPC64el CI resources? We recently noticed that the ARM64 build broke as we don't yet have ARM64 CI support (but it's in progress - see #349). We don't want a repeat of that and as such are very keen to ensure that as new architectures are added, our CI can test them all.

@jodh-intel
Copy link
Contributor

/cc @egernst, @grahamwhaley, @chavafg, @gnawux.

@nitkon
Copy link
Contributor Author

nitkon commented May 30, 2018

@jodh-intel : We have ppc64le support in travis-ci.org. So we need to enhance the runtime/.travis.yml file to run on ppc64le as well.

@jodh-intel
Copy link
Contributor

Hi @nitkon - great to hear! However, please not that we're actually trying to move away from Travis since, atleast for the public service that offers x86_64 services, it doesn't provide the virtualisation extensions we need to run the full suite of tests. See:

@amshinde
Copy link
Member

amshinde commented May 30, 2018

lgtm!

Approved with PullApprove

nitkon added 4 commits May 31, 2018 17:58
Fixes #302

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

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

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

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

PSS Measurement:
Qemu: 161699 KB
Proxy: 6784 KB
Shim: 8818 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 1999484 KB

@sboeuf
Copy link

sboeuf commented May 31, 2018

@nitkon thanks for your work reworking this PR, this LGTM now.
@jodh-intel please check if you're fine with latest changes and merge if you've nothing to add :)

@jodh-intel
Copy link
Contributor

still...

lgtm

Thanks @nitkon for all your work, persistence, and patience! 😄

Please feel free to raise a PR for the Travis changes you mentioned to ensure this new architecture continues to pass all the CI tests.

@jodh-intel jodh-intel merged commit 2400978 into kata-containers:master Jun 1, 2018
zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
…d-ns

namespace: Add check to make sure PID namespace is not received
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants