Use default `{ib-}sriov-cni` entrypoint script #581

zeeke · 2024-01-10T10:14:15Z

Avoid overriding entrypoint script for sriov-cni and ib-sriov-cni. It is more robust to rely on the image logic, as the source image can change the path where the binary is stored.

Move the copy logic back to the containers: section, as the image's entrypoint script ends with a "sleep forever" [1].

Mount /etc/os-release file to let the copy logic to make decision on which binary it should copy depending on the operating system.

refs:
[1] https://github.com/k8snetworkplumbingwg/sriov-cni/blob/07ecbf9b6ee867054df47423e51a800034ef4e05/images/entrypoint.sh#L63

cc @SchSeba

github-actions · 2024-01-10T10:14:26Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coveralls · 2024-01-10T10:20:18Z

Pull Request Test Coverage Report for Build 7974655862

Details

0 of 0 changed or added relevant lines in 0 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.03%) to 34.481%

Files with Coverage Reduction	New Missed Lines	%
pkg/daemon/daemon.go	1	38.06%

Totals
Change from base Build 7974112157:	0.03%
Covered Lines:	4135
Relevant Lines:	11992

💛 - Coveralls

mlguerrero12 · 2024-01-10T12:14:26Z

LGTM

adrianchiris · 2024-01-10T13:26:08Z

bindata/manifests/daemon/daemonset.yaml

+        volumeMounts:
+        - name: cnibin
+          mountPath: /host/opt/cni/bin
+        - name: os-release


who is using this mount ? sriov cni entrypoint does not use it AFAIK

adrianchiris · 2024-01-10T13:26:41Z

bindata/manifests/daemon/daemonset.yaml

+        volumeMounts:
+        - name: cnibin
+          mountPath: /host/opt/cni/bin
+        - name: os-release


same Q as above :)

adrianchiris · 2024-01-10T13:28:32Z

Move the copy logic back to the containers: section, as the image's entrypoint script ends with a "sleep forever" [1].

we could extend sriov-cni and ib-sriov-cni with a --oneshot flag which will avoid the sleep. WDYT ?
then we can keep in init containers

zeeke · 2024-01-10T16:29:49Z

we could extend sriov-cni and ib-sriov-cni with a --oneshot flag which will avoid the sleep. WDYT ?
then we can keep in init containers

I like that! Going to file a PR in sriov-cni and ib-sriov-cni.

Regarding the /etc/os-release, I need it to implement an openshift only change to support running the cni on multiple RHEL versions. I'm trying to keep the community changes as low as possible: the idea is to pass that file to the cni image entrypoint, where there can be multiple sriov-cni binaries. Then a script will copy the right binary to the host.

This logic is also present in other multus cni plugin:
https://github.com/openshift/cluster-network-operator/blob/873b3290a2de28fb7fb41ad498fc9872dcb1a93b/bindata/network/multus/multus.yaml#L13

github-actions · 2024-02-02T15:14:25Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

zeeke · 2024-02-02T15:40:56Z

/hold

this PR depends on:

github-actions · 2024-02-12T14:37:31Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-02-12T16:31:53Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

zeeke · 2024-02-13T10:23:00Z

There's still a problem with ib-sriov-cni. Waiting for

Remove linux-headers apk from Dockerfile ib-sriov-cni#84

zeeke · 2024-02-20T07:11:37Z

/hold cancel

related work on sriov-cni and ib-sriov-cni has been merged

SchSeba

nice work!

LGTM

adrianchiris

overall LGTM,

can you amend the commit msg.

i think this part is no longer true:

Move the copy logic back to the `containers:` section, as the image's
entrypoint script ends with a "sleep forever" [1].

we keep copy of cni bin in initContainer still, the cni image entrypoint now supports --no-sleep flag which allows it to function as init container.

Avoid overriding entrypoint script for sriov-cni and ib-sriov-cni. It is more robust to rely on the image logic, as the source image can change the path where the binary is stored. Mount `/etc/os-release` file to let the copy logic to make decision on which binary it should copy depending on the operating system. refs: [1] https://github.com/k8snetworkplumbingwg/sriov-cni/blob/07ecbf9b6ee867054df47423e51a800034ef4e05/images/entrypoint.sh#L63 Signed-off-by: Andrea Panattoni <[email protected]>

github-actions · 2024-02-20T13:59:52Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

adrianchiris · 2024-02-20T14:21:17Z

@zeeke feel free to remove the hold label and merge this one, thx for working on it !

adrianchiris reviewed Jan 10, 2024

View reviewed changes

zeeke mentioned this pull request Jan 11, 2024

Optionally avoid sleeping in entrypoint.sh k8snetworkplumbingwg/sriov-cni#289

Merged

zeeke force-pushed the use-cni-image-entrypoint branch from 21fd429 to 63dedfa Compare February 2, 2024 15:14

zeeke mentioned this pull request Feb 2, 2024

Optionally avoid sleeping in entrypoint.sh k8snetworkplumbingwg/ib-sriov-cni#83

Merged

github-actions bot added the hold label Feb 2, 2024

zeeke force-pushed the use-cni-image-entrypoint branch from 63dedfa to a7012c8 Compare February 12, 2024 14:37

zeeke force-pushed the use-cni-image-entrypoint branch from a7012c8 to 0241f36 Compare February 12, 2024 16:31

zeeke requested a review from adrianchiris February 20, 2024 07:11

SchSeba approved these changes Feb 20, 2024

View reviewed changes

adrianchiris reviewed Feb 20, 2024

View reviewed changes

zeeke force-pushed the use-cni-image-entrypoint branch from 0241f36 to 0a74848 Compare February 20, 2024 13:59

adrianchiris approved these changes Feb 20, 2024

View reviewed changes

zeeke removed the hold label Feb 20, 2024

zeeke merged commit 6d52067 into k8snetworkplumbingwg:master Feb 20, 2024
12 checks passed

zeeke mentioned this pull request Mar 18, 2024

OCPBUGS-26220: [release-4.15] Use default {ib-}sriov-cni entrypoint script openshift/sriov-network-operator#893

Closed

zeeke mentioned this pull request Mar 20, 2024

OCPBUGS-26220: [release-4.15] build for RHEL8 and RHEL9 openshift/sriov-cni#100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use default `{ib-}sriov-cni` entrypoint script #581

Use default `{ib-}sriov-cni` entrypoint script #581

zeeke commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

coveralls commented Jan 10, 2024 •

edited

Loading

mlguerrero12 commented Jan 10, 2024

adrianchiris Jan 10, 2024

adrianchiris Jan 10, 2024

adrianchiris commented Jan 10, 2024

zeeke commented Jan 10, 2024

github-actions bot commented Feb 2, 2024

zeeke commented Feb 2, 2024

github-actions bot commented Feb 12, 2024

github-actions bot commented Feb 12, 2024

zeeke commented Feb 13, 2024

zeeke commented Feb 20, 2024

SchSeba left a comment

adrianchiris left a comment

github-actions bot commented Feb 20, 2024

adrianchiris commented Feb 20, 2024

Use default {ib-}sriov-cni entrypoint script #581

Use default {ib-}sriov-cni entrypoint script #581

Conversation

zeeke commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

coveralls commented Jan 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 7974655862

Details

💛 - Coveralls

mlguerrero12 commented Jan 10, 2024

adrianchiris Jan 10, 2024

Choose a reason for hiding this comment

adrianchiris Jan 10, 2024

Choose a reason for hiding this comment

adrianchiris commented Jan 10, 2024

zeeke commented Jan 10, 2024

github-actions bot commented Feb 2, 2024

zeeke commented Feb 2, 2024

github-actions bot commented Feb 12, 2024

github-actions bot commented Feb 12, 2024

zeeke commented Feb 13, 2024

zeeke commented Feb 20, 2024

SchSeba left a comment

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2024

adrianchiris commented Feb 20, 2024

Use default `{ib-}sriov-cni` entrypoint script #581

Use default `{ib-}sriov-cni` entrypoint script #581

coveralls commented Jan 10, 2024 •

edited

Loading