ENG-84938: add ibKubernetesEnabled to SriovIBNetwork and render it into the generated IB NADs - #17
Open
rohan-togetherai wants to merge 83 commits into
Open
ENG-84938: add ibKubernetesEnabled to SriovIBNetwork and render it into the generated IB NADs#17rohan-togetherai wants to merge 83 commits into
rohan-togetherai wants to merge 83 commits into
Conversation
PrometheusRules allow recording pre-defined queries. Use `sriov_kubepoddevice` metric to add `pod|namespace` pair to the sriov metrics. Feature is enabled via the `METRICS_EXPORTER_PROMETHEUS_DEPLOY_RULE` environment variable. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
When the `metricsExporter` feature is turned off, deployed resources
should be removed. These changes fix the error:
```
│ 2024-08-28T14:07:57.699760017Z ERROR controller/controller.go:266 Reconciler error {"controller": "sriovoperatorconfig", "controllerGroup": "sriovnetwork.openshift.io", "controllerKind": "SriovOperatorConfig", "SriovOperatorConfig": {"name":"default","namespace":"openshift-sriov-network-operator"}, │
│ "namespace": "openshift-sriov-network-operator", "name": "default", "reconcileID": "fa841c50-dbb8-4c4c-9ddd-b98624fd2a24", "error": "failed to delete object &{map[apiVersion:monitoring.coreos.com/v1 kind:ServiceMonitor metadata:map[name:sriov-network-metrics-exporter namespace:openshift-sriov-network-operator] │
│ spec:map[endpoints:[map[bearerTokenFile:/var/run/secrets/kubernetes.io/serviceaccount/token honorLabels:true interval:30s port:sriov-network-metrics scheme:https tlsConfig:map[caFile:/etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt insecureSkipVerify:false serverName:sriov-network-metrics-expor │
│ ter-service.openshift-sriov-network-operator.svc]]] namespaceSelector:map[matchNames:[openshift-sriov-network-operator]] selector:map[matchLabels:map[name:sriov-network-metrics-exporter-service]]]]} with err: could not delete object (monitoring.coreos.com/v1, Kind=ServiceMonitor) openshift-sriov-network-operato │
│ r/sriov-network-metrics-exporter: servicemonitors.monitoring.coreos.com \"sriov-network-metrics-exporter\" is forbidden: User \"system:serviceaccount:openshift-sriov-network-operator:sriov-network-operator\" cannot delete resource \"servicemonitors\" in API group \"monitoring.coreos.com\" in the namespace \"ope │
│ nshift-sriov-network-operator\""}
```
Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
…er-rules [metrics 4/x] Metrics exporter rules
…devices Refactor some conformance tests to use `SRIOV_NODE_AND_DEVICE_NAME_FILTER`
if the current obj as annotation and the updated doesn't we still want to add the ones from the current object Signed-off-by: Sebastian Sch <sebassch@gmail.com>
When a user deletes the default SriovOperatorConfig resource and tries to recreate it afterwards, the operator webhooks returns the error: ``` Error from server (InternalError): error when creating "/tmp/opconfig.yml": Internal error occurred: failed calling webhook "operator-webhook.sriovnetwork.openshift.io": failed to call webhook: Post "https://operator-webhook-service.openshift-sriov-network-operator.svc:443/validating-custom-resource?timeout=10s": service "operator-webhook-service" not found ``` as the webhook configuration is still present, while the Service and the DaemonSet has been deleted. Delete all the webhook configurations when the user deletes the default SriovOperatorConfig Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
Fix merge annotation function
Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
The bash syntax was incorrect and yielded:
hack/env.sh: line 35: ${$RDMA_CNI_IMAGE:-}: bad substitution
…IMAGE Fix syntax for RDMA_CNI_IMAGE var substitution
It might happen that two SR-IOV pods, deployed on different node, are using devices
with the same PCI address. In such cases, the query suggested [1] by the sriov-network-metrics-exporter produces the error:
```
Error loading values found duplicate series for the match group {pciAddr="0000:3b:02.4"} on the right hand-side of the operation:
[
{
__name__="sriov_kubepoddevice",
container="test",
dev_type="openshift.io/intelnetdevice",
endpoint="sriov-network-metrics",
instance="10.1.98.60:9110",
job="sriov-network-metrics-exporter-service",
namespace="cnf-4916",
pciAddr="0000:3b:02.4",
pod="pod-cnfdr22.telco5g.eng.rdu2.redhat.com",
prometheus="openshift-monitoring/k8s",
service="sriov-network-metrics-exporter-service"
}, {
__name__="sriov_kubepoddevice",
container="test",
dev_type="openshift.io/intelnetdevice",
endpoint="sriov-network-metrics",
instance="10.1.98.230:9110",
job="sriov-network-metrics-exporter-service",
namespace="cnf-4916",
pciAddr="0000:3b:02.4",
pod="pod-dhcp-98-230.telco5g.eng.rdu2.redhat.com",
prometheus="openshift-monitoring/k8s",
service="sriov-network-metrics-exporter-service"
}
];many-to-many matching not allowed: matching labels must be unique on one side
```
Configure the ServiceMonitor resource to add a `node` label to all metrics.
The right query to get metrics, as updated in the PrometheusRule, will be:
```
sriov_vf_tx_packets * on (pciAddr,node) group_left(pod,namespace,dev_type) sriov_kubepoddevice
```
Also remove `pod`, `namespace` and `container` label from the `sriov_vf_*` metrics, as they were
wrongly set to `sriov-network-metrics-exporter-zj2n9`, `openshift-sriov-network-operator`, `kube-rbac-proxy`
[1] https://github.com/k8snetworkplumbingwg/sriov-network-metrics-exporter/blob/0f6a784f377ede87b95f31e569116ceb9775b5b9/README.md?plain=1#L38
Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
When we want to use config-drive in immutable systems, very often the config-drive is only used at boot and then umounted (e.g. ignition does this). Later when we want to fetch Metadata from the config drive, we actually have to mount it. In this PR, I'm adding similar code than coreos/ignition where we dynamically mount the config-drive is the device was found with the right label (config-2 or CONFIG-2 as documented in OpenStack). If the device is found, we mount it, fetch the data and umount it.
Fixes the following shellcheck error:
SC2068 (error): Double quote array expansions to avoid re-splitting elements.
https://www.shellcheck.net/wiki/SC2068
Fixes the following shellcheck error:
SC2148 (error): Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
https://www.shellcheck.net/wiki/SC2148
Fixes the following shellcheck errors:
SC2145 (error): Argument mixes string and array. Use * or separate argument.
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
https://www.shellcheck.net/wiki/SC2145
https://www.shellcheck.net/wiki/SC2199
Also fixes a typo in SUPPORTED_INTERFACE_SWITCHER_MODES.
Fixes the following shellcheck error:
SC2045 (error): Iterating over ls output is fragile. Use globs.
https://www.shellcheck.net/wiki/SC2045
On some kernels GetDevlinkDeviceParam may return empty values for some kernel parameters. The netlink library is able to handle this, but the code in GetDevlinkDeviceParam function may panic if unexpected value received. Add extra checks to avoid panics
Delete webhooks when SriovOperatorConfig is deleted
…getdevlinkdeviceparam Fix: GetDevlinkDeviceParam to handle edge-cases correctly
…er-drop-labels [metrics 5/x] Add node label to sriov_* metrics
`sriov_kubepoddevice` metric might end up in the Prometheus database after a while, as the default scrape interval is 30s. This leads to failures in the end-to-end lane like: ``` [sriov] Metrics Exporter When Prometheus operator is available [It] Metrics should have the correct labels /root/opr-ocp2-1/data/sriov-network-operator/sriov-network-operator/test/conformance/tests/test_exporter_metrics.go:132 [FAILED] no value for metric sriov_kubepoddevice ``` Put the metric assertion in an `Eventually` statement Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Fixes the following shellcheck error:
SC2081 (error): [ .. ] can't match globs. Use a case statement.
https://www.shellcheck.net/wiki/SC2081
Warns about shellcheck issues with severity `error`.
…-fix metrics: Fix `Metrics should have the correct labels` test
CI: Add a bash linter to pre-submits
openstack: dynamically mount the config-drive
When the operator changes the device-plugin Spec (e.g. .Spec.NodeSelector), it may happen that there are two device plugin pods for a given node, one that is terminating, the other that is initializing. If the config-daemon executes `restartDevicePluginPod()` at the same time, it may kill the terminating pod, while the initializing one will run with the old dp configuration. This may cause one or more resources to not being advertised, until a manual device plugin restart occurs. Make the config-daemon restart all the device-plugin instances it founds for its own node. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
…uids_clean_v4 # Conflicts: # pkg/host/types/interfaces.go
…ivery Add the ibKubernetesEnabled API field, regenerate both CRD copies with controller-gen v0.14.0, render the key into the ib-sriov NAD config (only when enabled; the sriov renderer sets the template key false), and pin the rendered NAD with a live-equivalent golden test. With the key true, the ib-sriov CNI refuses a GUID-less ADD with a named error instead of attaching without a programmed GUID (ENG-84938).
Round-2 review edit 5 for the ibKubernetesEnabled draft: - Add a controller reconciliation entry that creates a SriovIBNetwork with capabilities, pKey, linkState, and ibKubernetesEnabled set and asserts the reconciled NetworkAttachmentDefinition config carries "ibKubernetesEnabled":true alongside the existing keys. - Teach generateExpectedIBNetConfig to render capabilities, pkey, and ibKubernetesEnabled in template order. - Replace the foo: bar placeholder sample with a real SriovIBNetwork fail-closed example. Envtest after this change: Kubernetes mode ran 54 of 55 specs, all passed, 1 skipped; OpenShift mode ran and passed 55 of 55 (the suites were 53 of 54 and 54 of 54 before the new entry).
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Broly Security ScanWarning Summary 38 actionable finding(s) in this PR
13 highest-priority actionable rows in the table below (critical/high first, then top medium). 5 finding(s) below the
Fix SuggestionsContainers should not run with allowPrivilegeEscalation — deploy/operator.yaml:1Ensure the container does not use Minimize the admission of pods which lack an associated NetworkPolicy — deploy/operator.yaml:1Review this infrastructure finding and apply the remediation described in the Checkov documentation. Minimize the admission of containers with the NET_RAW capability — deploy/operator.yaml:1Add a
Ensure that the seccomp profile is set to docker/default or runtime/default — deploy/operator.yaml:1Ensure the container has a Minimize the admission of containers with capabilities assigned — deploy/operator.yaml:1Review this Kubernetes manifest finding and apply the remediation described in the Checkov documentation. Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/kubernetes-policies/kubernetes-policy-index/bc-k8s-34 Minimize wildcard use in Roles and ClusterRoles — deploy/role.yaml:1Replace wildcard entries in the Role's
unpinned action reference — .github/workflows/test.yml:107Pin actions/checkout to the commit v4 currently resolves to:
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -104,7 +104,7 @@
name: Shellcheck
runs-on: ubuntu-latest
steps:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
- name: Run ShellCheck
uses: ludeeus/action-shellcheck@master
with:unpinned action reference — .github/workflows/test.yml:109Pin ludeeus/action-shellcheck to the commit master currently resolves to:
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -106,7 +106,7 @@
steps:
- uses: actions/checkout@v4
- name: Run ShellCheck
- uses: ludeeus/action-shellcheck@master
+ uses: ludeeus/action-shellcheck@00b27aa7cb85167568cb48a3838b75f4265f2bca # master
with:
severity: error
Dismiss false positivesTick a box to dismiss the finding; untick it to bring the finding back. That is the same as replying
18 more dismissable finding(s) omitted (max 20 per scan). Reply with Note Re-scan this PR anytime with
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name: Shellcheck | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Run ShellCheck | ||
| uses: ludeeus/action-shellcheck@master |
Comment on lines
+103
to
+111
| shellcheck: | ||
| name: Shellcheck | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Run ShellCheck | ||
| uses: ludeeus/action-shellcheck@master | ||
| with: | ||
| severity: error |
| name: Shellcheck | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
| package mock_helper | ||
|
|
||
| import ( | ||
| "net" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
16 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds an optional
ibKubernetesEnabledboolean to the SriovIBNetwork spec and renders it into the generated NetworkAttachmentDefinition config. The template writes the key only when the field is set, so existing CRs render as they do today. Both CRD copies (config/crd/basesand the chartcrds/) are regenerated with controller-gen v0.14.0 throughmake manifests. A golden test pins the rendered config with the key set. A controller test reconciles a SriovIBNetwork with capabilities, pKey, linkState, and ibKubernetesEnabled set, and asserts the reconciled NAD carries"ibKubernetesEnabled":truealongside the existing keys. The sample CR is now a real fail-closed example instead of afoo: barplaceholder.A one-file change in tnetwork-operator, included here, syncs the vendored SriovIBNetwork CRD to the regenerated schema. The vendored copy was also missing the pre-existing
pKeyproperty that production IB NADs depend on; the sync adds that property too.Why
With
ibKubernetesEnabled: truein the NAD, the ib-sriov CNI refuses a GUID-less attach with a named error instead of silently attaching without a programmed GUID. Today the generator has no way to set the key: the operator API has no field for it.With the key set, a late GUID still blocks the attach. What changes is the failure: a named error before any VF is touched, instead of an attach that reports success with no GUID programmed. The delivery delay itself is fixed in the ib-kubernetes PR.
Verification
make manifestsreruns diff-clean. The two CRD copies in this repo are byte-equal, and the vendored tnetwork-operator copy matches them after its sync.Rollback
Revert the two commits. No live SriovIBNetwork CR sets the field, so no rendered NAD changes at merge or at revert. The tnetwork-operator sync is safe to keep either way: it corrects a stale vendored copy.
Not in this PR
This PR does not set the field on any live SriovIBNetwork CR. Setting the field on live CRs is the fail-closed change itself; it happens under the canary PR's prerequisite list, together with the ib-kubernetes fix and the annotation-latency alert. Chart version bump, operator image build, and rollout follow separately. No upstream contribution is filed yet.
Related to ENG-84938