Skip to content
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

Merged
merged 9 commits into from
Aug 21, 2023
Merged

Conversation

rawdaGastan
Copy link
Contributor

Copy link
Member

@muhamadazmy muhamadazmy left a 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
}
 

pkg/storage/filesystem/device.go Outdated Show resolved Hide resolved
pkg/storage/storage.go Outdated Show resolved Hide resolved
pkg/storage/filesystem/device.go Outdated Show resolved Hide resolved
pkg/storage/storage.go Outdated Show resolved Hide resolved
pkg/storage/storage.go Outdated Show resolved Hide resolved
pkg/storage/storage.go Show resolved Hide resolved
muhamadazmy
muhamadazmy previously approved these changes Aug 14, 2023
Copy link
Member

@muhamadazmy muhamadazmy left a 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
Copy link
Collaborator

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

return d.mgr.Seektime(context.Background(), d.Path)
}

// SetType sets the device type to the disk
func (d *DeviceInfo) SetType(typ pkg.DeviceType) error {
Copy link
Collaborator

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

// 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())
Copy link
Collaborator

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(), ...

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 {
Copy link
Collaborator

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

Copy link
Collaborator

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

return nil
}

// Type gets the device type from the disk
Copy link
Collaborator

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

Copy link
Collaborator

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


// 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"))
Copy link
Collaborator

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

@xmonader
Copy link
Collaborator

I noticed this in the seektime code maxux/seektime#2 , not sure how it affects the accuracy or the calculations @maxux @muhamadazmy ?

@maxux
Copy link
Contributor

maxux commented Aug 14, 2023

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 min and max seektime, this helped me previously to diagnostic a bug where HDD were sleeping and disks needed to be woke up on the first check; max was set to few of seconds and min were real value, this helped to understand what happened. This should be merged IMO. This can aswell add some precision on the check and is always helpful.

@muhamadazmy
Copy link
Member

@maxux current seektime version in zos return this

seektime -j /dev/vda
{"device": "/dev/vda", "type": "HDD", "elapsed": 564}

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 seektime pre-build in the seektime release. A static build is the best or in worst case make sure it uses the same libc version of the base zos image.

Once this build is available, an flist can be built for seektime that can be included in zos

@muhamadazmy muhamadazmy merged commit d96dff4 into main Aug 21, 2023
@muhamadazmy muhamadazmy deleted the development_reboot_disks_fix branch August 21, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants