-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
save device type instead of caching it #2023
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have few comments. but also i personally don't deep nesting like that, so the code might get some re-organized and or moved to separate functions.
for example
typ, err := s.diskType(device)
// where we have
func diskType(device) {
//get type
if found {
return
}
cached, found := cache.get()
if !found {
cached = detected()
}
// store on device
return
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't merge unless code is fully tested
@@ -68,10 +70,38 @@ func (i *DeviceInfo) Used() bool { | |||
return len(i.Label) != 0 || len(i.Filesystem) != 0 | |||
} | |||
|
|||
func (d *DeviceInfo) Type() (zos.DeviceType, error) { | |||
// DetectType returns the device type according to seektime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First time for me to know about the seektime tool, maybe we can enhance it to include that it's an approximation too https://github.com/maxux/seektime not 100% correct
pkg/storage/filesystem/device.go
Outdated
return d.mgr.Seektime(context.Background(), d.Path) | ||
} | ||
|
||
// SetType sets the device type to the disk | ||
func (d *DeviceInfo) SetType(typ pkg.DeviceType) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use zos.DeviceType? I'm not sure why we created that alias declaration DeviceType = DeviceType?
Also the rest of the functions DetectType, Type are working with zos.DeviceType
pkg/storage/filesystem/device.go
Outdated
// SetType sets the device type to the disk | ||
func (d *DeviceInfo) SetType(typ pkg.DeviceType) error { | ||
if err := os.WriteFile(filepath.Join("/mnt", d.Name(), ".seektime"), []byte(typ), 0644); err != nil { | ||
return errors.Wrapf(err, "failed to store device type for '%s'", d.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please include the full path as well so /mnt/d.Name(), ...
pkg/storage/filesystem/device.go
Outdated
return d.mgr.Seektime(context.Background(), d.Path) | ||
} | ||
|
||
// SetType sets the device type to the disk | ||
func (d *DeviceInfo) SetType(typ pkg.DeviceType) error { | ||
if err := os.WriteFile(filepath.Join("/mnt", d.Name(), ".seektime"), []byte(typ), 0644); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's extract the first argument of WriteFile into a varilable to improve the readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I believe we shouldn't use .seektime
extension, as it denotes the results of seektime (rpm/seektime) instead of actual harddisk type string, however if it's considered as "the output of seektime" tool, I'm good with that
pkg/storage/filesystem/device.go
Outdated
return nil | ||
} | ||
|
||
// Type gets the device type from the disk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's expand the docstring, that's based on .seektime
file existing in the /mnt/DeviceName.seektime, that contains the device type being SSD or HDD maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also i'm not sure of the behavior of the return type device, bool, err
pkg/storage/filesystem/device.go
Outdated
|
||
// Type gets the device type from the disk | ||
func (d *DeviceInfo) Type() (zos.DeviceType, bool, error) { | ||
data, err := os.ReadFile(filepath.Join("/mnt", d.Name(), ".seektime")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- let's extract to its own variable
- let's rename data to disktype
I noticed this in the seektime code maxux/seektime#2 , not sure how it affects the accuracy or the calculations @maxux @muhamadazmy ? |
Regarding maxux/seektime#2, this doesn't affect any accuracy in our case. But this threefoldtech/seektime#3 was ready for a long time now and were never merged, but this could provide a way better help to troubleshoot in case of issue. Does merging this PR would affect something in zos ? The json reply would change, maybe you need to support it ? This pull requests will print |
@maxux current seektime version in zos return this
if the format is going to change then zos also need to have some modification to support his. Right now it seems that seektime is only provided by the base image. so after merging of your branch, and if zos image is rebuilt, then zos will break. Instead what we need to do is provide a runtime package update for seektime so both the seektime version and the code to handle it to be installed in one go. Can you please provide a Once this build is available, an flist can be built for seektime that can be included in zos |
Issues