Detect and report an ErrorPvcNotFound/ErrorDataVolumeNotFound VM status #6171
Conversation
kubevirt-bot
added
release-note
zcahana
force-pushed
the
pvc_not_found
branch
2 times, most recently
from
ca391f5
to
c14ba69
Compare
|
/retest |
kubevirt-bot
added
lgtm
kubevirt-bot
removed
lgtm
|
/retest |
kubevirt-bot
added
the
needs-rebase
kubevirt-bot
removed
the
needs-rebase
|
/retest |
1 similar comment
|
/retest |
|
@jean-edouard can you give this another look when you have some time? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jean-edouard The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
Thanks @jean-edouard! Can you also /lgtm this, or do you prefer someone else to take a look too? |
Let's have someone else LGTM, in case I missed something. |
|
/cc |
| @@ -134,7 +134,27 @@ type templateService struct { | |||
| launcherSubGid int64 | |||
| } | |||
|
|
|||
| type PvcNotFoundError error | |||
| type PvcNotFoundError struct { | |||
| Err error | |||
There was a problem hiding this comment.
This seems to be a slightly weird nesting. A error is a type which implements Error() string. So you could just have a reason string field for instance. Did you do that so that you could use fmt.Errof? Maybe use fmt.Sprintf?
There was a problem hiding this comment.
Maybe you just want a nice constructor for the error? Something like NewPVCNotFoundError()?
There was a problem hiding this comment.
TBH I don't remember what was my rationale at the time when writing it like this (possible some partial inspiration from the Go 1.13 error handling pattern). Anyway I too don't see a good reason for it - will change to a reason string.
| @@ -1613,19 +1620,30 @@ func (c *VMIController) volumeReadyToAttachToNode(namespace string, volume virtv | |||
| } else if volume.PersistentVolumeClaim != nil { | |||
| name = volume.PersistentVolumeClaim.ClaimName | |||
| } | |||
|
|
|||
| dataVolumeFunc := dataVolumeByNameFunc(c.dataVolumeInformer, dataVolumes) | |||
There was a problem hiding this comment.
There is one other check if the volume is populated for hotplug. Is that covered too?
It may make sense to get rid of this callback function and now, that we have to do pre-validation to see if the DV exist at all, and just pass in the data volume to IsPopulated. Since that is provided by CDI, it could probably be wrapped so that one can just pass in the DV. Not a big thing but the flow is pretty bumpy.
There was a problem hiding this comment.
There is one other check if the volume is populated for hotplug. Is that covered too?
I think it is, since volumeReadyToAttachToNode is invoked for every datavolume, be it hotplugged or not.
It may make sense to get rid of this callback function and now, that we have to do pre-validation to see if the DV exist at all, and just pass in the data volume to IsPopulated. Since that is provided by CDI, it could probably be wrapped so that one can just pass in the DV. Not a big thing but the flow is pretty bumpy.
I'm not sure I entirely follow. Did you mean to change the code such that the dataVolumeFunc that is passed into IsPopulated will directly return the correct DataVolume object? If so, than this is not too far from what's already being done.
|
/retest |
1 similar comment
|
/retest |
zcahana
force-pushed
the
pvc_not_found
branch
from
c1ee7fb
to
bb1ce67
Compare
|
/retest |
…ound Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
|
/lgtm |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
1 similar comment
|
/retest |
What this PR does / why we need it:
This PR extends the VM status field (
.status.printableStatus) with newErrorPvcNotFound/ErrorDataVolumeNotFoundstatuses, reported when a VM with a PVC/DataVolume type of volume is started, and the referenced object does not exist.Special notes for your reviewer:
Reasonfield of the VMI'sSynchronized=Falsecondition set when the referenced DataVolume does not exist. Previously, it was (misleadingly) set toFailedPvcNotFound, whereas now it gets set toFailedDataVolumeNotFound.I deliberately didn't include any new functest, as I believe this is well covered by unit tests.Release note: