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

Refactors the configuration options document reference #1180

Merged

Conversation

dougbtv
Copy link
Member

@dougbtv dougbtv commented Oct 24, 2023

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.


* `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

Copy link
Member

Choose a reason for hiding this comment

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

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)

* `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`
Copy link
Member

Choose a reason for hiding this comment

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

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


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.
Copy link
Member

Choose a reason for hiding this comment

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

"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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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"`)
Copy link
Member

Choose a reason for hiding this comment

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

  1. 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

have a new section to address this

Copy link
Collaborator

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

Looks good.

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


## 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

* `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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


### Using clusterNetwork

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

Choose a reason for hiding this comment

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

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:


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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


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`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

All good; I was simply unaware of this.

Thanks for documenting it.


### Using delegates

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

Choose a reason for hiding this comment

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

nit

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

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

Coverage Status

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

Copy link
Member

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

LGTM, merged.

@s1061123 s1061123 merged commit ab7d64e into k8snetworkplumbingwg:master Jan 4, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants