multus: allow using NADs without inspectable CIDRs #12778

BlaineEXE · 2023-08-22T20:29:11Z

Description of your changes:

Change how Rook detects network CIDRs for Multus networks. The IPAM
configuration is only defined as an arbitrary string JSON blob with a
"type" field and nothing more. Rook's detection of CIDRs for whereabouts
had already grown out of date since the initial implementation.
Additionally, Rook did not support DHCP IPAM, which is a reasonable
choice for users. And more, Rook did not support CNI plugin chaining,
which further complicates NADs. Based on the CNI spec, network chaning
can result in any changes to network CIDRs from the first-given plugin.

All these problems make it more and more difficult for Rook to support
Multus by inspecting the NAD itself to predict network CIDRs. Instead,
it is better for Rook to treat the CNI process as a black box. To
preserve legacy functionality of auto-detecting networks and to make
that as robust as possible, change to a canary-style architecture like
that used for Ceph mons, from which Rook will detect the network CIDRs
if possible.

Also allow users to specify overrides for CIDR ranges. This allows Rook
to still support esoteric and unexpected NAD or network configurations
where a CIDR range is not detectable or where the range detected would
be incomplete. Because it may be impossible for Rook to understand the
network CIDRs wholistically while residing only on a portion of the
network, this feature should have been present from Multus's inception.

Improving CIDR auto-detection and allowing users to specify overrides
for auto-detected CIDRs rounds out Rook's Multus support for CephCluster
(core/RADOS) installations. No further architectural changes should be
needed for CephClusters as regards application of public/cluster network
CIDRs for Multus networks.

Signed-off-by: Blaine Gardner blaine.gardner@ibm.com

Which issue is resolved by this Pull Request:
Resolves #12459

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

pkg/apis/ceph.rook.io/v1/types.go

pkg/apis/ceph.rook.io/v1/cluster.go

pkg/apis/ceph.rook.io/v1/types.go

pkg/apis/ceph.rook.io/v1/network.go

pkg/apis/ceph.rook.io/v1/types.go

pkg/operator/k8sutil/network.go

pkg/apis/ceph.rook.io/v1/network.go

pkg/apis/ceph.rook.io/v1/types.go

BlaineEXE · 2023-08-31T23:59:05Z

deploy/examples/cluster-test.yaml

+  network:
+    provider: "multus"
+    selectors:
+      public: public-net


Note to self: TODO: remove this

BlaineEXE · 2023-09-01T18:57:03Z

This is ready for review, but I am adding do not merge label while I test this for DHCP networks. Results of this testing may inform some doc or minor code changes.

BlaineEXE · 2023-09-01T18:59:53Z

pkg/operator/ceph/cluster/mon/mon_test.go

-		rookVersion: "myversion",
+		rookImage:   "myversion",


These changes may not be technically necessary, but I wasted at least 5 hours adding the operator config to this struct and all of the code places where it is called by parents before I realized that rookVersion was being used to hold the rook image string. I'd rather this not happen to anyone else.

Agreed, I have been confused by this as well

BlaineEXE · 2023-09-01T19:01:07Z

pkg/operator/ceph/controller/network.go

+	// This job is complex to overcome 2 specific limitations:
+	// - CNI/Multus' `k8s.v1.cni.cncf.io/network-status` annotation only reports IP addrs, not CIDRs
+	// - `ip addr show` in the pod shows CIDRs but also contains extra IPv6 SLAAC addrs in addition
+	//   to the addr(s) attached by CNI/Multus
+	// Rook must cross-reference both pieces of info to accurately read the CIDR(s) for the net.
+	// Both pieces of info must come from the same Pod+Container in order to be cross-ref'd.
+	// Use downward API to allow the cmd reporter job to report both pieces of info at the same time


I suggest reading this before reviewing.

BlaineEXE · 2023-09-01T19:01:15Z

pkg/operator/ceph/controller/network.go

+	// TODO: do we need resource requests/requirements? it's just sleeping
+	// ... if needed, maybe logcollector, with small requirements would be a good option?


@travisn thoughts?

Some environments are strict about requiring resource requests/limits on all pods, so we at least need an option. The requirements are going to be very low. I'd suggest we define a new category of resources for this in the cluster CR, though by default I'd say we don't need to set them.

I removed this todo without resolving it in this PR. It looks like the cmd reporter doesn't set any resource requests/requirements at all, and it looks like DetectCephVersion() also doesn't set any resources. Can we instead take an item to handle resources for the cmd-reporter as a general work item? I think it'll bloat this more, and it seems like we don't have anyone with any big problems around this today.

Oh interesting, if the cmd reporter already doesn't set requests/requirements, then we can skip it for now.

pkg/operator/ceph/config/config.go

Documentation/CRDs/Cluster/ceph-cluster-crd.md

subhamkrai

Some issue with crds, maybe try make crds again?

travisn · 2023-09-05T20:56:53Z

The multus cluster test is failing:

+ sed -i 's|#deviceFilter:|deviceFilter: sdb|g' cluster-multus-test.yaml
sed: can't read cluster-multus-test.yaml: No such file or directory
Error: Process completed with exit code 2.

travisn

Looks great overall, just some minor comments and questions.

Documentation/CRDs/Cluster/ceph-cluster-crd.md

travisn · 2023-09-05T21:14:21Z

pkg/operator/ceph/controller/network.go

+	// TODO: do we need resource requests/requirements? it's just sleeping
+	// ... if needed, maybe logcollector, with small requirements would be a good option?


Some environments are strict about requiring resource requests/limits on all pods, so we at least need an option. The requirements are going to be very low. I'd suggest we define a new category of resources for this in the cluster CR, though by default I'd say we don't need to set them.

travisn · 2023-09-05T21:17:53Z

pkg/operator/ceph/controller/network.go

+					}}}}}}
+	mnt := corev1.VolumeMount{
+		Name:      "network-status",
+		MountPath: "/tmp",


nit: What about a path like this? Maybe I just think of /tmp as a scratch folder for someone connecting to the pod.

Suggested change

MountPath: "/tmp",

MountPath: "/var/lib/rook/multus",

travisn · 2023-09-05T21:27:26Z

deploy/examples/cluster-multus-test.yaml

@@ -1,49 +0,0 @@
-#################################################################################################################


The only multus examples are in the docs now? What about updating this example to match the doc example?

This was an accidental deletion. The file should be back now, but let me know if you see it disappear again. I have been having trouble getting it to stay put for some reason

Documentation/CRDs/Cluster/ceph-cluster-crd.md

pkg/apis/ceph.rook.io/v1/network.go

pkg/apis/ceph.rook.io/v1/types.go

Documentation/CRDs/Cluster/ceph-cluster-crd.md

travisn · 2023-09-05T21:39:50Z

pkg/apis/ceph.rook.io/v1/types.go

+// A list of CIDRs.
+type CIDRList []CIDR
+
+// ^ Note on kubebuilder:validation:Pattern above:


Can the note be placed next to the pattern above? It seems disconnected to be several lines below.

travisn · 2023-09-05T21:40:35Z

pkg/operator/ceph/cluster/mon/mon_test.go

-		rookVersion: "myversion",
+		rookImage:   "myversion",


Agreed, I have been confused by this as well

BlaineEXE · 2023-09-05T23:10:38Z

Working to address feedback now, but I was able to confirm that the new auto-detection method is successfully able to get a valid CIDR when the DHCP IPAM is used. 🎉
I am not sure how to set this up in CI. I borrowed someone's home kube lab for the day to validate that.

subhamkrai

@BlaineEXE multus ci is failing because

pod/rook-ceph-network-cluster-canary-mgsqm          0/1     Init:1/2   0          6m18s   10.244.246.151   fv-az1031-835   <none>           <none>
pod/rook-ceph-network-public-canary-vk4nr           0/1     Init:1/2   0          6m18s   10.244.246.150   fv-az1031-835   <none>           <none>

 status:
    conditions:
    - lastProbeTime: null
      lastTransitionTime: "2023-09-06T00:15:59Z"
      message: 'containers with incomplete status: [wait-for-network-status-annotation]'
      reason: ContainersNotInitialized
      status: "False"
      type: Initialized

travisn

The CI is looking good, just reran a few tests to confirm they're intermittent. Good to see the multus test passing. I'm ready to approve assuming manual testing is completed.

travisn · 2023-09-06T22:25:01Z

Documentation/Storage-Configuration/Advanced/ceph-configuration.md

@@ -363,12 +363,17 @@ ceph osd primary-affinity osd.0 0

 ## OSD Dedicated Network

+!!! outdated


outdated is one of the rendered keywords? I'm not even sure where the list is defined.

Good point. It looks like these are the ones we can use: https://squidfunk.github.io/mkdocs-material/reference/admonitions/

I think info, tip (more catching icon than info), or warning could be good choices.

subhamkrai

I'm putting LGTM, I had a few questions that have been addressed. Thanks

Change how Rook detects network CIDRs for Multus networks. The IPAM configuration is only defined as an arbitrary string JSON blob with a "type" field and nothing more. Rook's detection of CIDRs for whereabouts had already grown out of date since the initial implementation. Additionally, Rook did not support DHCP IPAM, which is a reasonable choice for users. And more, Rook did not support CNI plugin chaining, which further complicates NADs. Based on the CNI spec, network chaning can result in any changes to network CIDRs from the first-given plugin. All these problems make it more and more difficult for Rook to support Multus by inspecting the NAD itself to predict network CIDRs. Instead, it is better for Rook to treat the CNI process as a black box. To preserve legacy functionality of auto-detecting networks and to make that as robust as possible, change to a canary-style architecture like that used for Ceph mons, from which Rook will detect the network CIDRs if possible. Also allow users to specify overrides for CIDR ranges. This allows Rook to still support esoteric and unexpected NAD or network configurations where a CIDR range is not detectable or where the range detected would be incomplete. Because it may be impossible for Rook to understand the network CIDRs wholistically while residing only on a portion of the network, this feature should have been present from Multus's inception. Improving CIDR auto-detection and allowing users to specify overrides for auto-detected CIDRs rounds out Rook's Multus support for CephCluster (core/RADOS) installations. No further architectural changes should be needed for CephClusters as regards application of public/cluster network CIDRs for Multus networks. Signed-off-by: Blaine Gardner <blaine.gardner@ibm.com>

multus: allow using NADs without inspectable CIDRs (backport #12778)

BlaineEXE requested a review from travisn August 22, 2023 20:29

BlaineEXE commented Aug 22, 2023

View reviewed changes

pkg/apis/ceph.rook.io/v1/types.go Show resolved Hide resolved

BlaineEXE commented Aug 22, 2023

View reviewed changes

pkg/apis/ceph.rook.io/v1/cluster.go Outdated Show resolved Hide resolved

travisn reviewed Aug 22, 2023

View reviewed changes

pkg/apis/ceph.rook.io/v1/types.go Show resolved Hide resolved

pkg/apis/ceph.rook.io/v1/network.go Show resolved Hide resolved

pkg/apis/ceph.rook.io/v1/types.go Outdated Show resolved Hide resolved

BlaineEXE commented Aug 23, 2023

View reviewed changes

pkg/operator/k8sutil/network.go Outdated Show resolved Hide resolved

subhamkrai reviewed Aug 24, 2023

View reviewed changes

pkg/apis/ceph.rook.io/v1/network.go Show resolved Hide resolved

subhamkrai reviewed Aug 24, 2023

View reviewed changes

pkg/apis/ceph.rook.io/v1/types.go Outdated Show resolved Hide resolved

BlaineEXE commented Aug 31, 2023

View reviewed changes

BlaineEXE force-pushed the multus-allow-cidr-spec branch 2 times, most recently from 8b8ee8e to d9e543e Compare September 1, 2023 18:54

BlaineEXE requested review from travisn and subhamkrai September 1, 2023 18:55

BlaineEXE marked this pull request as ready for review September 1, 2023 18:56

BlaineEXE added the do-not-merge DO NOT MERGE :) label Sep 1, 2023

BlaineEXE commented Sep 1, 2023

View reviewed changes

pkg/operator/ceph/config/config.go Show resolved Hide resolved

BlaineEXE force-pushed the multus-allow-cidr-spec branch 5 times, most recently from 3220bee to 48176d5 Compare September 1, 2023 20:57

subhamkrai reviewed Sep 4, 2023

View reviewed changes

Documentation/CRDs/Cluster/ceph-cluster-crd.md Outdated Show resolved Hide resolved

subhamkrai reviewed Sep 4, 2023

View reviewed changes

Documentation/CRDs/Cluster/ceph-cluster-crd.md Outdated Show resolved Hide resolved

subhamkrai requested changes Sep 4, 2023

View reviewed changes

BlaineEXE force-pushed the multus-allow-cidr-spec branch 2 times, most recently from f05f38d to c898c3b Compare September 5, 2023 15:18

travisn requested changes Sep 5, 2023

View reviewed changes

BlaineEXE force-pushed the multus-allow-cidr-spec branch from c898c3b to de88eda Compare September 5, 2023 22:16

BlaineEXE force-pushed the multus-allow-cidr-spec branch from de88eda to f5acbf1 Compare September 5, 2023 23:58

subhamkrai reviewed Sep 6, 2023

View reviewed changes

BlaineEXE force-pushed the multus-allow-cidr-spec branch 2 times, most recently from 5d4a322 to 0ba1578 Compare September 6, 2023 16:30

travisn reviewed Sep 6, 2023

View reviewed changes

subhamkrai approved these changes Sep 7, 2023

View reviewed changes

BlaineEXE force-pushed the multus-allow-cidr-spec branch from 0ba1578 to 3c43268 Compare September 7, 2023 16:13

travisn approved these changes Sep 7, 2023

View reviewed changes

BlaineEXE added backport-release-1.12 and removed do-not-merge DO NOT MERGE :) labels Sep 7, 2023

BlaineEXE merged commit 17f0072 into rook:master Sep 7, 2023
48 of 50 checks passed

mergify bot mentioned this pull request Sep 7, 2023

multus: allow using NADs without inspectable CIDRs (backport #12778) #12866

Merged

BlaineEXE deleted the multus-allow-cidr-spec branch September 7, 2023 19:57

BlaineEXE added a commit that referenced this pull request Sep 7, 2023

Merge pull request #12866 from rook/mergify/bp/release-1.12/pr-12778

522b49b

multus: allow using NADs without inspectable CIDRs (backport #12778)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multus: allow using NADs without inspectable CIDRs #12778

multus: allow using NADs without inspectable CIDRs #12778

BlaineEXE commented Aug 22, 2023 •

edited

BlaineEXE Aug 31, 2023

BlaineEXE commented Sep 1, 2023

BlaineEXE Sep 1, 2023

travisn Sep 5, 2023

BlaineEXE Sep 1, 2023

BlaineEXE Sep 1, 2023

travisn Sep 5, 2023

BlaineEXE Sep 5, 2023

travisn Sep 6, 2023

subhamkrai left a comment

travisn commented Sep 5, 2023

travisn left a comment

travisn Sep 5, 2023

travisn Sep 5, 2023

travisn Sep 5, 2023

BlaineEXE Sep 5, 2023

travisn Sep 5, 2023

travisn Sep 5, 2023

BlaineEXE commented Sep 5, 2023

subhamkrai left a comment

travisn left a comment

travisn Sep 6, 2023

BlaineEXE Sep 7, 2023 •

edited

subhamkrai left a comment

		// TODO: do we need resource requests/requirements? it's just sleeping
		// ... if needed, maybe logcollector, with small requirements would be a good option?

		@@ -1,49 +0,0 @@
		#################################################################################################################

		@@ -363,12 +363,17 @@ ceph osd primary-affinity osd.0 0

		## OSD Dedicated Network

		!!! outdated

multus: allow using NADs without inspectable CIDRs #12778

multus: allow using NADs without inspectable CIDRs #12778

Conversation

BlaineEXE commented Aug 22, 2023 • edited

Choose a reason for hiding this comment

BlaineEXE commented Sep 1, 2023

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

subhamkrai left a comment

Choose a reason for hiding this comment

travisn commented Sep 5, 2023

travisn left a 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

BlaineEXE commented Sep 5, 2023

subhamkrai left a comment

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE Sep 7, 2023 • edited

Choose a reason for hiding this comment

subhamkrai left a comment

Choose a reason for hiding this comment

BlaineEXE commented Aug 22, 2023 •

edited

BlaineEXE Sep 7, 2023 •

edited