Unit tests necessarily need to be maintained with the code they test so
it makes sense to keep the Unit Test Advice document into the main repo
since that is where the majority of unit tests reside.
Note: The
[`Unit-Test-Advice.md` file](https://github.com/kata-containers/tests/blob/main/Unit-Test-Advice.md)
was copied from the `tests` repo when it's `HEAD` was
38855f1f40.
Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
Move the documentation requirements document link up so that it appears
immediately below the "How to Contribute" section.
Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
os.Exit() will terminate program immediately, the defer functions
won't be executed, so we add defer functions again before os.Exit().
Refer to https://pkg.go.dev/os#ExitFixes: #3059
Signed-off-by: Binbin Zhang <binbin36520@gmail.com>
Replace some `unwrap()` and `expect()` calls with code to return the
error to the caller.
Fixes: #3011.
Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
Improved the `verify_cid()` function that validates container ID's by
removing the need for an `unwrap()`.
Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
Change `baremount()` to accept `Path` values rather than string values
since:
- `Path` is more natural given the function deals with paths.
- This minimises the caller having to convert between string and `Path`
types, which simplifies the surrounding code.
Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
This PR links the kata-deloy installation instructions to the
docs/install folder.
Fixes: #2450
Signed-off-by: João Vanzuita <joao.vanzuita@de.bosch.com>
Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
Although the binary name of the shipped binary is `qemu-system-x86_64`,
and we only ship kata-deploy for `x86_64`, we better leaving the
architecture specific name out of our README file.
Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
The unrecognized option: 'deny-warnings' args caused `make optimize` failed.
Fixed the Makefile of the agent project, make sure the `make optimize` command
execute correctly. This PR modify the rustc args from '--deny-warnings' to
'--deny warnings'.
Fixes: #3104
Signed-off-by: wangyongchao.bj <wangyongchao.bj@inspur.com>
Commit 3c9ae7f made /test_kata_deploy run
against HEAD, but it also mistakenly removed all the checks that ensure
/test_kata_deploy only runs when explicitly called.
Mea culpa on this, and let's add the tests back.
Fixes: #3101
Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
We noticed s390x test failures on several of the watcher unit tests.
Discovered that on s390 in particular, if we update a file in quick
sucecssion, the time stampe on the file would not be unique between the
writes. Through testing, we observe that a 20 millisecond delay is very
reliable for being able to observe the timestamp update. Let's ensure we
have this delay between writes for our tests so our tests are more
reliable.
In "the real world" we'll be polling for changes every 2 seconds, and
frequency of filesystem updates will be on order of minutes and days,
rather that microseconds.
Fixes: #2946
Signed-off-by: Eric Ernst <eric_ernst@apple.com>
The k8s SR-IOV plugin, when it assigns a VFIO device to a container, adds
an variable of the form PCIDEVICE_<identifier> to the container's
environment, so that the payload knows which device is which. The contents
of the variable gives the PCI address of the device to use.
Kata allows VFIO devices to be passed in to a Kata container, however it
runs within a VM which has a different PCI topology. In order for the
payload to find the right device, the environment variables therefore need
to be converted to list the guest PCI addresses instead of the host PCI
addresses.
fixes#2897
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
update_spec_devices() takes a bunch of updates for the device entries in
the OCI spec and applies them, adjusting things in both the linux.devices
and linux.resources.devices sections of the spec.
It's important that each entry in the spec only be updated once. Currently
we ensure this by first creating an index of where the entries are, then
consulting that as we apply each update, so that earlier updates don't
cause us to incorrectly detect an entry as being relevant to a later
update. This method works, but it's quite awkward.
This inverts the loop structure in update_spec_devices() to make this
clearer. Instead of stepping through each update and finding the relevant
entries in the spec to change, we step through each entry in the spec and
find the relevant update. This makes it structurally clear that we're only
updating each entry once.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
We have a test case commented as testing the case where linux.devices is
empty in the OCI spec. While it's true that linux.devices is empth in this
example, the reason it fails isn't specifically because it's empty but
because it doesn't contain a device for the update we're trying to apply.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
update_spec_devices() explicitly checks for being called with an empty
container path and fails. We have a unit test to verify this behaviour.
But while an empty container_path probably does mean something has gone
wrong elsewhere, that's also true of any number of other bad paths. Having
an empty string here doesn't prevent what we're doing in this function
making sense - we can compare it to the strings in the OCI spec perfectly
well (though more likely we simply won't find it there).
So, there's no real reason to check this one particular odd case.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The DevIndex data structure keeps track of devices in the OCI
specification. We used to carry it around to quite a lot of
functions, but it's now used only within update_spec_devices(). That
means we can simplify things a bit by just open coding the maps we
need, rather than declaring a special type.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
update_spec_device() adjusts the OCI spec for device differences
between the host and guest. It is called repeatedly for each device
we need to alter. These calls are now all in a single loop in
add_devices(), so it makes more sense to move the loop into a renamed
update_spec_devices() and process all the fixups in one call.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Currently the DevNumUpdate structure is created with a path to a
device node in the VM, which is then used by update_spec_device().
However the only piece of information that update_spec_device()
actually needs is the VM side major and minor numbers for the device.
We can determine those when we create the DevNumUpdate structure.
This means we detect errors earlier and as a bonus we don't need to
make a copy of the vm path string.
Since that change requires updating 2 of the log statements, we take the
opportunity to update all the log statements to structured style.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
For each device in the OCI spec we need to update it to reflect the guest
rather than the host. We do this with additional device information
provided by the runtime. There should only be one update for each device
though, if there are multiple, something has gone horribly wrong.
Detect and report this situation, for safety.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
As we process container devices in the agent, we repeatedly call
update_spec_device() to adjust the OCI spec as necessary for differences
between the host and the VM. This means that for the whole of a pretty
complex call graph, the spec is in a partially-updated state - neither
fully as it was on the host, not fully as it will be for the container
within the VM.
Worse, it's not discernable from the contents itself which parts of the
spec have already been updated and which have not. We used to have real
bugs because of this, until the DevIndex structure was introduced, but that
means a whole, fairly complex, parallel data structure needs to be passed
around this call graph just to keep track of the state we're in.
Start simplifying this by having the device handler functions not directly
update the spec, but instead return an update structure describing the
change they need. Once all the devices are added, add_devices() will
process all the updates as a batch.
Note that collecting the updates in a HashMap, rather than a simple Vec
doesn't make a lot of sense in the current code, but will reduce churn
in future changes which make use of it.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Currently update_spec_device() takes parameters 'vm_path' and 'final_path'
to give it the information it needs to update a single device in the OCI
spec for the guest. This bundles these parameters into a single structure
type describing the updates to a single device. This doesn't accomplish
much immediately, but will allow a number of further cleanups.
At the same time we change the representation of vm_path from a Unicode
string to a std::path::Path, which is a bit more natural since we are
performing file operations on it.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
virtio_blk_device_handler(), virtio_blk_ccw_device_handler() and
virtio_scsi_device_handler() all take a clone of their 'device' parameter.
They appear to do this in order to get a mutable copy in which they can
update the vm_path field.
However, the copy is dropped at the end of the function, so the only thing
that's used in it is the vm_path field passed to update_spec_device()
afterwards.
We can avoid the clone by just using a local variable for the vm_path.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
update_spec_device() takes a 'final_path' parameter which gives the
name the device should be given in the "inner" OCI spec. We need this
for VFIO devices where the name the payload sees needs to match the
VM's IOMMU groups. However, in all other cases (for now, and maybe
forever), this is the same as the original 'container_path' given in
the input OCI spec. To make this clearer and simplify callers, make
this parameter an Option, and only update the device name if it is
non-None.
Additionally, update_spec_device() needs to call to_string() on
update_path to get an owned version. Rust convention[0] is to let the
caller decide whether it should copy, or just give an existing owned
version to the function. Change from &str to String to allow that; it
doesn't buy us anything right now, but will make some things a little
nicer in future.
[0] https://rust-lang.github.io/api-guidelines/flexibility.html?highlight=clone#caller-decides-where-to-copy-and-place-data-c-caller-control
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
update_spec_device() takes a 'host_path' parameter which it uses to locate
the device to correct in the OCI spec. Although this will usually be the
path of the device on the host, it doesn't have to be - a traditional
runtime like runc would create a device node of that name in the container
with the given (host) major and minor numbers. To clarify that, rename it
to 'container_path'.
We also update the block comment to explain the distinctions more
carefully. Finally we update some variable names in tests to match.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
This error is returned if we have information for a device from the
runtime, but a matching device does not appear in the OCI spec. However,
the name for the device we print is the name from the VM, rather than the
name from the container which is what we actually expect in the spec.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
update_spec_devices() includes an unsafe block, in order to call the libc
functions to get the major and minor numbers from a device ID. However,
the nix crate already has a safe wrapper for this function, which we use in
other places in the file.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
- Even a directory could be a symlink - check for this. This is very
common when using configmaps/secrets
- Add unit test to better mimic a configmap, configmap update
- We would never remove directories before. Let's ensure that these are
added to the watched_list, and verify in unit tests
- Update unit tests which exercise maximum number of files per entry. There's a change
in behavior now that we consider directories/symlinks watchable as well.
For these tests, it means we support one less file in a watchable mount.
Signed-off-by: Eric Ernst <eric_ernst@apple.com>
The current implementation just copies the file, dereferencing any
simlinks in the process. This results in symlinks no being preserved,
and a change in layout relative to the mount that we are making
watchable.
What we want is something like "cp -d"
This isn't available in a crate, so let's go ahead and introduce a copy
function which will create a symlink with same relative path if the
source file is a symlink. Regular files are handled with the standard
fs::copy.
Introduce a unit test to verify symlinks are now handled appropriately.
Fixes: #2950
Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Is the past few releases we ended up hitting issues that could be easily
avoided if `/test_kata_deploy` would use HEAD instead of a specific
tarball.
By the end of the day, we want to ensure kata-deploy works, but before
we cut a release we also want to ensure that the binaries used in that
release are in a good shape. If we don't do that we end up either
having to roll a release back, or to cut a second release in a really
short time (and that's time consuming).
Note: there's code duplication here that could and should be avoided,b
but I sincerely would prefer treating it in a different PR.
Fixes: #3001
Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
