Skip to content
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

small fixes in documentation #33

Merged
merged 1 commit into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,11 @@ spec:
#### Configuration details

* `numVFs`: if provided, configure SR-IOV VFs via nvconfig.
* This is a mandatory parameter.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rather make this field required in our API instead of only documenting this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is required, the additional comment here in the docs is for transparency

Copy link
Member

@tobiasgiese tobiasgiese Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is not defined as required via CRD validation. Is this on purpose?
We should add the annotation // +required

// Number of VFs to be configured
NumVfs int `json:"numVfs"`

xref https://book.kubebuilder.io/reference/markers/crd-validation

Copy link
Collaborator Author

@almaslennikov almaslennikov Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't have the omitempty attribute which in turn doesn't allow to create a template without it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is not a strict contract to the API. Additionally it is not user visible while reading the API docs or running kubectl explain. With the annotation on it the user will see it automatically

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* E.g: if `numVFs=2` then `SRIOV_EN=1` and `SRIOV_NUM_OF_VFS=2`.
* If `numVFs=0` then `SRIOV_EN=0` and `SRIOV_NUM_OF_VFS=0`.
* `linkType`: if provided configure `linkType` for the NIC for all NIC ports.
* This is a mandatory parameter.
* E.g `linkType = Infiniband` then set `LINK_TYPE_P1=IB` and `LINK_TYPE_P2=IB` if second PCI function is present
* `pciPerformanceOptimized`: performs PCI performance optimizations. If enabled then by default the following will happen:
* Set nvconfig `MAX_ACC_OUT_READ` nvconfig parameter.
Expand All @@ -121,6 +123,7 @@ spec:
* Set nvconfig `ATS_ENABLED=0`
* Can only be enabled when `pciPerformanceOptimized` is enabled
* `rawNvConfig`: a `map[string]string` which contains NVConfig parameters to apply for a NIC on all of its PFs.
* Both the numeric values and their string aliases, supported by NVConfig, are allowed (e.g. `REAL_TIME_CLOCK_ENABLE=False`, `REAL_TIME_CLOCK_ENABLE=0`).
* For per port parameters (suffix `_P1`, `_P2`) parameters with `_P2` suffix are ignored if the device is single port.

### NicDevice
Expand Down
3 changes: 3 additions & 0 deletions api/v1alpha1/nicconfigurationtemplate_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,11 @@ type NvConfigParam struct {
// ConfigurationTemplateSpec is a set of configurations for the NICs
type ConfigurationTemplateSpec struct {
// Number of VFs to be configured
// +required
NumVfs int `json:"numVfs"`
// LinkType to be configured, Ethernet|Infiniband
// +kubebuilder:validation:Enum=Ethernet;Infiniband
// +required
LinkType LinkTypeEnum `json:"linkType"`
// PCI performance optimization settings
PciPerformanceOptimized *PciPerformanceOptimizedSpec `json:"pciPerformanceOptimized,omitempty"`
Expand All @@ -99,6 +101,7 @@ type NicConfigurationTemplateSpec struct {
// NodeSelector contains labels required on the node
NodeSelector map[string]string `json:"nodeSelector,omitempty"`
// NIC selector configuration
// +required
NicSelector *NicSelectorSpec `json:"nicSelector"`
// ResetToDefault specifies whether node agent needs to perform a reset flow
// The following operations will be performed:
Expand Down
2 changes: 1 addition & 1 deletion pkg/host/configvalidation.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (v *configValidationImpl) ConstructNvParamMapFromTemplate(
if port.NetworkInterface != "" && v.utils.GetLinkType(port.NetworkInterface) != desiredLinkType {
err := types.IncorrectSpecError(
fmt.Sprintf(
"device doesn't support link type change, wrong link type provided in the template, should be: %s",
"device does not support link type change, wrong link type provided in the template, should be: %s",
v.utils.GetLinkType(port.NetworkInterface)))
log.Log.Error(err, "incorrect spec", "device", device.Name)
return desiredParameters, err
Expand Down
2 changes: 1 addition & 1 deletion pkg/host/configvalidation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ var _ = Describe("ConfigValidationImpl", func() {
defaultValues := map[string]string{}

_, err := validator.ConstructNvParamMapFromTemplate(device, defaultValues)
Expect(err).To(MatchError("incorrect spec: device doesn't support link type change, wrong link type provided in the template, should be: Ethernet"))
Expect(err).To(MatchError("incorrect spec: device does not support link type change, wrong link type provided in the template, should be: Ethernet"))
})
It("should not report an error when LinkType can be changed and template differs from the actual status", func() {
mockHostUtils.On("GetLinkType", mock.Anything).Return(consts.Ethernet)
Expand Down
Loading