update crds for spiderpool dra feature #3527
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3527 +/- ##
=======================================
Coverage 81.16% 81.16%
=======================================
Files 50 50
Lines 4391 4391
=======================================
Hits 3564 3564
Misses 670 670
Partials 157 157
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -95,6 +99,10 @@ type SpiderIPvlanCniConfig struct { | |||
| // +kubebuilder:validation:Optional | |||
| Bond *BondConfig `json:"bond,omitempty"` | |||
|
|
|||
| // +kubebuilder:default=false | |||
| // +kubebuilder:validation:Optional | |||
| EnableRdma bool `json:"enableRdma"` | |||
There was a problem hiding this comment.
I am wondering whether there should be a new field called "rdmaResourceName" here
There was a problem hiding this comment.
done, maybe we can only use one field for this. If the value is empty, which means we don't use the rdma plugin. WDYT?
| @@ -79,6 +79,9 @@ type SpiderMacvlanCniConfig struct { | |||
| // +kubebuilder:validation:Optional | |||
| Bond *BondConfig `json:"bond,omitempty"` | |||
|
|
|||
| // +kubebuilder:validation:Optional | |||
There was a problem hiding this comment.
there should be another filed enableRdma
There was a problem hiding this comment.
maybe we can only use one field for this. If the value is empty, which means we don't use the rdma plugin. WDYT?
There was a problem hiding this comment.
I do not think that way. Although, RdmaResourceName is a manual way. If no enableRdma, the user has no way to tell whether turn on it or not , when it supports to automatically inject resource name
and currently, sriov already has enableRdma field
There was a problem hiding this comment.
I'm ok with if we should keep consistent with sriov.
If no enableRdma, the user has no way to tell whether turn on it or not , when it supports to automatically inject resource name
ah, I think the resourceName is not to be automatically injected, configures it only manually, the Auto-injection means configuring the resourceName into the container's resources.
There was a problem hiding this comment.
follow your advice, done.
Signed-off-by: cyclinder <[email protected]>
|
@weizhoublue ready to merge, please, this pr is pending spidernet-io/multus-cni#2. |
Thanks for contributing!
What type of PR is this?
What this PR does / why we need it:
update crds for spiderpool dra feature
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: