search

mikelangelo-project / capstan

Capstan, a tool for packaging and running your application on OSv.
http://osv.io/capstan/
Other
19 stars 7 forks source link

Add support to compose OSv images with ROFS (Read-Only Filesystem) #101

Closed wkozaczuk closed 6 years ago

wkozaczuk commented 6 years ago

These enclosed changes implement logic enhancing capstan to compose OSv images with ROFS filesystem as described by https://raw.githubusercontent.com/cloudius-systems/osv/master/scripts/gen-rofs-img.py.

The package compose command has new '-- fs' option allowing to specify filesystem - zfs or rofs. The zfs fileystem is default.

The example of building OSv image with ROFS: capstan package compose java_example -fs rofs

It fixes #98.

miha-plesko commented 6 years ago

@wkozaczuk the code seems good and default behavior seems not to be changed. Can I ask you for some short documentation as well, please? By adding a new file Documentation/OsvFilesystem.md (and a link to it from "Documentation" section in the main readme). Don't need much, but at least quick description of what is the difference for user if she picks ROFS over ZFS so she can decide which one to pick.

Also, I will not be testing the PR in details as we currently have no effort available for this repo. But I'm willing to merge as long as you can confirm that basic usecase works for you with both filesystems. By basic example I mean e.g. to run simple node.js appliaction https://github.com/mikelangelo-project/capstan/blob/master/Documentation/RuntimeNode.md#interactive-node-interpreter (build unikernel, upload files, run unikernel). Can you please confirm that those steps all still work?

Thanks for your contribution!

miha-plesko commented 6 years ago

Do you happen to know why Travis is red now? Looks like it's not connected to this patch, but I'm not sure. Would be great if we can make it green.

wkozaczuk commented 6 years ago

Hi Miha,

I am so glad to hear from you! Thanks for your efforts.

I have tested the app against some Java packages and native app. I will check NodeJS as well. Lastly I will add the documentation you are asking.

As far as failing tests go it is actually unrelated to my changes but instead to the updates in the Yaml library capstan depends on. So locally all the tests were passing locally on my computer until I ran 'go get -u all' which I did after I saw the Jenkins errors. So you can reproduce locally yourself on master without my changes.

I debugged the problem and it is not very easy to fix. I am thinking it is a bug in that library that causes the 'created' timestamp file to get enclosed in unnecessary quotes like so:

CASE #1: miliseconds should not get marshalled
yaml_test.go:70:
    c.Check(string(data), Equals, args.expected+"\n")
... obtained string = "created: \"2017-09-14T18:08:16+02:00\"\n"
... expected string = "created: 2017-09-14T18:08:16+02:00\n"

In essence this code https://github.com/go-yaml/yaml/blob/v2/encode.go#L272-L307 decides to use yaml_DOUBLE_QUOTED_SCALAR_STYLE style. Not sure how to fix it yet.

One way would be to go back to previous version of the library (I am sure something must have changed to make it behave differently). What do you think?

Waldek

On Fri, Sep 14, 2018 at 5:03 AM Miha Pleško notifications@github.com wrote:

@wkozaczuk https://github.com/wkozaczuk the code seems good and default behavior seems not to be changed. Can I ask you for some short documentation as well, please? By adding a new file Documentation/OsvFilesystem.md (and a link to it from "Documentation" https://github.com/mikelangelo-project/capstan#documentation section in the main readme). Don't need much, but at least quick description of what is the difference for user if she picks ROFS over ZFS so she can decide which one to pick.

Also, I will not be testing the PR in details as we currently have no effort available for this repo. But I'm willing to merge as long as you can confirm that basic usecase works for you with both filesystems. By basic example I mean e.g. to run simple node.js appliaction https://github.com/mikelangelo-project/capstan/blob/master/Documentation/RuntimeNode.md#interactive-node-interpreter (build unikernel, upload files, run unikernel). Can you please confirm that those steps all still work?

