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.

Sign up for GitHub

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

Jump to bottom

Check claimName on DV status #530

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Check claimName on DV status #530

merged 2 commits into from
Aug 15, 2023

Conversation

bennyz
Copy link
Member

@bennyz bennyz commented Aug 10, 2023

It seems like dv.PVC is always nil because we don't create a DV from a PVC, this leads to failing to find the DV whenever the importer pod fails and it will restart forever instead of 3 times

@bennyz bennyz requested a review from ahadas as a code owner August 10, 2023 10:46
@bennyz bennyz force-pushed the fix-importer-pod-lookup branch from 43417c8 to b24805f Compare August 10, 2023 10:49
@ahadas ahadas requested a review from liranr23 August 10, 2023 12:14
liranr23
liranr23 previously approved these changes Aug 10, 2023
View reviewed changes
Copy link
Member

@liranr23 liranr23 left a comment

Choose a reason for hiding this comment

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

The change is ok but the commit is strange. The PVC is created by the DV. Isn't dv.PVC should be the created PVC by the DV?

@bennyz bennyz marked this pull request as draft August 10, 2023 14:23
@bennyz
Copy link
Member Author

bennyz commented Aug 10, 2023

The change is ok but the commit is strange. The PVC is created by the DV. Isn't dv.PVC should be the created PVC by the DV?

I'll have to look at this again, I got fooled by the name of the object, this is not the CDI DataVolume, it's our DataVolume[1] wrapper...

[1]

forklift/pkg/controller/plan/kubevirt.go

Line 1690 in 76efe8a

type DataVolume struct {

@liranr23 liranr23 dismissed their stale review August 10, 2023 15:11

further changes

@bennyz bennyz marked this pull request as ready for review August 13, 2023 10:43
@bennyz
Copy link
Member Author

bennyz commented Aug 13, 2023

The change is ok but the commit is strange. The PVC is created by the DV. Isn't dv.PVC should be the created PVC by the DV?

I'll have to look at this again, I got fooled by the name of the object, this is not the CDI DataVolume, it's our DataVolume[1] wrapper...

[1]

forklift/pkg/controller/plan/kubevirt.go

Line 1690 in 76efe8a

type DataVolume struct {

ok, so the wrapper is only populated when calling VirtualMachineMap

liranr23
liranr23 approved these changes Aug 13, 2023
View reviewed changes
pkg/controller/plan/kubevirt.go Outdated Show resolved Hide resolved
ahadas
ahadas reviewed Aug 13, 2023
View reviewed changes
pkg/controller/plan/kubevirt.go Outdated Show resolved Hide resolved
pkg/controller/plan/migration.go Show resolved Hide resolved
@ahadas ahadas added this to the v2.5.0 milestone Aug 15, 2023
bennyz added 2 commits August 15, 2023 10:47
@bennyz
Check claimName on DV status
2a038d0 
It seems like dv.PVC is always nil because we don't create a DV from a
PVC, this leads to failing to find the DV whenever the importer pod
fails and it will restart forever instead of 3 times

Signed-off-by: Benny Zlotnik <[email protected]>
@bennyz
rename DataVolume to DataVolumeWrapper
7ea8beb 
Make it slightly less confusing

Signed-off-by: Benny Zlotnik <[email protected]>
@bennyz bennyz force-pushed the fix-importer-pod-lookup branch from c2178e5 to 7ea8beb Compare August 15, 2023 07:52
@bennyz bennyz mentioned this pull request Aug 15, 2023
Open
@sonarcloud
Copy link

sonarcloud bot commented Aug 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
5.1% 5.1% Duplication

@bennyz
Copy link
Member Author

bennyz commented Aug 15, 2023

Created #539

@ahadas ahadas merged commit 638e7b0 into kubev2v:main Aug 15, 2023
@bennyz bennyz deleted the fix-importer-pod-lookup branch August 15, 2023 12:10
@bennyz bennyz mentioned this pull request Aug 15, 2023
Merged
@ahadas ahadas removed this from the v2.5.0 milestone Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liranr23 liranr23 liranr23 approved these changes

@ahadas ahadas ahadas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bennyz @ahadas @liranr23