Skip to content

Kubevirt metrics capture specs#270

Open
nasark wants to merge 6 commits into
ManageIQ:masterfrom
nasark:metrics_capture_specs
Open

Kubevirt metrics capture specs#270
nasark wants to merge 6 commits into
ManageIQ:masterfrom
nasark:metrics_capture_specs

Conversation

@nasark

@nasark nasark commented Dec 19, 2024

Copy link
Copy Markdown
Member

We were missing some test coverage for Kubevirt metrics capture

@miq-bot assign @agrare
@miq-bot add_label test
@miq-bot add_reviewer @agrare

Comment on lines -73 to -77
raise TargetValidationError, "ems not defined" unless @ext_management_system
raise TargetValidationWarning, "no associated hardware" unless @vm_hardware
raise ManageIQ::Providers::Kubevirt::InfraManager::MetricsCapture::TargetValidationError, "ems not defined" unless @ext_management_system
raise ManageIQ::Providers::Kubevirt::InfraManager::MetricsCapture::TargetValidationWarning, "no associated hardware" unless @vm_hardware

raise TargetValidationError, "cores not defined" unless @vm_cores.to_i > 0
raise TargetValidationError, "memory not defined" unless @vm_memory.to_i > 0

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was needed since the capture context is subclassed from k8s which has its own error handling classes

class ManageIQ::Providers::Kubevirt::InfraManager::MetricsCapture::PrometheusCaptureContext < ManageIQ::Providers::Kubernetes::ContainerManager::MetricsCapture::PrometheusCaptureContext

This should not be needed if ManageIQ/manageiq-providers-kubernetes#543 is resolved but that may necessitate other changes to the specs

token: KUBEVIRT_TOKEN
metrics_hostname: METRICS_HOSTNAME
kubevirt:
<<: *kubevirt_defaults

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can't remember if we changed this yet or not. cc @jrafanie

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not yet. I haven't had time to get back to it. The referenced changes are coming from this core change: ManageIQ/manageiq#23292 and how each plugin would use these changes: ManageIQ/manageiq-providers-vmware#928

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I might just leave the warnings with rails 7.1 for now and people can fix them when their usage of rails secrets in 7.1 doesn't work or gets a warning. Note, I don't think we actually use rails to store secrets encrypted. That's the part that's completely gone in rails 7.1. Access is probably gone in 7.2.

@miq-bot

miq-bot commented Jun 9, 2025

Copy link
Copy Markdown
Member

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot

miq-bot commented Dec 26, 2025

Copy link
Copy Markdown
Member

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

1 similar comment
@miq-bot

miq-bot commented Apr 6, 2026

Copy link
Copy Markdown
Member

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants