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

ProblemEnvironmentをリソースを元にschedulerする #46

Merged
merged 7 commits into from
Nov 21, 2022

Conversation

motacapla
Copy link
Contributor

@motacapla motacapla commented Nov 11, 2022

#42

互いの値には相関があるので、CPU/メモリ使用率の合計値の昇順に選ぶようにした

@motacapla motacapla changed the title [WIP]ProblemEnvironmentをリソースを元にschedulerする ProblemEnvironmentをリソースを元にschedulerする Nov 16, 2022
Comment on lines +125 to +126
MemoryUsedPercent: strconv.FormatFloat(virtualMemory.UsedPercent, 'f', -1, 64),
CPUUsedPercent: strconv.FormatFloat(cpuUsedPercents[0], 'f', -1, 64),
Copy link
Contributor Author

@motacapla motacapla Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@motacapla
Copy link
Contributor Author

まだバグってて動いてないのでWIP戻します

@motacapla motacapla changed the title ProblemEnvironmentをリソースを元にschedulerする [WIP] ProblemEnvironmentをリソースを元にschedulerする Nov 16, 2022
Comment on lines +31 to +32
MemoryUsedPercent string `json:"memoryUsedPercent"`
CPUUsedPercent string `json:"cpuUsedPercent"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stringじゃなくてfloatでもつのがいいかなと思います!

Copy link
Contributor Author

@motacapla motacapla Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここfloatだとコンパイルが通らなかったので、なくなくstringにしてました
後でもう一度試してみます!

warningですね

/home/motacapla/netcon-problem-management-subsystem/api/v1alpha1/worker_types.go:31:20: found float, the usage of which is highly discouraged, as support for them varies across languages. Please consider serializing your float as string instead. If you are really sure you want to use them, re-run with crd:allowDangerousTypes=true

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

調べてたら似たような議論が出てきましたね。
kubernetes-sigs/controller-tools#245

ななめ読みする感じだと、floatは環境によって精度が変わったりするからintの方がいいよって感じですかね

方針はお任せするので、stringでもintでもめう氏が良さそうな方でお願いします!


workerLength := len(workers.Items)
if workerLength < 2 {
// at least 1 worker must exist
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

基本的にはこの仮定が成り立つと思いますが、nil pointer dereferenceでクラッシュされるのは困るので、electWorkerから(string, error)を返すようにするか、workerLength == 0の時は空文字列を返すようにして、electWorkerの呼び出し元で適切にエラーハンドリングしてくれる方が嬉しいです!

Comment on lines 121 to 147
type WorkerResource struct {
Name string
CPUUsedPercent float64
MemoryUsedPercent float64
SumOfResourcesUsedPercent float64
}
arr := make([]WorkerResource, 0, workerLength)
for i := 0; i < workerLength; i++ {
cpuUsedPct, err := strconv.ParseFloat(workers.Items[i].Status.WorkerInfo.CPUUsedPercent, 64)
if err != nil {
log.Error(err, "failed to parse CPUUsedPercent for worker election")
cpuUsedPct = MAX_USED_PERCENT
}
memoryUsedPercent, err := strconv.ParseFloat(workers.Items[i].Status.WorkerInfo.MemoryUsedPercent, 64)
if err != nil {
log.Error(err, "failed to parse CPUUsedPercent for worker election")
memoryUsedPercent = MAX_USED_PERCENT
}
sumOfResourcesUsedPercent := cpuUsedPct + memoryUsedPercent

arr = append(arr, WorkerResource{workers.Items[i].Name, cpuUsedPct, memoryUsedPercent, sumOfResourcesUsedPercent})
}
sort.Slice(workers, func(i, j int) bool {
return arr[i].SumOfResourcesUsedPercent < arr[j].SumOfResourcesUsedPercent
})
return arr[0].Name
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@motacapla motacapla changed the title [WIP] ProblemEnvironmentをリソースを元にschedulerする ProblemEnvironmentをリソースを元にschedulerする Nov 16, 2022
Comment on lines +203 to +204
message := "failed to elect worker for scheduling"
log.Info(message)
Copy link
Contributor Author

@motacapla motacapla Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

エラー時は特に何も更新せずに終えようと思ってましたが、この方針で良いですか?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scheduledっていうstatusがあったような気がする?ので、それをFalseに更新して、reasonに「workerが無かったよ」的なコメントを入れておけばいい気がします!(そのまま更新しないで抜けてもしばらくするとresync期間後に再Reconcileが走るはずなのでそれで問題ないと思います!)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue切って別のPRで回収して全然大丈夫です!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そのまま更新しないで抜けてもしばらくするとresync期間後に再Reconcileが走るはずなのでそれで問題ないと思います!

こっちを想定してました、下記対応のほうが丁寧なので、あとで直してみますね

Scheduledっていうstatusがあったような気がする?ので、それをFalseに更新して、reasonに「workerが無かったよ」的なコメントを入れておけばいい気がします!

Copy link
Collaborator

@proelbtn proelbtn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

基本的にはOKです!

Comment on lines +203 to +204
message := "failed to elect worker for scheduling"
log.Info(message)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scheduledっていうstatusがあったような気がする?ので、それをFalseに更新して、reasonに「workerが無かったよ」的なコメントを入れておけばいい気がします!(そのまま更新しないで抜けてもしばらくするとresync期間後に再Reconcileが走るはずなのでそれで問題ないと思います!)

@proelbtn proelbtn linked an issue Nov 19, 2022 that may be closed by this pull request
@motacapla motacapla merged commit 9199284 into master Nov 21, 2022
@motacapla motacapla deleted the meu/schedule-probenv-with-resources branch November 21, 2022 10:43
Comment on lines +203 to +204
// TODO: statusの Scheduled = Falseに更新する
// 更新せずとも、再reconcileが走るので問題はないはず
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#48

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.

ProblemEnvironmentをリソースを元にschedulerする
2 participants