Refactors the configuration options document reference #1180

dougbtv · 2023-10-24T18:12:48Z

There's some backlog in here that needs to be addressed.

This could probably use another pass in order to address the configmap as used in the thick plugin as well. However, at least this sets the stage for that change.

s1061123 · 2023-10-26T13:49:10Z

docs/configuration.md

+
+* `clusterNetwork` (string, required if not using `delegates`): default CNI network for pod-to-pod connectivity used in kubernetes cluster (Pod IP and so on). The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file.
+* `defaultNetworks` ([]string, optional): Additional / secondary network attachment that is always attached to each pod. The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file
+


Good description. Could you add one more to mention that the net-attach-def's namespace? (i.e. multusNamespace specifies the net-attach-def's namespace for clusterNetwork and defaultNetwork)

s1061123 · 2023-10-26T13:49:33Z

docs/configuration.md

-* `clusterNetwork` (string, required): default CNI network for pods, used in kubernetes cluster (Pod IP and so on): name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file
-* `defaultNetworks` ([]string, required): default CNI network attachment: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file
+* [`readinessindicatorfile`](#Default-Network-Readiness-Indicator): The path to a file whose existence denotes that the default network is ready
+message to next when some missing error. Defaults to false.
 * `systemNamespaces` ([]string, optional): list of namespaces for Kubernetes system (namespaces listed here will not have `defaultNetworks` added)
 * `multusNamespace` (string, optional): namespace for `clusterNetwork`/`defaultNetworks`


We may also add default value is 'kube-system'

s1061123 · 2023-10-26T13:51:55Z

docs/configuration.md

+
+Options:
+
+* `clusterNetwork` (string, required if not using `delegates`): default CNI network for pod-to-pod connectivity used in kubernetes cluster (Pod IP and so on). The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file.


"CNI json file name (without extension, .conf/.conflist)" might be 'name' field in CNI config file (not file name).

Note: It also applies to defaultNetwork.

Thanks, I re-read the logic and added a new section that has details of how to use this value

s1061123 · 2023-10-26T13:54:12Z

docs/configuration.md

-1. File path for CNI json confile file.
-1. Multus failed to find network. Multus raise error message
+1. `NetworkAttachmentDefinition` custom resource object for a given network name, in the namespace as specified by the `multusNamespace` configuration option.
+2. CNI json config file as found in the directory specified by the `confDir` option. The value should be without extension, e.g. .conf/.conflist. For example if you have a file named `test.conf`, you would specify the basename  `test`. The given name for `clusterNetwork` should match the value for `name` key in the config file (e.g. `"name": "test"` in "test.conf" when `"clusterNetwork": "test"`)


CNI json config file as found in the directory specified by the confDir option. The value should be without extension, e.g. .conf/.conflist.

This might not "filename". The 'name' of CNI json config. Could you please double check that?

have a new section to address this

maiqueb

Looks good.

I mostly just oppose the "pod-to-pod" term.

maiqueb · 2023-10-26T13:49:53Z

docs/configuration.md

+
+## Introduction
+
+Aside from setting options for Multus, one of the goals of configuration is to set the configuration for your *default network*. The default network is also sometimes referred as the "primary CNI plugin" or "primary network" and is the CNI plugin that is used to provide pod-to-pod connectivity in your cluster. Common examples include Flannel, Weave, Calico, Cillium, and OVN-Kubernetes, among others.


Hm, I don't know ... You can provide pod-to-pod connectivity using secondary networks.

I would say it is the plugin used to implement the kubernetes networking model.

Fair enough, it is unspecific enough here, and maybe is confusing. Thanks for the link, that's a way better way to say it.

maiqueb · 2023-10-26T13:56:32Z

docs/configuration.md

 * `systemNamespaces` ([]string, optional): list of namespaces for Kubernetes system (namespaces listed here will not have `defaultNetworks` added)
 * `multusNamespace` (string, optional): namespace for `clusterNetwork`/`defaultNetworks`
-* `delegates` ([]map,required): number of delegate details in the Multus


I don't think we should remove this attribute from here. It is still valid.

I actually just moved it to its own section called Using delegates

maiqueb · 2023-10-26T13:58:24Z

docs/configuration.md

+
+### Using clusterNetwork
+
+Using the `clusterNetwork` option and the `delegates` are mutually exclusive. If `clusterNetwork` is set, the `delegates` field is ignored. Therefore:


nit: I'd bolden the mutually exclusive part.

Suggested change

Using the `clusterNetwork` option and the `delegates` are mutually exclusive. If `clusterNetwork` is set, the `delegates` field is ignored. Therefore:

Using the `clusterNetwork` option and the `delegates` are **mutually exclusive**. If `clusterNetwork` is set, the `delegates` field is ignored. Therefore:

maiqueb · 2023-10-26T13:59:59Z

docs/configuration.md

+
+Options:
+
+* `clusterNetwork` (string, required if not using `delegates`): default CNI network for pod-to-pod connectivity used in kubernetes cluster (Pod IP and so on). The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file.


I'd replace pod-to-pod with cluster default network. We've already explained that above; from that point on, I would just "abuse" that definition.

It would look like:

Suggested change

* `clusterNetwork` (string, required if not using `delegates`): default CNI network for pod-to-pod connectivity used in kubernetes cluster (Pod IP and so on). The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file.

* `clusterNetwork` (string, required if not using `delegates`): CNI network for the kubernetes cluster default network. The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file.

thanks. yeah I'm calling it a "default CNI plugin" and reducing the other stuff

maiqueb · 2023-10-26T14:02:21Z

docs/configuration.md

+
+This expression refers to `NetworkAttachmentDefinition` custom resource names. Optionally, you could specify paths to CNI configurations. Therefore, there must be `NetworkAttachmentDefinitions` already created with the names `sidecarNetwork` and `exampleNetwork`.
+
+This means that in addition to the cluster network, each pod would be assigned two additional networks by default, and the pod would present three interfaces, e.g. `eth0`, `net1`, and `net2`, with `net1` and `net2` being set by the above described `NetworkAttachmentDefinitions`.


But if we define a pod's network selection elements, these are not applied, right ? i.e. this is the default when network selection elements are not provided.

(just wanted to clarify, since I wasn't aware of this feature 0_o ) The text is OK as is IMO.

I am going to have to look this up / test it 🤔 I didn't realize this either

actually I think it's always attached, and always in addition to the items in the selection element: https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/pkg/k8sclient/k8sclient.go#L524-L565

All good; I was simply unaware of this.

Thanks for documenting it.

maiqueb · 2023-10-26T14:05:02Z

docs/configuration.md

+
+### Using delegates
+
+If `clusterNetwork` is not set, you must use `delegates`.


nit

Suggested change

If `clusterNetwork` is not set, you must use `delegates`.

If `clusterNetwork` is not set, you **must** use `delegates`.

coveralls · 2023-12-20T13:54:30Z

coverage: 63.211% (-0.3%) from 63.468%
when pulling 22bff04 on dougbtv:configuration-options-refactor
into acfbd42 on k8snetworkplumbingwg:master.

s1061123

LGTM, merged.

s1061123 requested changes Oct 26, 2023

View reviewed changes

maiqueb reviewed Oct 26, 2023

View reviewed changes

Refactors the configuration options document reference

22bff04

dougbtv force-pushed the configuration-options-refactor branch from e40de41 to 22bff04 Compare December 20, 2023 13:49

s1061123 approved these changes Jan 4, 2024

View reviewed changes

s1061123 merged commit ab7d64e into k8snetworkplumbingwg:master Jan 4, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors the configuration options document reference #1180

Refactors the configuration options document reference #1180


		* `clusterNetwork` (string, required if not using `delegates`): default CNI network for pod-to-pod connectivity used in kubernetes cluster (Pod IP and so on). The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file.
		* `defaultNetworks` ([]string, optional): Additional / secondary network attachment that is always attached to each pod. The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file


		Options:

		* `clusterNetwork` (string, required if not using `delegates`): default CNI network for pod-to-pod connectivity used in kubernetes cluster (Pod IP and so on). The following values are valid: name of network-attachment-definition, CNI json file name (without extension, .conf/.conflist), directory for CNI config file or absolute file path for CNI config file.


		## Introduction

		Aside from setting options for Multus, one of the goals of configuration is to set the configuration for your default network. The default network is also sometimes referred as the "primary CNI plugin" or "primary network" and is the CNI plugin that is used to provide pod-to-pod connectivity in your cluster. Common examples include Flannel, Weave, Calico, Cillium, and OVN-Kubernetes, among others.


		### Using clusterNetwork

		Using the `clusterNetwork` option and the `delegates` are mutually exclusive. If `clusterNetwork` is set, the `delegates` field is ignored. Therefore:


		This expression refers to `NetworkAttachmentDefinition` custom resource names. Optionally, you could specify paths to CNI configurations. Therefore, there must be `NetworkAttachmentDefinitions` already created with the names `sidecarNetwork` and `exampleNetwork`.

		This means that in addition to the cluster network, each pod would be assigned two additional networks by default, and the pod would present three interfaces, e.g. `eth0`, `net1`, and `net2`, with `net1` and `net2` being set by the above described `NetworkAttachmentDefinitions`.


		### Using delegates

		If `clusterNetwork` is not set, you must use `delegates`.

	If `clusterNetwork` is not set, you must use `delegates`.
	If `clusterNetwork` is not set, you must use `delegates`.

Refactors the configuration options document reference #1180

Refactors the configuration options document reference #1180

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment