Sort zmount workloads in descending order #2017
Conversation
AbdelrahmanElawady
requested review from
muhamadazmy,
delandtj,
MarioBassem,
ashraffouda,
rawdaGastan and
xmonader
as code owners
pkg/provision/engine.go
Outdated
| if err != nil { | ||
| return 0, err | ||
| } | ||
| data, ok := dataI.(*zos.ZMount) |
There was a problem hiding this comment.
at this point it should be called zmount instead of data
pkg/provision/engine.go
Outdated
| if !ok { | ||
| return 0, errors.New("not a zmount") | ||
| } | ||
| return data.Size, nil |
There was a problem hiding this comment.
zmount.Size after the rename
pkg/provision/engine.go
Outdated
| } | ||
| data, ok := dataI.(*zos.ZMount) | ||
| if !ok { | ||
| return 0, errors.New("not a zmount") |
There was a problem hiding this comment.
I would be curious to see the raw data included here in the error, also the message should get improved instead of not a zmount
pkg/provision/engine.go
Outdated
| log.Error().Err(err).Stringer("id", workloads[j].ID).Msg("failed to get zmount size") | ||
| } | ||
|
|
||
| return sizeI > sizeJ |
There was a problem hiding this comment.
How can we depend on sizeI and sizeJ while they're supposed to be the zero value returns of getZmountSize in case of errors? I believe each branch after the error check should return a bool (e.g false in the first case and true in the 2nd one)
There was a problem hiding this comment.
Also, maybe extract the code to sort ZMount workloads into its own function, so it could be easily tested
pkg/provision/engine.go
Outdated
| @@ -867,10 +868,44 @@ func (e *NativeEngine) uninstallDeployment(ctx context.Context, dl *gridtypes.De | |||
|
|
|||
| } | |||
|
|
|||
| func getZmountSize(wl *gridtypes.WorkloadWithID) (gridtypes.Unit, error) { | |||
There was a problem hiding this comment.
| func getZmountSize(wl *gridtypes.WorkloadWithID) (gridtypes.Unit, error) { | |
| func getZmountSize(wl *gridtypes.Workload) (gridtypes.Unit, error) { |
you don't need to pass the wrapper type WorloadWithID
pkg/provision/engine.go
Outdated
| @@ -867,10 +868,44 @@ func (e *NativeEngine) uninstallDeployment(ctx context.Context, dl *gridtypes.De | |||
|
|
|||
| } | |||
|
|
|||
| func getZmountSize(wl *gridtypes.WorkloadWithID) (gridtypes.Unit, error) { | |||
| dataI, err := wl.WorkloadData() | |||
There was a problem hiding this comment.
Don't use the I suffix, data is enough. It add no value to the variable name
| dataI, err := wl.WorkloadData() | |
| data, err := wl.WorkloadData() |
pkg/provision/engine.go
Outdated
| sort.Slice(workloads, func(i, j int) bool { | ||
| sizeI, err := getZmountSize(workloads[i]) | ||
| if err != nil { | ||
| log.Error().Err(err).Stringer("id", workloads[i].ID).Msg("failed to get zmount size") |
There was a problem hiding this comment.
i get why you log but i think it's not needed here. I won't show an error in the node logs because of wrong user data. This can confuse operators to see node giving errors that might make them think something wrong with the node itself while it's actual just user data that is wrong and that it was handled.
I would personally not log it.
pkg/provision/engine.go
Outdated
| sizeJ, err := getZmountSize(workloads[j]) | ||
| if err != nil { | ||
| log.Error().Err(err).Stringer("id", workloads[j].ID).Msg("failed to get zmount size") | ||
| return true |
There was a problem hiding this comment.
Why do u return true here but false in the first case ? just wondering. I think you should return false always in case if both elements are equal.
Sort zmount workloads so larger disks get assigned first.
Related: #2016