Thanks for your contribution!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mikelangelo-project/capstan/pull/101#issuecomment-421283309, or mute the thread https://github.com/notifications/unsubscribe-auth/AFDSIZoBtJgVezKtkd439-bVjyOonyWOks5ua3DlgaJpZM4WoZzI .

wkozaczuk commented 6 years ago

It looks like dependencies management in Go is still not a solved problem - https://blog.boatswain.io/post/manage-go-dependencies-using-dep/

On Fri, Sep 14, 2018 at 7:42 AM Waldek Kozaczuk jwkozaczuk@gmail.com wrote:

Hi Miha,

I am so glad to hear from you! Thanks for your efforts.

I have tested the app against some Java packages and native app. I will check NodeJS as well. Lastly I will add the documentation you are asking.

As far as failing tests go it is actually unrelated to my changes but instead to the updates in the Yaml library capstan depends on. So locally all the tests were passing locally on my computer until I ran 'go get -u all' which I did after I saw the Jenkins errors. So you can reproduce locally yourself on master without my changes.

I debugged the problem and it is not very easy to fix. I am thinking it is a bug in that library that causes the 'created' timestamp file to get enclosed in unnecessary quotes like so:

CASE #1: miliseconds should not get marshalled
yaml_test.go:70:
    c.Check(string(data), Equals, args.expected+"\n")
... obtained string = "created: \"2017-09-14T18:08:16+02:00\"\n"
... expected string = "created: 2017-09-14T18:08:16+02:00\n"

In essence this code https://github.com/go-yaml/yaml/blob/v2/encode.go#L272-L307 decides to use yaml_DOUBLE_QUOTED_SCALAR_STYLE style. Not sure how to fix it yet.

One way would be to go back to previous version of the library (I am sure something must have changed to make it behave differently). What do you think?

Waldek

On Fri, Sep 14, 2018 at 5:03 AM Miha Pleško notifications@github.com wrote:

@wkozaczuk https://github.com/wkozaczuk the code seems good and default behavior seems not to be changed. Can I ask you for some short documentation as well, please? By adding a new file Documentation/OsvFilesystem.md (and a link to it from "Documentation" https://github.com/mikelangelo-project/capstan#documentation section in the main readme). Don't need much, but at least quick description of what is the difference for user if she picks ROFS over ZFS so she can decide which one to pick.

Also, I will not be testing the PR in details as we currently have no effort available for this repo. But I'm willing to merge as long as you can confirm that basic usecase works for you with both filesystems. By basic example I mean e.g. to run simple node.js appliaction https://github.com/mikelangelo-project/capstan/blob/master/Documentation/RuntimeNode.md#interactive-node-interpreter (build unikernel, upload files, run unikernel). Can you please confirm that those steps all still work?

Thanks for your contribution!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mikelangelo-project/capstan/pull/101#issuecomment-421283309, or mute the thread https://github.com/notifications/unsubscribe-auth/AFDSIZoBtJgVezKtkd439-bVjyOonyWOks5ua3DlgaJpZM4WoZzI .

miha-plesko commented 6 years ago

Thanks @wkozaczuk for looking at the problem. It seems like my fault because I monkey-patched datetime marshalling https://github.com/mikelangelo-project/capstan/blob/master/core/yaml.go#L16, that's why it's failing now with newer version of the library. I agree that we can lock dependencies, and I think it's somehow possible by using GoDeps, our current setup is here https://github.com/mikelangelo-project/capstan/tree/master/Godeps. But I'm not sure if Travis is really using GoDeps.

Anyway, if you're all on fire to help us fix this in separate PR I'd appreciate the effort, but we can merge even with red Travis when you provide the documentation, as long as you can confirm tests are all green in your local dev at least. I trust you know what you're doing based on our great experience in past.

wkozaczuk commented 6 years ago

I will try to come up with some workaround for this YAML issue.

On Fri, Sep 14, 2018 at 8:08 AM Miha Pleško notifications@github.com wrote:

Thanks @wkozaczuk https://github.com/wkozaczuk for looking at the problem. It seems like my fault because I monkey-patched datetime marshalling https://github.com/mikelangelo-project/capstan/blob/master/core/yaml.go#L16, that's why it's failing now with newer version of the library. I agree that we can lock dependencies, and I think it's somehow possible by using GoDeps, our current setup is here https://github.com/mikelangelo-project/capstan/tree/master/Godeps. But I'm not sure if Travis is really using GoDeps.

Anyway, if you're all on fire to help us fix this in separate PR I'd appreciate the effort, but we can merge even with red Travis when you provide the documentation, as long as you can confirm tests are all green in your local dev at least. I trust you know what you're doing based on our great experience in past.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mikelangelo-project/capstan/pull/101#issuecomment-421338656, or mute the thread https://github.com/notifications/unsubscribe-auth/AFDSIXHC1PjzDm5vE_o5qQ8Gc5-NsIKvks5ua5wygaJpZM4WoZzI .

wkozaczuk commented 6 years ago

I ran the tests as you requested. All looks good.

I also pushed 2 new commits to my branch with the updates to the documentation.

Still no solution for the YAML issue.

Waldek

On Fri, Sep 14, 2018 at 8:55 AM Waldek Kozaczuk jwkozaczuk@gmail.com wrote:

I will try to come up with some workaround for this YAML issue.

On Fri, Sep 14, 2018 at 8:08 AM Miha Pleško notifications@github.com wrote:

Thanks @wkozaczuk https://github.com/wkozaczuk for looking at the problem. It seems like my fault because I monkey-patched datetime marshalling https://github.com/mikelangelo-project/capstan/blob/master/core/yaml.go#L16, that's why it's failing now with newer version of the library. I agree that we can lock dependencies, and I think it's somehow possible by using GoDeps, our current setup is here https://github.com/mikelangelo-project/capstan/tree/master/Godeps. But I'm not sure if Travis is really using GoDeps.

Anyway, if you're all on fire to help us fix this in separate PR I'd appreciate the effort, but we can merge even with red Travis when you provide the documentation, as long as you can confirm tests are all green in your local dev at least. I trust you know what you're doing based on our great experience in past.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mikelangelo-project/capstan/pull/101#issuecomment-421338656, or mute the thread https://github.com/notifications/unsubscribe-auth/AFDSIXHC1PjzDm5vE_o5qQ8Gc5-NsIKvks5ua5wygaJpZM4WoZzI .

miha-plesko commented 6 years ago

@wkozaczuk perfect docs. Neat patch. No reason to let this PR sit unmerged, thank you!

wkozaczuk commented 6 years ago

Thanks @Miha Pleško miha.plesko@xlab.si

On Fri, Sep 14, 2018 at 2:26 PM Miha Pleško notifications@github.com wrote:

Merged #101 https://github.com/mikelangelo-project/capstan/pull/101 into master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mikelangelo-project/capstan/pull/101#event-1846758027, or mute the thread https://github.com/notifications/unsubscribe-auth/AFDSIR3SrblzplJZechnL1WFs022OHXHks5ua_TvgaJpZM4WoZzI .

wkozaczuk commented 6 years ago

I have just created new PR to fix that timestamp YAML issue. It is quite ugly but I could find a better proper way. I think it is a bug in yaml.v2 library.

On Fri, Sep 14, 2018 at 2:39 PM Waldek Kozaczuk jwkozaczuk@gmail.com wrote:

Thanks @Miha Pleško miha.plesko@xlab.si

On Fri, Sep 14, 2018 at 2:26 PM Miha Pleško notifications@github.com wrote:

Merged #101 https://github.com/mikelangelo-project/capstan/pull/101 into master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mikelangelo-project/capstan/pull/101#event-1846758027, or mute the thread https://github.com/notifications/unsubscribe-auth/AFDSIR3SrblzplJZechnL1WFs022OHXHks5ua_TvgaJpZM4WoZzI .