-
Notifications
You must be signed in to change notification settings - Fork 109
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.
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
[metrics 1/x] Support sriov-network-metrics-exporter
#655
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9584220421Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work thanks!
I add some comments in the code.
some other points:
- can you also add to the k8s and ocp CI files the option to have LOCAL image for this. it will allow us to add the CI for that repo also
- I think we need in the namespace to add the monitoring annotation for prometheus to scrap this namespace
- we also need a ServiceMonitoring object no?
- will you be able to implement a func test to see that the feature is working as expected?
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some small comments in general the only major stuff we need to take care of is removing the request for the configMap.
I can already see a problem with that for example in RHEL the driver version is the same as the kernel and it's for sure different from the u/s driver version
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit otherwise LGTM.
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two small nits an a request for a following up PR
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/hold
unit we do a release for the operator
Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
Thanks for your PR,
To skip the vendors CIs use one of:
|
@SchSeba @adrianchiris Operator v1.3.0 has been released. Can we proceed with this? |
@@ -102,5 +106,7 @@ images: | |||
sriovDevicePlugin: ghcr.io/k8snetworkplumbingwg/sriov-network-device-plugin | |||
resourcesInjector: ghcr.io/k8snetworkplumbingwg/network-resources-injector | |||
webhook: ghcr.io/k8snetworkplumbingwg/sriov-network-operator-webhook | |||
metricsExporter: ghcr.io/k8snetworkplumbingwg/sriov-network-metrics-exporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we have chart push on release (tag) please adjust hack/release/chart-update.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! thanks for pointing out
Deploy `sriov-network-metrics-exporter` DaemonSet and related configuration. The feature is activated by the feature gate `metricsExporter`. Add deployment logic to the SriovOperatorConfig reconcile loop. The operator's environment variable `SRIOV_NETWORK_METRICS_EXPORTER_IMAGE` controls the exporter image to deploy. Update helm charts with `.Values.images.metricsExporter` with the same semantic. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
Read `LOCAL_SRIOV_NETWORK_METRICS_EXPORTER_IMAGE` environment variable to deploy a custom version of the sriov-network-metrics-exporter. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
Thanks for your PR,
To skip the vendors CIs use one of:
|
@zeeke i wasnt able to find sriov-network-metrics-exporter container image tagged with v1.0.0 can you do another release to trigger creation of release image with tag ? merging this one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Deploy
sriov-network-metrics-exporter
DaemonSet and related configuration. The feature is activated by the feature gatemetricsExporter
.Add deployment logic to the SriovOperatorConfig reconcile loop.
The operator's environment variable
SRIOV_NETWORK_METRICS_EXPORTER_IMAGE
controls the exporter image to deploy. Update helm charts with.Values.images.metricsExporter
with the same semantic.depends on: