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

ci: Add integration testing, misc cleanup #141

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

cgwalters
Copy link
Contributor

Right now we only have unit tests that run unprivileged. Add a proper "integration test" (ok it's a lame shell script) that runs as root and creates and mounts a composefs and does some sanity checking. (This could obviously be greatly extended, this is just to have a starting point)

While we have the patient open

  • The only CI tests were doing cross builds; i.e. we were ignoring x86_64. Add a non-cross build, and capture its result (as a tarball) for integration testing
  • Add hacking/installdeps.sh which captures our build dependencies for Debian at least and can be reused by mutiple jobs
  • Add permissions: actions: read because we don't want our actions to be able to mutate anything - this makes untrusted PRs much safer

Right now we only have unit tests that run unprivileged.  Add
a proper "integration test" (ok it's a lame shell script) that
runs as root and creates and mounts a composefs and does some
sanity checking.  (This could obviously be greatly extended, this
is just to have a starting point)

While we have the patient open

- The only CI tests were doing *cross* builds; i.e. we were ignoring
  x86_64.  Add a non-cross build, and capture its result (as
  a tarball) for integration testing
- Add `hacking/installdeps.sh` which captures our build dependencies
  for Debian at least and can be reused by mutiple jobs
- Add `permissions: actions: read` because we don't want our
  actions to be able to mutate anything - this makes untrusted
  PRs much safer

Signed-off-by: Colin Walters <[email protected]>
@cgwalters
Copy link
Contributor Author

I feel like I deserve a cookie 🍪 for figuring out how to find erofs.ko from https://packages.ubuntu.com/source/focal-updates/linux-azure-5.15

Ah oops, I broke the cross build in the last iteration. Hmm fun, the run-on-arch action doesn't mount the sourcedir. Annoying. Will look

@hsiangkao
Copy link
Contributor

hsiangkao commented Jun 7, 2023

Hi @cgwalters,

Many thanks for the effort! I will take more time on composefs use cases as I can if possible as well (if there are any request needed).

As for erofs kernel modules, I've seen an issue before about this,
actions/runner-images#7587
and this:
actions/runner-images#7610

Not quite sure how to proceed though.

@cgwalters
Copy link
Contributor Author

I will take more time on composefs use cases as I can if possible as well (if there are any request needed).

Cool, would love to chat with you and have you involved! Speaking just for myself (I can't imagine the other maintainers would have a problem with this) we could add you as an official reviewer/committer on the project and you could "offically" review/approve PRs to start.

(I'd also like to change this project to require a secondary reviewer to merge changes; opinions on that @alexlarsson ?)

There's a realtime meeting that is being driven from the ostree context (see ostreedev/ostree#2867 ) but if you see what I'm thinking over in #125 I think we should really "hollow out" the stuff that's in ostree, which actually strongly argues for it being a composefs meeting, not an ostree meeting. (So we would determine timing/tooling/etc from the containers/ context, not sure if that practically changes anything immediately, but still)

@cgwalters
Copy link
Contributor Author

As for erofs kernel modules, I've seen an issue before about this,

Oh we're all good, see this line in this PR - I was just unfamiliar with the structure of the Ubuntu kernel packaging.

(It didn't help that mount.composefs if erofs.ko is missing just says No such device; I ended up using https://github.com/mxschmitt/action-tmate to log into the action runner and use strace -f mount.composefs ... to debig)

@hsiangkao
Copy link
Contributor

hsiangkao commented Jun 7, 2023

I will take more time on composefs use cases as I can if possible as well (if there are any request needed).

Cool, would love to chat with you and have you involved! Speaking just for myself (I can't imagine the other maintainers would have a problem with this) we could add you as an official reviewer/committer on the project and you could "offically" review/approve PRs to start.

(I'd also like to change this project to require a secondary reviewer to merge changes; opinions on that @alexlarsson ?)

It's not necessary to add me as composefs project reviewers currently since my own main time is working on EROFS deflate support to enable intel IAA hardware accelerator for now. Later I will have time to review composefs code and do something for composefs use cases on EROFS.

Yes, from my own view, in the long term, if you're interested in EROFS development (and reviewers or maintainers), I'm quite open to it as long as the project itself could be healthy proceeded and involved. Or just use EROFS itself is fine, I and other developers could help on these features alternatively. Actually currently I'm a bit overloaded with EROFS (many TODOs on the list) but I still do as much as I can ;) So feel free to raise good features for composefs honestly and I could seek time helping on this as long as it could involve more people interest in the long term... That is what I'd like to see for an open source project.

There's a realtime meeting that is being driven from the ostree context (see ostreedev/ostree#2867 ) but if you see what I'm thinking over in #125 I think we should really "hollow out" the stuff that's in ostree, which actually strongly argues for it being a composefs meeting, not an ostree meeting. (So we would determine timing/tooling/etc from the containers/ context, not sure if that practically changes anything immediately, but still)

Yes, but my english is not quite good. I could listen but realtime discussion in English might be , sigh...

Copy link
Collaborator

@alexlarsson alexlarsson left a comment

Choose a reason for hiding this comment

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

lgtm

@cgwalters cgwalters merged commit 580d3c6 into containers:main Jun 7, 2023
@alexlarsson
Copy link
Collaborator

(I'd also like to change this project to require a secondary reviewer to merge changes; opinions on that @alexlarsson ?)

@cgwalters As for this, i agree. But I don't seem to have any permissions to change this. @giuseppe are you listed as admin or something and can do this? Maybe we can add colin as reviewer too?

@cgwalters
Copy link
Contributor Author

It's not necessary to add me as composefs project reviewers currently since my own main time is working on EROFS deflate support to enable intel IAA hardware accelerator for now.

Interesting. One thing I'd note related to this is that it's not an EROFS feature that would be directly useful for composefs, which just uses EROFS for metadata (and this feature is about data compression I assume), and for us data is on a separate backing filesystem. (Which in theory could be EROFS too...)

Later I will have time to review composefs code and do something for composefs use cases on EROFS.

Cool. I think for now we can just sync up organically, maybe via "tagging" you on PRs that are relevant to the EROFS parts.

Yes, but my english is not quite good. I could listen but realtime discussion in English might be , sigh...

No worries at all, I think everyone here knows we work in a global community and should be accomodating to varying English language skills. In the end we also communicate with code 😄

@cgwalters
Copy link
Contributor Author

@cgwalters As for this, i agree. But I don't seem to have any permissions to change this. @giuseppe are you listed as admin or something and can do this? Maybe we can add colin as reviewer too?

I do, I've done it now; changes to main require a distinct approver and require the "integration" and "clang-format" tests to pass right now.

I think between the 3 of us so far we can probably avoid having PRs linger too long, and hopefully we'll build up the set of reviewers even more - there's definitely interest!

@hsiangkao
Copy link
Contributor

hsiangkao commented Jun 7, 2023

Interesting. One thing I'd note related to this is that it's not an EROFS feature that would be directly useful for composefs, which just uses EROFS for metadata (and this feature is about data compression I assume), and for us data is on a separate backing filesystem. (Which in theory could be EROFS too...)

Add stacking approach to a filesystem is quite hard, see currently ecryptfs [1] (it's almost orphan and will be removed). Overlayfs might be the only stacking fs in the kernel eventually since lots of corner cases about this.

On the other side, overlayfs could do a writeable layer for us and if lower data layers are in overlayfs and in a same fs (e.g. XFS or btrfs) as well, it can do partial copyup with reflink for us (by using clone_file_range).

If we do stacking approach on our side, we also need to consider writable layer stuffs as above, and then the complete story could become somewhat controversial for the whole Linux filesystem community honestly due to duplicated features since the current approach (erofs+overlayfs) works (although not quite to the principal performance limit) and proposed features can benefit to the whole Linux community. Or if there could be a liboverlayfs in the future for EROFS to integrate, there could be another story (apart from composefs model, I do think it's quite useful to have an in-line writable layer for EROFS but it needs careful discussion), but I've seen it might be a long term plan and we should not be blocked on this.

No worries at all, I think everyone here knows we work in a global community and should be accomodating to varying English language skills. In the end we also communicate with code 😄

Yes I will try anyway. :)

[1] https://lore.kernel.org/r/ZCuSLNnFQEdOHW0c@sequoia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants