Kubevirt: cdiupload retry, thin metrics #4509
Conversation
github-actions
bot
requested review from
deitch,
eriknordmark,
OhmSpectator,
rene,
rouming,
rucoder and
shjala
andrewd-zededa
force-pushed
the
kubevirt-volumemgr-cdi
branch
2 times, most recently
from
43665e9 to
7c6799c
Compare
|
With the rebase I think this PR is approaching ready for review. I'll wait for the latest CI runs. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4509 +/- ##
=======================================
Coverage 20.90% 20.90%
=======================================
Files 13 13
Lines 2894 2894
=======================================
Hits 605 605
Misses 2163 2163
Partials 126 126 ☔ View full report in Codecov by Sentry. |
|
|
||
| // waitForPVCUploadComplete: Loop until PVC upload annotations show upload complete | ||
| func waitForPVCUploadComplete(pvcName string, log *base.LogObject) error { | ||
| clientset, err := GetClientSet() |
There was a problem hiding this comment.
From where might we call this function which can take 500 seconds? Does it need to kick the watchdog?
There was a problem hiding this comment.
This is called in kubeapi/vitoapiserver.go RolloutDiskToPVC() which runs in the volume create worker context. I do see now that volumemgr calls a log.Fatal if AddWorkCreate -> TrySubmit returns queue full which seems to be set to a constant 20 right now.
Looks like the watchdog might not need to be kicked but if we get a burst of 20 queued up volume creates behind a slow one then the log.Fatal will take down pillar anyways.
There was a problem hiding this comment.
I see one issue in that waitForPVCUploadComplete is not using the caller's context which can cancel if the volume create is cancelled. I'll submit a change to handle that.
There was a problem hiding this comment.
Its in the latest push
There was a problem hiding this comment.
Thanks. If it runs from the volume create handler then we don't need to worry about a watchdog.
But it makes sense stating that in a comment.
There was a problem hiding this comment.
just added a comment over kubeapi.RolloutDiskToPVC and kubeapi.waitForPVCUploadComplete
andrewd-zededa
force-pushed
the
kubevirt-volumemgr-cdi
branch
from
7c6799c to
ee065e7
Compare
andrewd-zededa
force-pushed
the
kubevirt-volumemgr-cdi
branch
from
ee065e7 to
326e700
Compare
|
Realized it wasn't marked ready for review, and we're already reviewing anyways so I changed it. |
pkg/pillar/diskmetrics/usage.go
Outdated
| @@ -29,7 +29,8 @@ func StatAllocatedBytes(path string) (uint64, error) { | |||
| if err != nil { | |||
| return uint64(0), err | |||
| } | |||
| return uint64(stat.Blocks * int64(stat.Blksize)), nil | |||
| // stat.Blocks is always 512-byte blocks | |||
There was a problem hiding this comment.
Indeed, although POSIX says is implementation defined. Here, stat.Blksize should be usually 4Kb, which means this function was reporting 8x bigger the true allocated size... this is a bug, I would separate this fix into a dedicated commit (or even a PR)....
There was a problem hiding this comment.
I can split this to another PR, as you noted POSIX does define it as implementation defined. I missed that musl's <sys/param.h> does define it under DEV_BSIZE. I could move this to another PR and pull that in with cgo.
There was a problem hiding this comment.
Is it 512 even on zfs? Does this 8x overreporting show up in the UI?
There was a problem hiding this comment.
removed this change for now, will submit in another PR shortly
pkg/pillar/hypervisor/kubevirt.go
Outdated
| @@ -128,10 +128,17 @@ func (metrics *kubevirtMetrics) fill(domainName, metricName string, value interf | |||
| cpuNs := assignToInt64(value) * int64(time.Second) | |||
| r.CPUTotalNs = r.CPUTotalNs + uint64(cpuNs) | |||
| case "kubevirt_vmi_memory_usable_bytes": | |||
| // The amount of memory which can be reclaimed by balloon without pushing the guest system to swap, | |||
| // corresponds to ‘Available’ in /proc/meminfo | |||
| // https://kubevirt.io/monitoring/metrics.html#kubevirt | |||
| r.AvailableMemory = uint32(assignToInt64(value)) / BytesInMegabyte | |||
There was a problem hiding this comment.
I'm just curious here: metrics are called ..._bytes although the return value it's in MB.... is this expected?
Also, is uint32 enough? That limits the maximum value to 4GB....
There was a problem hiding this comment.
Yes the uint32 downcast here before the division is incorrect, fix in the next push.
There was a problem hiding this comment.
resolved in latest push
andrewd-zededa
force-pushed
the
kubevirt-volumemgr-cdi
branch
from
326e700 to
dfeb823
Compare
- Implement retries of CDI upload to pvc to handle intermittent timeouts to k8s api. - Switch kubevirt memory metric as "_total" suffix was removed from "kubevirt_vmi_memory_domain_bytes" - Create DiskMetric with names for sdX and pvc name to help match them in system debug. - Fix AppDiskMetric thin allocation reporting by moving csihandler GetVolumeDetails to reading allocated space from lh volume object and Populate() filling in FileLocation. - Use thin allocation check when dir has prefix of types.VolumeEncryptedDirName or types.VolumeClearDirName to handle subdirs like kubevirt /persist/vault/volumes/replicas/... - waitForPVCUploadComplete needs to use caller's context to allow quicker exit if volume create is cancelled Signed-off-by: Andrew Durbin <andrewd@zededa.com>
andrewd-zededa
force-pushed
the
kubevirt-volumemgr-cdi
branch
from
dfeb823 to
1502185
Compare
|
Another push for missing comments. |
intermittent timeouts to k8s api.
was removed from "kubevirt_vmi_memory_domain_bytes"
match them in system debug.
csihandler GetVolumeDetails to reading allocated space
from lh volume object and Populate() filling in FileLocation.