openmainframeproject / feilong

Feilong is a open source z/VM cloud connector project under the Open Mainframe Project umbrella that will accelerate the z/VM adoption, extending its ecosystem and its user experience. It provides a set of APIs to operate z/VM including guest, image, network, volume etc.
https://www.openmainframeproject.org/projects/feilong
Apache License 2.0
35 stars 70 forks source link

How to transport multiple files with DeployGuest()? #769

Closed Bischoff closed 8 months ago

Bischoff commented 10 months ago

It might be useful to use both a cloud-init parameters disk and a network.doscript script to initialize a guest.

In that case, the only way I have found to do that is to specify "transportfiles:" twice in the json used by DeployGuest.

However, RFC 8259 discourages the use of duplicate keys in a json file. As a matter of fact, several json serializers / deserializers cannot cope with them.

Did I miss something? Or should the value of transportfiles be an array of strings rather than a single string?

jichenjc commented 8 months ago

Or should the value of transportfiles be an array of strings rather than a single string?

is there is requirement, I think we can add support for this

but it might break the scenario that we supported before (the transportfiles from string to array) so should we consider a new param otherwise it will be uncompatible change?

Bischoff commented 8 months ago

is there is requirement, I think we can add support for this

but it might break the scenario that we supported before (the transportfiles from string to array) so should we consider a new param otherwise it will be uncompatible change?

I am not 100 % sure this is a requirement. I am able to use only the cloud-init parameters disk and forget about the network parameters. What is sure is that, if you need both, you currently have to use dubious JSON syntax. But I am not sure there are use cases where you need both. That would need to be clarified.

Yes, an array of strings would break compatibility - unless you allow both string and array of strings. Notice that "transportfiles:" is plural. What about allowing something like:

"transportfiles": "file1,file2"

i.e. a comma-separated list of files? It would still be a single string. Not really elegant though.

jichenjc commented 8 months ago

"transportfiles": "file1,file2"

this might be good thing to try and make sense to me (conceptually) but this might need another contract to distinguish disk and network param per your below use case..

disk and forget about the network parameters.>

Bischoff commented 8 months ago

"transportfiles": "file1,file2"

this might be good thing to try and make sense to me (conceptually) but this might need another contract to distinguish disk and network param per your below use case..

I was under the impression Feilong was acting based on the file extension (.iso for the cloud-init parameters disk, and .doscript for the network parameters). But it is only an educated guess. That would still work with a comma-separated list:

"transportfiles": "cloud-init.iso,network.doscript"

But I dislike the approach - why use a string with a separator inside the string, when JSON has provisions for list of strings?

How hard would it be to allow a string OR a string list when parsing?

EDIT: I just noticed that we already have a similar case:

7.5.13. Get Guests interface stats

userid path string A string that contains single userid or userids delimited by comma, like id1, id2, id3
jichenjc commented 8 months ago

ISO file

yes, we have 2 phase init (due to various tech reason) phase 1 create ISO from input to phase 2, this is in early phase of VM startup (we call it zvmguestconfig) https://github.com/openmainframeproject/feilong/blob/master/tools/share/zvmguestconfigure#L24 phase 2 give the ISO to cloud-init as Active engine input (cloud-init start in later phase of Linux bootup, so we have to make 2 phase)

the original developer left the team so in my limit memory on this is those scripts is the one to do the init in zvmguest config https://github.com/openmainframeproject/feilong/blob/master/tools/share/zvmguestconfigure#L134

Bischoff commented 8 months ago

Hmmm, according to latest link, it can even be three files, not 2:

#   Files with a file type of:
  #     iso that can be used to setup iso9660 loop device that will be consumed by cloud-init
  #     sh that can be used to execute shell script
  #     doscript that contains a invokeScript.sh which will call the other script in it to do the speicial work

I still do not know for sure what the "official" way to pass several of them is. Having several "transportfiles": entries in the JSON works, but I don't know if it was planned that way.

I would be curious to know what OpenStack sends to Feilong... After all, the OpenStack integration must have the same problem.

jichenjc commented 8 months ago

I would be curious to know what OpenStack sends to Feilong... After all, the OpenStack integration must have the same problem.

nothing special :) ,just call the REST API provided by feilong https://github.com/openstack/nova/blob/master/nova/virt/zvm/hypervisor.py#L91

Bischoff commented 8 months ago

nothing special :) ,just call the REST API provided by feilong https://github.com/openstack/nova/blob/master/nova/virt/zvm/hypervisor.py#L91

Thanks for the pointer (we're speaking of guest_deploy(), not guest_create()).

Question is: does OpenStack pass only one file, and is fine with it? What is the value of transportfiles= parameter at run time? Or could we even intercept the value of the JSON that is sent?

jichenjc commented 8 months ago

does OpenStack pass only one file, and is fine with it

it's been a few years I checked the code and hopefully my understanding still correct, seems I think openstack pass 1 file only (it's not openstack standard, it's what zvm driver implemented ,which also done by our team) https://github.com/openstack/nova/blob/master/nova/virt/zvm/driver.py#L165 https://github.com/openstack/nova/blob/master/nova/virt/zvm/utils.py#L95

so it's a config drive ISO file location and used directly by feilong from openstack side (as they are same Linux)

Bischoff commented 8 months ago

it's been a few years I checked the code and hopefully my understanding still correct, seems I think openstack pass 1 file only (it's not openstack standard, it's what zvm driver implemented ,which also done by our team) https://github.com/openstack/nova/blob/master/nova/virt/zvm/driver.py#L165 https://github.com/openstack/nova/blob/master/nova/virt/zvm/utils.py#L95

so it's a config drive ISO file location and used directly by feilong from openstack side (as they are same Linux)

In my case, the terraform provider accepts both the .iso and the .doscript files:

https://github.com/Bischoff/terraform-provider-feilong/blob/8a69efe9306895094f1d97039e7efb0d9d6b2cbf/provider/feilong_guest.go#L183

and there is a hack in the Go library to accept 2 files separated by a comma:

https://github.com/Bischoff/feilong-client-go/blob/650af0e749651471ebf043e8f14e191ff64f08e3/guests.go#L364

but, in practice, I always only use the iso file:

https://github.com/uyuni-project/sumaform/blob/a7f4851e4b408d2548e75b0e69230dc40b1dd89a/backend_modules/feilong/host/main.tf#L36

So, it seems that transportfiles, despite being plural, is always called with only one file.

What do we do?

I am a big YAGNI fan, so I am ready to simplify my code, but it will reveal stupid if someday we accept several files. Also, the Feilong doc is misleading:

What is your suggestion?

jichenjc commented 8 months ago

I will be off for a couple of days @bjhuangr @SeanHQF could you please help check above questions? Thanks

Bischoff commented 8 months ago

In the meantime, I removed all my code to support the network script files.

I can always revert that if Feilong starts supporting several transport files in transportfiles field.

Bischoff commented 8 months ago

@jichenjc that is not really a "solution", but the current code works if your use case does not require you to transmit 2 files or more.

Just cloes the issue?

jichenjc commented 8 months ago

ok, feel free to close this issue, so current logic is good enough (and I agree need some polishment on the doc etc) for feilong

and I agree with your YAGNI suggestion, as mentioned this code has been there for a while so unless we need additional change for more use case we may let it be

Bischoff commented 8 months ago

Not solved, but closing as discussed. Good enough for now.