-
Notifications
You must be signed in to change notification settings - Fork 49
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
Support optional config #530
Conversation
dd763e1
to
871d33f
Compare
871d33f
to
fd81987
Compare
@ykulazhenkov @rollandf PTAL |
bbbaeff
to
6c8432e
Compare
@ykulazhenkov addressed your comments, PTAL |
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, thx
@@ -208,11 +212,11 @@ type IBKubernetesSpec struct { | |||
UfmSecret string `json:"ufmSecret,omitempty"` | |||
} | |||
|
|||
// NVIPAMSpec describes configuration options for nv-ipam | |||
// 1. Image information for nv-ipam | |||
// 2. Config for nv-ipam in JSON format |
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: this comment if a bit inaccurate because we specify a string representation of JSON here
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.
what exactly would you like to appear here ?
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.
maybe just 'Config for nv-ipam' is better option with a good examples and defaults. Anyway, I won't block PR for this or other nit
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.
ack will update
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.
Updated.
@@ -561,7 +561,7 @@ optionally deployed components: | |||
| `nvIpam.repository` | string | `ghcr.io/mellanox` | NVIDIA IPAM Plugin image repository | | |||
| `nvIpam.version` | string | `v0.0.3` | NVIDIA IPAM Plugin image version | | |||
| `nvIpam.imagePullSecrets` | list | `[]` | An optional list of references to secrets to use for pulling any of the Plugin image | | |||
| `nvIpam.config` | string | `''` | Network pool configuration as described in https://github.com/Mellanox/nvidia-k8s-ipam | | |||
| `nvIpam.config` | string | `{"pools": {"rdma-pool": {"subnet": "192.168.0.0/16", "perNodeBlockSize": 100, "gateway": "192.168.0.1"}}}` | Network pool configuration as described in [nvidia-k8s-ipam](https://github.com/Mellanox/nvidia-k8s-ipam), the default defines a single IP Pool named `"rdma-pool"`| |
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: JSON shouble be coverd in double quotes
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.
what do you mean ?
something like:
"{\"foo\": \"bar\"}"
dont u think it will hinder readability ?
also the value above is what we put in values.yaml as default there are no double quotes there.
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.
I mean:
"{"pools": {"rdma-pool": {"subnet": "192.168.0.0/16", "perNodeBlockSize": 100, "gateway": "192.168.0.1"}}}"
also the value above is what we put in values.yaml as default there are no double quotes there.
that's because we use pipe symbol |
thare which means multi-line string
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.
ack, ill update
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.
updated.
@@ -11,6 +11,7 @@ | |||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
{{ if .CrSpec.Config -}} |
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, add deletion of this comfigmap logic to NicClusterPolicy controller for a consistancy: if CRD is changed and config is deleted we have to delete config map
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.
There is a general mechanims at state level where resources are being deleted if needed.
do you really think we need a finer grained mechanism ?
if user sets config to nil, its expected that the next steps for him is to re-create the config map with the given name.
so it will be deleted by the user.
thinking about it, NOT deleting it may be actually better, because then the user may know the name of the config map it needs to re-create :).
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.
There is a general mechanims at state level where resources are being deleted if needed. do you really think we need a finer grained mechanism ?
Thiswon't delete ConfigMap if Multus should be deployed
if cr.Spec.SecondaryNetwork == nil || cr.Spec.SecondaryNetwork.Multus == nil {
if user sets config to nil, its expected that the next steps for him is to re-create the config map with the given name. so it will be deleted by the user.
thinking about it, NOT deleting it may be actually better, because then the user may know the name of the config map it needs to re-create :).
I don't understans this use-case when user deletes operator-generated ConfigMap and creates it's own. My assumption is that all configuration should be done via NicClusterPolicy
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.
I don't understans this use-case when user deletes operator-generated ConfigMap and creates it's own. My assumption is that all configuration should be done via NicClusterPolicy
the use-case is having configuration provided out of band. this was an internal request.
where:
- user deploys network operator and provides nicclusterpolicy with nil config for e.g nv-ipam
- at a later step when network provisioning occurs the user defines the needed config map for nv-ipam
this was the ask, in contrast to changing nicclusterpolicy at a later step which will probably mess up argo CD or smthn,
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.
the use-case is having configuration provided out of band. this was an internal request.
in this case, please add a note about this feature in a README and this will be good to merge
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.
won't delete ConfigMap if Multus should be deployed
Yes, and i think we dont need a finer grained mechansim to handle it (for now...)
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.
added a section in README.
Check [Upgrade section in Helm Chart documentation](deployment/network-operator/README.md#upgrade) for details. | ||
|
||
## Externally Provided Configurations For Network Operator Sub-Components |
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 PTAL
we can either keep this section here or move it to its own document under /docs/
as its an advanced use case.
4496082
to
2cf48d5
Compare
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 addressing my comments. It could be merged after rebase and CI passed
- Consolidate image spec with config to a common struct and use where applicable (device plugins, multus, nv-ipam) - set Config field as optional. use nil value to determine if unset. Signed-off-by: adrianc <adrianc@nvidia.com>
Run: `make generate && make manifests && make release-build` Signed-off-by: adrianc <adrianc@nvidia.com>
This is required to allow providing configurations for device plugins (RDMA, SR-IOV), nv-ipam out of band. That is, the user creates it prior to deploying these components with network operator. or alternatively restarting the relevant daemonset after config map creation. note that for multus, not providing configuration for configMap, will lead to multus using "auto" configuration same as it does today. Signed-off-by: adrianc <adrianc@nvidia.com>
Signed-off-by: adrianc <adrianc@nvidia.com>
Signed-off-by: adrianc <adrianc@nvidia.com>
Signed-off-by: adrianc <adrianc@nvidia.com>
define a single pool to be used for rdma networks to serve as default configuration. Signed-off-by: adrianc <adrianc@nvidia.com>
update README to reflect the new default for nvIpam config Signed-off-by: adrianc <adrianc@nvidia.com>
- Update project README - Update Helm README, adding quotes to default configuration of nv-ipam - Update comment in CRD Signed-off-by: adrianc <adrianc@nvidia.com>
2cf48d5
to
c4e9a99
Compare
@e0ne PTAL |
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 addressing my comments
@e0ne Feel free to merge this one when ready. |
No description provided.