stage1/init/common: GenerateMounts refactoring #3589
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
|
note to self: this embeds a copy of #3590, see tracking in there. |
|
Sorry for the delay, we have been traveling a bit recently and still going through backlog. I'll try to have a review pass on this soon, in the meanwhile I think the test change (in rkt_non_root_test.go) is superfluous now. |
|
@lucab, thank you, I will rebase a commit with the latest changes in the master branch. |
|
Not sure if the failures in the |
stage1/init/common/mount.go
Outdated
|
|
||
| // Map of hostpath -> Mount | ||
| mnts := make(map[string]schema.Mount) | ||
| // mountsMap is a mapping of mount point path to mount point details. |
There was a problem hiding this comment.
Minor doc issue: this is a mapping from mount point name, right?
There was a problem hiding this comment.
The name in this context means a file-system path, like /mnt/dir1, this is why I decided to make it clear and call it a "path". If the "mount point name" is preferable I will change it.
There was a problem hiding this comment.
Ah, I see. the fact that it's a types.ACName is a bit confusing, as UNIX paths can have more characters than an ACName.
There was a problem hiding this comment.
@squeed, sorry, it was my incorrect understanding, I looked closely at the manifest specification, and this is really a name of the mount point, not a path:
"mountPoints": [
{
"name": "work",
"path": "/var/lib/work",
"readOnly": false
}
]Source: https://github.com/appc/spec/blob/master/spec/aci.md I will update the comments accordingly.
|
I think this PR is mostly fine from my point of view, but I'll wait for @squeed to come back for the final review. |
stage1/init/common/mount.go
Outdated
|
|
||
| // processed defines a set of processed mount points, so we don't | ||
| // have to walk though them multiple times. | ||
| processed map[string]struct{} |
There was a problem hiding this comment.
This seems to indicate that RuntimeMounts is essentially only iterable once.
That makes me a bit uncomfortable - this sort of subtle side-effecting behaviour - I could easily see another developer calling MountsFunc() more than once.
Either RuntimeMounts should error / panic if called twice, or should be non-side-effecting.
There was a problem hiding this comment.
The idea was to decouple the implementation into multiple small functions, but it resulted in a need to share the state between them. Thereby I moved this state into a structure, but now it really makes more sense to define a separate type to bypass that state, in order to get rid of undesirable side-effect.
|
Can you add some documentation to the It does make me uncomfortable to have so hidden state - the "processed" array is a bit "spooky". Would you mind documenting its use as a shortcut? |
|
Sure, I will update the documentation with more details. |
|
I've moved the state to a separate type Additionally I've renamed functions |
|
Can one of the admins verify this patch? |
|
For some reason |
|
Can one of the admins verify this patch? |
|
Rebasing on top of current master (ie. after #3670) would probably help on the testing side. |
|
@lucab, thank you! I'll give it a try. |
stage1/init/common/mount.go
Outdated
| UID: &defaultUID, | ||
| GID: &defaultGID, | ||
| } | ||
| log.Printf("warning: no volume specified for mount point "+ |
There was a problem hiding this comment.
I prefer not breaking long loglines to make sure they remain greppable.
There was a problem hiding this comment.
I've squashed comment into a single line.
This patch provides a refactoring of the GenerateMounts function from the "stage1/init/common" package in order to reduce the amount of the loops over the mount points. The implementation replaces the old function "GenerateMounts" and ships a new type called "RuntimeMounts" that provides "Mounts" to reproduce previous behavior and a new function "MountsFunc" that calls a user-defined function for each mount point.
|
It seems fine overall. @squeed can you please have a final pass on this? |
| break | ||
| } | ||
| } | ||
| mpoint, _ := s.mountPointOf(m.Volume) |
There was a problem hiding this comment.
What happens if mpoint is nil?
There was a problem hiding this comment.
This function returns instance of types.MountPoint, which is a structure and default value for it is an empty structure. Therefore, when mount point was not found, it returns types.MountPoint{}, where ReadOnly attribute is set to false - a default value for boolean type.
The answer is mpoint is never going to be equal to nil.
|
Hi, do I need to perform any additional changes or to explain unclear implementation? The review got stuck for a week, apologies for being too noisy. |
This patch provides a refactoring of the GenerateMounts function from
the "stage1/init/common" package in order to reduce the amount of the
loops over the mount points.
The implementation replaces the old function "GenerateMounts" and ships
a new type called "RuntimeMounts" that provides "Mounts" to reproduce
previous behavior and a new function "MountsFunc" that calls a
user-defined function for each mount point.
closes #2380