support for driver specific pid files #1171
Conversation
|
Info, This made a lot of sense as hostagent is the one starting and managing the driver. And store is not required to know about it. Even though it can. |
pkg/driver/driver.go
Outdated
| @@ -12,6 +12,8 @@ type Driver interface { | |||
|
|
|||
| CreateDisk() error | |||
|
|
|||
| GetPIDName() string | |||
There was a problem hiding this comment.
| GetPIDName() string | |
| Name() string |
The pid file name can be just Name() + ".pid"
There was a problem hiding this comment.
Ah nice !! This looks more clean.
Done the changes
pkg/qemu/qemu.go
Outdated
| @@ -31,6 +31,7 @@ type Config struct { | |||
| Name string | |||
| InstanceDir string | |||
| LimaYAML *limayaml.LimaYAML | |||
| Driver *LimaQemuDriver | |||
There was a problem hiding this comment.
With this change the whole Config struct becomes redundant because all other fields are just copies from the Driver struct.
Would it make sense to get rid of Config and just pass around Driver instead?
There was a problem hiding this comment.
Sure, will adapt this change as well.
I was in a confusion of whether to do / not as we will update all those config stuff :)
There was a problem hiding this comment.
I have not properly thought this through; it just occurred to me that there was no additional information in Config beyond what is already in Driver. In general I would try to avoid keeping the same data in multiple places because then it can get out of sync, and hard to debug...
Maybe wait for feedback from @AkihiroSuda before making this change?
There was a problem hiding this comment.
If this Driver addition is just for getting the driver name, can we just get the driver name from the yaml?
There was a problem hiding this comment.
We could, but what is the reason for having a separate Config struct? The base Driver is just pointers to instance data and YAML:
type BaseDriver struct {
Instance *store.Instance
Yaml *limayaml.LimaYAML
}
pkg/hostagent/api/api.go
Outdated
| @@ -1,5 +1,6 @@ | |||
| package api | |||
|
|
|||
| type Info struct { | |||
| SSHLocalPort int `json:"sshLocalPort,omitempty"` | |||
| SSHLocalPort int `json:"sshLocalPort,omitempty"` | |||
| DriverName string `json:"driverName,omitempty"` | |||
There was a problem hiding this comment.
- Can we eliminate this from the
Info? Just like CPU nums, arch, memory, etc. - If we really can't remove this, this should be named like
vmTypeorvmDriverName, as we also have a bunch of other "drivers" such as mount driver, network driver, etc. - We can also consider marshaling the entire YAML as a JSON here if we are going to copy other YAML properties too here
There was a problem hiding this comment.
- As drivers are already relying on store, the moment we rely on driver it will have this cyclic import problem. We can look into resolving it as well.
- If am not able to resolve the above issue i will look into moving other properties to rely on this info endpoint itself by YAML to JSON conversion in info endopoint, so that store will simply depend on hostagent info and not assume anything directly from configuration
There was a problem hiding this comment.
@AkihiroSuda
One way i could think of solving it is, our pid names can always be equal to vmType. This way, in instance.Inspect() we can simply rely on vmType to get according pid.
But we could also argue that with this assumption we don't even need Name() in driver interface :)
Let me know if this is fine. In that case, i will maybe get rid of Name itself fully from driver and go with vmType itself.
Signed-off-by: Balaji Vijayakumar <kuttibalaji.v6@gmail.com>
This PR resolves this discussion
#1147 (comment)