Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

albatross_daemon: discover root policy and insert this #190

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Conversation

hannesm
Copy link
Collaborator

@hannesm hannesm commented Sep 18, 2024

Previously, there was no such thing. Now, to be able to avoid overcommitment, we discover the available cpus, memory, block device size, and bridges at startup. This results in some changed semantics:

  • if a bridge used by some policy is not configured, we bail early
  • if the policies lead to overcommitment, we bail early

We discover the root policy at startup each time (and also compare to the previous one), since we want to discover the data of the current system. It may be different from the previous one (network bridges, available memory, ...).

//cc @PizieDust (also happy to get a review from @reynir)

Previously, there was no such thing. Now, to be able to avoid overcommitment, we
discover the available cpus, memory, block device size, and bridges at startup.
This results in some changed semantics:
- if a bridge used by some policy is not configured, we bail early
- if the policies lead to overcommitment, we bail early

We discover the root policy at startup each time (and also compare to the
previous one), since we want to discover the data of the current system. It may
be different from the previous one (network bridges, available memory, ...).
Copy link
Contributor

@PizieDust PizieDust left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. 💯

Copy link
Contributor

@reynir reynir left a comment

Choose a reason for hiding this comment

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

I think this looks good. There are some Linux details I would need to look further into. I'd also like to test it myself

src/vmm_stubs.c Outdated Show resolved Hide resolved
}

#ifdef __linux__
#include <sys/vfs.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

From the man page it says:

#include <sys/vfs.h>    /* or <sys/statfs.h> */

CAMLparam1(path);
struct statfs s;
const char *p = String_val(path);
if (statfs(p, &s) < 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

On Linux it seems statfs(2) is deprecated in favor of statvfs(3). I'm not sure why or whether it's important.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I first tried with statvfs, but this reported on FreeBSD the wrong numbers. of course we can use statfs on BSD, and statvfs on linux, if this is preferable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm interesting. According to this stackoverflow discussion Linux' statfs() doesn't work well with large files. Though, testing with a ~8 TB filesystem it seems to work fine. So I'm not sure where the limit is (I don't have access to larger filesystems I think). https://stackoverflow.com/questions/1653163/difference-between-statvfs-and-statfs-system-calls

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe we should guard against < 0...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, the ~8 TB filesystem uses 128K blocks (zfs) - so it would have about as many blocks as a 250 GB filesystem using 4k blocks...

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should guard against < 0...

Done in 83fd12f

if n = 0 then acc else gen_cpu (Vmm_core.IS.add (pred n) acc) (pred n)
in
Ok { Vmm_core.Policy.vms = max_int ;
cpuids = gen_cpu Vmm_core.IS.empty cpus ;
Copy link
Contributor

Choose a reason for hiding this comment

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

A potential helper function vm_count_of_cpu could be a list of tuples that indicate how many vms are currently bound to each cpu. Will this be better on albatross or mollymawk?

Something like:

[(1,20);(2,0)] 

etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not keen on modifying the policy type. I think in mollymawk there could be some of that information, i.e.:

  • we have the root policy, seeing that CPUs 0..19 exist
  • we have two policies "foo" with cpu id 0,1,2,3,4,5 and "bar" with cpu id 6, 7, 8, 9, 10
  • we display the information in a way that 0..10 are "already in use" (but can as well be assigned), and 11..19 are not used at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so, indeed this fits better into mollymawk from my point of view.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed I think this approach is much better

src/vmm_stubs.c Outdated Show resolved Hide resolved
Copy link
Contributor

@PizieDust PizieDust left a comment

Choose a reason for hiding this comment

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

I'm happy about this root_policy. Thank you @hannesm

Copy link
Contributor

@reynir reynir left a comment

Choose a reason for hiding this comment

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

I think this is fine to merge. The Linux statfs() vs statvfs() issue can be addressed later if it turns out to be a problem (famously, It Works For Me™).

There are Nix CI errors that maybe @Julow is motivated to fix?

@reynir reynir merged commit 07806a5 into main Sep 18, 2024
9 of 13 checks passed
@reynir reynir deleted the root-policy branch September 18, 2024 14:49
hannesm added a commit to hannesm/opam-repository that referenced this pull request Oct 7, 2024
CHANGES:

* Albatross-daemon: discover and provide root policy with the available system
  resources (robur-coop/albatross#190 @hannesm @reynir @PizieDust)
* Vmm_core.Policy.usable: check that block size, if present, is non-negative
  (robur-coop/albatross#192 @hannesm)
* BREAKING: rename vm to unikernel (robur-coop/albatross#192 @hannesm)
* Document Vmm_trie (robur-coop/albatross#192 @hannesm)
* Parse /proc/pid/status less strict (robur-coop/albatross#193 @Firobe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants