-
Notifications
You must be signed in to change notification settings - Fork 114
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
Deploy RDMA CNI as a part of SR-IOV Operator #661
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9858421191Details
💛 - Coveralls |
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
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
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:
|
@SchSeba could you please review this PR? |
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.
sorry for the super late review.
I add some small comments
@@ -92,6 +92,23 @@ spec: | |||
mountPath: /host/etc/os-release | |||
readOnly: true | |||
{{- end }} | |||
{{- if ne .RDMACNIImage "" }} |
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.
nit: please can the if statement be the same as {{- if .OVSCNIImage }}
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
- name: rdma-cni | ||
image: {{.RDMACNIImage}} | ||
command: | ||
- /bin/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.
please let's not have a copy like this here.
can we create an entrypoint.sh and use it in the image for the rdma-cni repo (I can review and approve it)
the same way we do for the sriov-cni
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.
PR to SR-IOV CNI: k8snetworkplumbingwg/rdma-cni#56
hack/env.sh
Outdated
@@ -13,6 +15,8 @@ else | |||
[ -z $SRIOV_INFINIBAND_CNI_IMAGE ] && echo "SRIOV_INFINIBAND_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1 | |||
# check that OVS_CNI_IMAGE is set to any value, empty string is a valid value | |||
[ -z ${OVS_CNI_IMAGE+set} ] && echo "OVS_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1 | |||
# check that RDMA_CNI_IMAGE is set to any value, empty string is a valid value | |||
[ -z ${$RDMA_CNI_IMAGE+set} ] && echo "RDMA_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1 |
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.
please do something like we did for the ovs-cni
# ensure that OVS_CNI_IMAGE is set, empty string is a valid value
OVS_CNI_IMAGE=${OVS_CNI_IMAGE:-}
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 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.
just two small comments and we should be good to go nice work
hack/env.sh
Outdated
@@ -11,6 +13,8 @@ if [ -z $SKIP_VAR_SET ]; then | |||
else | |||
# ensure that OVS_CNI_IMAGE is set, empty string is a valid value | |||
OVS_CNI_IMAGE=${OVS_CNI_IMAGE:-} | |||
# ensure that RDMA_CNI_IMAGE is set, empty string is a valid value | |||
$RDMA_CNI_IMAGE=${$RDMA_CNI_IMAGE:-} |
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.
$RDMA_CNI_IMAGE= -> RDMA_CNI_IMAGE=
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
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.
@e0ne please rebase so we can give it a final review and merge. |
Thanks for your PR,
To skip the vendors CIs use one of:
|
done |
hack/env.sh
Outdated
@@ -3,6 +3,8 @@ if [ -z $SKIP_VAR_SET ]; then | |||
export SRIOV_INFINIBAND_CNI_IMAGE=${SRIOV_INFINIBAND_CNI_IMAGE:-ghcr.io/k8snetworkplumbingwg/ib-sriov-cni} | |||
# OVS_CNI_IMAGE can be explicitly set to empty value, use default only if the var is not set | |||
export OVS_CNI_IMAGE=${OVS_CNI_IMAGE-ghcr.io/k8snetworkplumbingwg/ovs-cni-plugin} | |||
# RDMA_CNI_IMAGE can be explicitly set to empty value, use default only if the var is not set | |||
export RDMA_CNI_IMAGE=${RDMA_CNI_IMAGE-ghcr.io/k8snetworkplumbingwg/rdma-cni} |
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.
Please align indentation.
hack/env.sh
Outdated
@@ -13,6 +15,8 @@ if [ -z $SKIP_VAR_SET ]; then | |||
else | |||
# ensure that OVS_CNI_IMAGE is set, empty string is a valid value | |||
OVS_CNI_IMAGE=${OVS_CNI_IMAGE:-} | |||
# ensure that RDMA_CNI_IMAGE is set, empty string is a valid value | |||
RDMA_CNI_IMAGE=${$RDMA_CNI_IMAGE:-} |
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.
Please align indentation.
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, please align indent as @rollandf commented and we can merge.
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.
@e0ne can you rebase against master ?
@@ -103,6 +103,7 @@ images: | |||
sriovCni: ghcr.io/k8snetworkplumbingwg/sriov-cni | |||
ibSriovCni: ghcr.io/k8snetworkplumbingwg/ib-sriov-cni | |||
ovsCni: ghcr.io/k8snetworkplumbingwg/ovs-cni-plugin | |||
rdmaCni: ghcr.io/k8snetworkplumbingwg/rdma-cni |
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.
please update helm chart update script :
OVS_CNI_TAG=$(get_latest_github_tag k8snetworkplumbingwg ovs-cni) |
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.
@adrianchiris done
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
nice work!
/hold Hi @e0ne please check the k8s CI looks like the helm deploy doesn't work |
Signed-off-by: Ivan Kolodiazhnyi <[email protected]>
Thanks for your PR,
To skip the vendors CIs use one of:
|
fixed a typo |
why do we need to to it? |
/hold cancel |
Hi @adrianchiris can you please approve so we can merge this one? |
No description provided.