[local-volume] Refactored GetFsAvailableByte to avoid using Frsize in local-volume provisioner #226
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
ianchakeres
changed the title
| func (u *volumeUtil) GetFsAvailableByte(fullPath string) (uint64, error) { | ||
| var s unix.Statfs_t | ||
| if err := unix.Statfs(fullPath, &s); err != nil { | ||
| //largely copied from kubernetes/pkg/volume/util/fs.go:FsInfo(path) |
There was a problem hiding this comment.
Do we get vendoring/import issues if we call it directly?
There was a problem hiding this comment.
I had some vendoring issues related to log_dir being defined twice, by glog being imported twice.
For example,
panic: /var/folders/59/yc7_f4fd53nbyw868zqpk78cn7shx9/T/go-build393895106/github.com/kubernetes-incubator/external-storage/local-volume/provisioner/pkg/discovery/_test/discovery.test flag redefined: log_dir
I was able to resolve it by locally executing
glide install --strip-vendor --strip-vcs
by following the instructions here on stack overflow.
I haven't dealt with vendoring before.
Do you know where I'd add the appropriate flags to glide to ensure that dependencies are flattened?
There was a problem hiding this comment.
@wongma7 can you help Ian import this K8s library
There was a problem hiding this comment.
we most likely can't (yet) because it imports "k8s.io/kubernetes/pkg/api."
but yes --strip-vendor/-v is the correct flag needed to remove nested dependencies & to flatten everything
There was a problem hiding this comment.
Can you explain the limitation?
There was a problem hiding this comment.
There will only be issues if we use functions from kubernetes/kubernetes/pkg/volume/util/ that accept or return kubernetes/kubernetes/pkg/api types, because we want to work with downstream k8s.io/api types instead.
It seems Fsinfo doesn't have this issue, so we won't run into any compilation errors when vendoring kubernetes/kubernetes/pkg/volume/util/ & using it. But generally we want to solve this issue for long-term so we can make use of existing code #165
There was a problem hiding this comment.
So here's the steps i would do:
import kubernetes/pkg/volume/util
call util.Fsinfo where GetFsAvailableByte was called before
run glide up -v
run glide-vc --use-lock-file
There was a problem hiding this comment.
@wongma7 - thanks for the advice. I was able to execute the commands.
PTAL and let me know if the additional files in this commit look good.
ianchakeres
force-pushed
the
replace-frsize
branch
from
309229a to
28d7182
Compare
|
This is a lot of files to pull into vendor... Should we try moving FsInfo to a higher level package so we can pull in from client-go instead? |
|
What I got is it won't be accepted if we try to put the code into client-go when the code will need to be moved to k8s.io/pkg anyway. So we are left with either vendoring huge amounts of files or copying code as-needed. Either case would be only temporary, ideally. TBH I would prefer we copy code as needed, we are all under the same k8s org&license after all, and it's not like we'll need to manually update the copied code of simple util functions frequently if at all. And FWIW, from an initial attempt, refactoring pkg/volume/util is near impossible anyway atm, it is heavily tied to pkg/volume and idk how it will be made consumable for k8s.io/pkg or anywhere... |
|
Ah sorry, I instead of client-go, I meant putting it into k8s.io/pkg. Instead of pulling all of pkg/volume/util, can we just pull out the pieces we need for now? |
ianchakeres
force-pushed
the
replace-frsize
branch
2 times, most recently
from
99ca77e to
db975cf
Compare
|
I'm okay with current direction, don't feel strongly either way. When k8s.io/pkg becomes a thing we can simply change a few import lines, run glide again, and all should be well. And I'd hate for your glide troubles to have been for nothing :) |
|
Ok sounds good, let's continue with the vendoring direction. |
ianchakeres
force-pushed
the
replace-frsize
branch
from
73912f8 to
b7e9887
Compare
GetFsAvailableByte now uses the same method as kubernetes/pkg/volume/util/fs.go:FsInfo(path) to calculate available bytes
ianchakeres
force-pushed
the
replace-frsize
branch
from
b7e9887 to
b76fcc1
Compare
|
/lgtm @wongma7 can you check out the vendoring changes? |
|
LGTM 👍 |
…size [local-volume] Refactored GetFsAvailableByte to avoid using Frsize in local-volume provisioner
update owners
GetFsAvailableByte now uses the same method as kubernetes/pkg/volume/util/fs.go:FsInfo(path) to calculate available bytes
Fixes: #224
Related to this discussion: #132
After this PR is merged, mac osx (darwin) uses can successfully execute make test on the local-volume provisioner.