Skip to content

feat: Add KubeVirt VirtualMachine custom health checks [#7175] #7176

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
Merged
merged 1 commit into from
Nov 4, 2021
Merged

feat: Add KubeVirt VirtualMachine custom health checks [#7175] #7176

merged 1 commit into from
Nov 4, 2021

Conversation

cwilkers
Copy link
Contributor

@cwilkers cwilkers commented Sep 7, 2021

Signed-off-by: cwilkers cwilkers@redhat.com

Checklist:

Fixes #7175 Resource Customizations for KubeVirt Data Types

This does not need to be in release notes nor docs

Red Hat appears in USERS.md

@cwilkers cwilkers force-pushed the kubevirt-healthscripts branch from 914af8a to a9c777f Compare September 7, 2021 17:06
zcahana
zcahana reviewed Sep 9, 2021
View reviewed changes
resource_customizations/kubevirt.io/VirtualMachineInstance/health.lua
@@ -0,0 +1,15 @@
hs = { status="Progressing", message="No status available"}
Copy link
Contributor

Choose a reason for hiding this comment

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

For a VMI, there's a stable .status.phase field which can be used (alongside the Ready and Paused conditions).
Here's a reasonable mapping to Argo CD health states:

  • Pending, Scheduling, Scheduled --> Progressing
  • Succeeded --> Suspended (can also use Healthy but I think Suspended conveys the more significant aspect here)
  • Failed --> Degraded (can also use Suspended and have the hs.message discriminate between Succeeded/Failed/Paused)
  • Unknown --> Unknown
  • Running -->
    • Paused --> Suspended
    • !Ready --> Degraded
    • Ready --> Healthy

resource_customizations/cdi.kubevirt.io/DataVolumes/health.lua Outdated
@@ -0,0 +1,12 @@
hs = { status="Progressing", message="No status available"}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can supplement the phase with the Running condition to try and expose errors:

  • Succeeded --> Healthy
  • Failed --> Degraded
  • Unknown --> Unknown
  • Paused --> Suspended
  • Other phase + Running --> Progressing
  • Other phase + !Running --> Degraded

@cwilkers cwilkers force-pushed the kubevirt-healthscripts branch from a9c777f to 6b6138e Compare September 9, 2021 11:55
zcahana
zcahana reviewed Sep 19, 2021
View reviewed changes
zcahana
zcahana reviewed Sep 19, 2021
View reviewed changes
@cwilkers cwilkers force-pushed the kubevirt-healthscripts branch 3 times, most recently from ed4fe4f to 01fa9ec Compare September 24, 2021 16:41
@codecov
Copy link

codecov bot commented Sep 25, 2021 •
edited
Loading

Codecov Report

Merging #7176 (93ac6f2) into master (b264729) will increase coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7176      +/-   ##
==========================================
+ Coverage   41.10%   41.40%   +0.29%     
==========================================
  Files         160      161       +1     
  Lines       21505    21622     +117     
==========================================
+ Hits         8840     8952     +112     
- Misses      11399    11408       +9     
+ Partials     1266     1262       -4
Impacted Files Coverage Δ
util/env/env.go 61.53% <0.00%> (-22.68%) ⬇️
server/application/application.go 32.18% <0.00%> (-0.60%) ⬇️
server/cluster/cluster.go 22.51% <0.00%> (-0.36%) ⬇️
util/git/creds.go 10.00% <0.00%> (-0.30%) ⬇️
controller/appcontroller.go 53.09% <0.00%> (-0.22%) ⬇️
reposerver/repository/repository.go 60.69% <0.00%> (-0.16%) ⬇️
util/git/client.go 44.74% <0.00%> (-0.14%) ⬇️
controller/cache/cache.go 10.50% <0.00%> (-0.12%) ⬇️
util/settings/settings.go 46.83% <0.00%> (-0.11%) ⬇️
cmd/argocd/commands/login.go 2.29% <0.00%> (-0.03%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b264729...93ac6f2. Read the comment docs.

@jessesuen jessesuen self-assigned this Oct 21, 2021
@jessesuen jessesuen self-requested a review October 21, 2021 15:25
@sbose78
Copy link
Contributor

sbose78 commented Oct 21, 2021

Side question that shouldn't necessarily impact the merge:
@cwilkers what would be the best way to test this out end-to-end? Would you have sample CRs I could use to try this out on OpenShift ?

@cwilkers
Copy link
Contributor Author

Side question that shouldn't necessarily impact the merge: @cwilkers what would be the best way to test this out end-to-end? Would you have sample CRs I could use to try this out on OpenShift ?

Yes! The basic examples I have been using are from KubeVirt's labs.

The DataVolume can be created as soon as KubeVirt is up and running (lab1 on that site) Then there is a VirtualMachine to add, and the VirtualMachineInstance is created from the VM once it gets scheduled on a node. In normal usage, you would never directly create a VMI resource, only a VM or DV.

jessesuen
jessesuen reviewed Oct 21, 2021
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

I took a look at this PR since it got put onto the Argo CD maintainer's agenda. I'm not familiar with Kubevirt, but it looks like @zcahana is, based on GitHub activity, so I'm trusting his feedback on this PR and would like him to sign off on it before merging.

@zcahana
Copy link
Contributor

zcahana commented Oct 24, 2021

I took a look at this PR since it got put onto the Argo CD maintainer's agenda. I'm not familiar with Kubevirt, but it looks like @zcahana is, based on GitHub activity, so I'm trusting his feedback on this PR and would like him to sign off on it before merging.

Thanks @jessesuen.

@cwilkers please take a look at some previous review comments that have been made here, some of them are still outstanding, and IMO enables to provide a pretty solid/accurate status reporting in a rather simple manner (i.e., without complex logic).
Please let me know what you think.

@cwilkers
Copy link
Contributor Author

I took a look at this PR since it got put onto the Argo CD maintainer's agenda. I'm not familiar with Kubevirt, but it looks like @zcahana is, based on GitHub activity, so I'm trusting his feedback on this PR and would like him to sign off on it before merging.

Thanks @jessesuen.

@cwilkers please take a look at some previous review comments that have been made here, some of them are still outstanding, and IMO enables to provide a pretty solid/accurate status reporting in a rather simple manner (i.e., without complex logic). Please let me know what you think.

@zcahana, I was holding off another round of improvements until I had gotten the issue scoped for triage (while it was passing CI). I will integrate the feedback, test locally and push a commit/amend. Thanks @jessesuen, @zcahana !

@cwilkers cwilkers force-pushed the kubevirt-healthscripts branch from 01fa9ec to 71137eb Compare October 28, 2021 00:07
@cwilkers
Copy link
Contributor Author

@jessesuen @zcahana I have made the requested changes and everything tests okay locally. If you could take one more look, I'd appreciate it!

zcahana
zcahana reviewed Oct 28, 2021
View reviewed changes
@zcahana
Copy link
Contributor

zcahana commented Oct 28, 2021

I have made the requested changes and everything tests okay locally. If you could take one more look, I'd appreciate it!

Looks good @cwilkers! See a few additional minor comments, after which I think we're good to go.

@cwilkers
feat: Add KubeVirt custom health checks
93ac6f2 
Signed-off-by: cwilkers <cwilkers@redhat.com>
@cwilkers cwilkers force-pushed the kubevirt-healthscripts branch from 71137eb to 93ac6f2 Compare October 28, 2021 13:49
@zcahana
Copy link
Contributor

zcahana commented Oct 28, 2021

@cwilkers looks perfect now, thank you!
@jessesuen this is handed over to you now, from my perspective this is good to go as-is.

@cwilkers
Copy link
Contributor Author

@jessesuen Could I trouble you for another CI workflow run?

@jessesuen jessesuen merged commit fc13eda into argoproj:master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessesuen jessesuen jessesuen approved these changes

+2 more reviewers

@rmohr rmohr rmohr left review comments

@zcahana zcahana zcahana left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@jessesuen jessesuen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resource Customizations for KubeVirt Data Types
5 participants
@cwilkers @sbose78 @zcahana @rmohr @jessesuen