Skip to content

Conversation

@dcookspi
Copy link
Collaborator

@dcookspi dcookspi commented Oct 3, 2024

This adds support for list of layer references file to spfs run's command line to allow for larger environments (number of layesr) in smaller/shorter command lines.

This changes the EnvSpecItem for live layer files to allow more than one kind of yaml file in environment specifiers. It adds an EnvLayersFile that can contain a list of spfs references: disgests, tags, etc.. This extends and reworks the parsing and enums to support the new file, and moves LiveLayers to their own source code file.

Note: I'm not sure about some of the names I used for enums and entries, and the location of at least one enum. See the TODO: markers in the code, suggestions welcome.

An example layers list file:

# layers.spfs.yaml
api: v0/layerlist
layers:
  - A7USTIBXPXHMD5CYEIIOBMFLM3X77ESVR3WAUXQ7XQQGTHKH7DMQ====
  - spfs/some/tag/to/some/object
  - /path/to/some/live_layer.spfs.yaml
  - 6PJDUUENJYFFDKZWRKHDUXK4FGGZ7FHDYMZ7P4CXORUG6TUMDJ7A====

Note:
I haven't put any checks in to stop references to other layer list files in the code, or any depth limits on them. So it should be possible to put paths to other layer list files in a layer list file, and nest them indefinitely. I haven't tried that. If that's a concern I can add a check or limit.

@dcookspi dcookspi added SPI AOI Area of interest for SPI SPI-0.43 Marked for inclusion in SPI 0.43 build labels Oct 3, 2024
@dcookspi dcookspi self-assigned this Oct 3, 2024
@dcookspi dcookspi requested review from jrray and rydrman October 3, 2024 19:43
api: SpfsFileApiVersion,
}

// TODO: rename SpfsEnvFile maybe?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm leaning towards calling these spec files to keep it generic and mimic the terminology used in spk.

We might introduce other types of files in the future that follow the api: convention and they won't all necessarily belong to a common theme for what they are used for.

So SpfsSpecFile is what I'm thinking or something along those lines.

BindMount(BindMount),
}

// TODO: move and rename? where should this live?
Copy link
Collaborator

Choose a reason for hiding this comment

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

To go with my other comment, SpfsSpecApiVersion. I don't think it needs the word "file" in there.

Yes it should be in its own file since the concept is more general than live layers. Or is it? The doc string suggests they are both live layer types.

Copy link
Collaborator

@rydrman rydrman Oct 9, 2024

Choose a reason for hiding this comment

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

Perhaps an spfs::schema module to parallel the spk one? IMO this can just be spfs::schema::{ApiVersion, Spec} since we don't need to reestablish that it's in the spfs crate

@dcookspi dcookspi force-pushed the add-layers-file-support-to-spfs branch from 694be0f to 1f7061b Compare October 10, 2024 20:07
@dcookspi
Copy link
Collaborator Author

dcookspi commented Oct 10, 2024

Todo:

  • remove the spfs from the api verion mapping related types
  • decide on references to layer files inside layer files, currently a recursive reference will break spfs run

@dcookspi dcookspi force-pushed the add-layers-file-support-to-spfs branch from fa45018 to 8639671 Compare October 21, 2024 22:41
Comment on lines +35 to +36
static SEEN_SPEC_FILES: Lazy<std::sync::Mutex<HashSet<PathBuf>>> =
Lazy::new(|| std::sync::Mutex::new(HashSet::new()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like this ever gets cleared, right? We have some long-running processes and I worry that we might even simply try to load the same file twice which would cause this to error with no way to recover. It's something that I can refactor if we run into it, but for the future I think we should avoid this kind of global state unless it actually lives in the command line crate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's not cleared. I've added a function to clear it, in case we need it. I did feel while working on this, the things in tracking/env.rs seemed to be partly closer to command line level (parsing), and partly lower level (tag/digest resolution).

pub enum SpecApiVersion {
#[serde(rename = "v0/layer", alias = "v0/livelayer")]
V0Layer,
#[serde(rename = "v0/layerlist")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

my final hesitation with layerlist is that it feels too explicit - eg we won't be able to extend the data in this file without the name becoming confusing. Did we already consider v0/platform? Because I could also see this being a really useful way to define and then commit platforms directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is closer to an EnvSpec. It's basically anEnvSpec in a file. How about v0/envspec?
Or v0/referencelist to line up with what spfs run -h calls an env spec (REFERENCE).

Copy link
Collaborator

@jrray jrray Oct 30, 2024

Choose a reason for hiding this comment

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

🎉 spfs/v0/runspec 🎉
/spfs/v0/runspec?
v0/spfs/runspec?

v0/runspec [/spk] [/spfs]

There are now 2 kinds of .spfs.yaml file that can be given to spfs run
as environemnt spec items. This adds parsing and enums to support
both, and moves LiveLayers to their own source code file.

Signed-off-by: David Gilligan-Cook <[email protected]>
Signed-off-by: David Gilligan-Cook <[email protected]>
Uses a spfs spec filepath cache for the check, will error if a
duplicate filepath is encountered.

Adds flattening nested list of layers files into the EnvSpec during parsing.

Signed-off-by: David Gilligan-Cook <[email protected]>
Signed-off-by: David Gilligan-Cook <[email protected]>
layers files to two passes.

Signed-off-by: David Gilligan-Cook <[email protected]>
Signed-off-by: David Gilligan-Cook <[email protected]>
@dcookspi dcookspi force-pushed the add-layers-file-support-to-spfs branch from 8874ae1 to d70eb50 Compare October 31, 2024 00:52
Signed-off-by: David Gilligan-Cook <[email protected]>
Comment on lines +12 to +13
alias = "v0/livelayer",
alias = "v0/layer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to remove these aliases unless you have a lot of these files lying around already - it seems better if we don't need to support these unqualified ones

Signed-off-by: David Gilligan-Cook <[email protected]>
@dcookspi
Copy link
Collaborator Author

I needed to update the docs the for spfs/v0/runspec change. So that's done now.

@dcookspi dcookspi merged commit f199440 into main Oct 31, 2024
5 checks passed
@dcookspi dcookspi deleted the add-layers-file-support-to-spfs branch October 31, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

SPI AOI Area of interest for SPI SPI-0.43 Marked for inclusion in SPI 0.43 build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants