Adding requirements to support ARM instance #469
Conversation
network_test.go
Outdated
| fctesting.RequiresRoot(t) | ||
|
|
||
| cniBinPath := []string{"/opt/cni/bin", testDataBin} | ||
| cniBinPath := []string{"/testdata/bin", testDataBin} |
There was a problem hiding this comment.
| cniBinPath := []string{"/testdata/bin", testDataBin} | |
| cniBinPath := []string{testDataBin, "/opt/cni/bin"} |
testDataBin is defined here and should provide the path to these CNI plugins built:
firecracker-go-sdk/machine_test.go
Line 71 in 4648d28
I don't think we can guarantee "/testdata" exists.
There was a problem hiding this comment.
Previously CNI plugins are set towards /opt/cni/bin which consists of binaries in X86 format. But when we require the ARM related binaries and they exists in testdata/bin.
There was a problem hiding this comment.
Yes, we should put the testdata binaries on PATH before "/opt/cni/bin"; testDataBin gives us a reference to the path to testdata binaries. "/testdata/bin" is not guranteed to exist on all setups so we should use the testDataBin variable
firecracker-go-sdk/machine_test.go
Lines 69 to 71 in 8378e6a
There was a problem hiding this comment.
Changed the variable to testDataBin
network_test.go
Outdated
| assert.FileExists(t, expectedCacheDirPath, "CNI cache dir doesn't exist after vm startup") | ||
|
|
||
| testPing(t, vmIP, 3, 5*time.Second) | ||
| if runtime.GOARCH != "arm64" { |
There was a problem hiding this comment.
worth checking for arm as well?
There was a problem hiding this comment.
I have tried by printing runtime.GOARCH to check if the arch is ARM64
There was a problem hiding this comment.
Unlike sending Ctrl + Alt + Del, networking should work on aarch64. Do you know the root cause?
There was a problem hiding this comment.
TestPing() is giving internet gateway error such as "Destination Host Unreachable" and not able to ping multiple VM's at a time.
vvejell1
force-pushed
the
arm
branch
3 times, most recently
from
dd86cd6 to
6d2bbf6
Compare
.buildkite/pipeline.yml
Outdated
| commands: | ||
| - "sudo -E PATH=$PATH FC_TEST_DATA_PATH=${FC_TEST_DATA_PATH} make -C examples/cmd/snapshotting run" | ||
| - "sudo -E PATH=$PATH FC_TEST_DATA_PATH=${FC_TEST_DATA_PATH} make -C examples/cmd/snapshotting clean" | ||
| - "sudo -E PATH=$FC_TEST_DATA_PATH:$PATH FC_TEST_DATA_PATH=${FC_TEST_DATA_PATH} make -C examples/cmd/snapshotting run" |
There was a problem hiding this comment.
To get Arm version binaries like Jailer and Firecracker which are located in TEST_DATA_PATH and added it to PATH
network_test.go
Outdated
| assert.FileExists(t, expectedCacheDirPath, "CNI cache dir doesn't exist after vm startup") | ||
|
|
||
| testPing(t, vmIP, 3, 5*time.Second) | ||
| if runtime.GOARCH != "arm64" { |
There was a problem hiding this comment.
Unlike sending Ctrl + Alt + Del, networking should work on aarch64. Do you know the root cause?
|
Your PR description is good. Please put some of that in the commit message itself. |
vvejell1
force-pushed
the
arm
branch
13 times, most recently
from
68e22ab to
b985524
Compare
network_test.go
Outdated
|
|
||
| numVMs := 10 | ||
| vmIPs := make(chan string, numVMs) | ||
| if runtime.GOARCH != "arm64" { |
There was a problem hiding this comment.
- Let's use t.Skip instead
- If possible, we should skip this test entirely. It doesn't make much sense to setup the VM, if we don't run any validations against the VM.
There was a problem hiding this comment.
sure, let me try in that case.
There was a problem hiding this comment.
After using t.skip() here, TestMicroVMExecution test got failed in the BuildKite because we are skipping and not allowing the VM's to perform the test.
https://buildkite.com/firecracker-microvm/vvejella-firecracker-go-sdk-arm/builds/27#01852ca2-9f4c-41aa-b2a4-0d6e834c6399:~:text=FAIL%3A-,TestMicroVMExecution,-(30.03s)
There was a problem hiding this comment.
I don't think they are related. The VM here and TestMicroVMExecution's VM are different.
There was a problem hiding this comment.
May be it is causing flaky test and now I test is fine
network_test.go
Outdated
| numVMs := 10 | ||
| vmIPs := make(chan string, numVMs) | ||
| if runtime.GOARCH == "arm64" { | ||
| t.Skip() |
There was a problem hiding this comment.
Let's return from here. So that we don't have to nest the lines below.
There was a problem hiding this comment.
Changed the skip by replacing it with return without nesting the lines below.
vvejell1
force-pushed
the
arm
branch
2 times, most recently
from
d8ae7a5 to
2778a98
Compare
vvejell1
force-pushed
the
arm
branch
3 times, most recently
from
eb0e9db to
1516929
Compare
- If arch is ARM, SendCtrlAltDel is not supported and Skip testPing Signed-off-by: Vaishnavi Vejella <[email protected]>
Adding requirements to support ARM instance Signed-off-by: Debian <[email protected]>
3 participants
Signed-off-by: Vaishnavi Vejella [email protected]
Issue:
While trying to perform ARM integration test against firecracker-go-sdk, found that SendCtrlAltDel is not supported on aarch64 and TestNetworkMachineCNIWithConfFile test is failing because testPing is not supported when we are running the test on multiple VM's.
Description of changes:
If the architecture is ARM64, we are not able to send SendCtrlAltDel (API) on aarch64, so stopping the machine using
StopVMM()and also skipping the test by not passing the VM's then TestNetworkMachineCNIWithConfFile will get passed.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.