network: improve the IsHost() function #14253

obnoxxx · 2024-05-22T16:58:48Z

This change is a follow-up to PR #13693 .
It is also meant to provide a continuation of the closed, stale PR #13878 .

Finally, it is intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR
#13651 .

This change improves the documentation comment and the unit testing for the IsHost() method of the CephCluster.Network spec.

The implementation of IsHost() is changed to validate the Network Spec and return error if the spec is invalid.

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
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.

BlaineEXE · 2024-05-22T17:19:14Z

One quick thing:
The commit subject overflows the hard 70-character limit, and thus also overflows the recommended 50 char limit. Please reduce the commit subject title's length. IMO, "network: improve IsHost() function" is sufficient, as docs and unit tests are reasonable assumptions for any PR. Though you're free to title it as you like as long as it's strictly under 70 chars.

BlaineEXE · 2024-05-22T17:20:01Z

go.sum

-github.com/portworx/sched-ops v0.20.4-openstorage-rc3 h1:tXnHsjZT2wZ2BCXf8avDoya7zGyCgLNUC8Upt+WEQrY=
+github.com/portworx/sched-ops v0.20.4-openstorage-rc3 h1:46EZ+vYCJ3qmQolvgDCrGuPz8Tf0vIds41RuF0dqVEw=


Why is this changing? I don't see anything that appears related in the code changes.

This also seems to be the reason why CI is failing. Here's what I see when I look at the CI output:

... verifying github.com/portworx/sched-ops@v0.20.4-openstorage-rc3: checksum mismatch downloaded: h1:tXnHsjZT2wZ2BCXf8avDoya7zGyCgLNUC8Upt+WEQrY= go.sum: h1:46EZ+vYCJ3qmQolvgDCrGuPz8Tf0vIds41RuF0dqVEw= SECURITY ERROR This download does NOT match an earlier download recorded in go.sum. The bits may have been replaced on the origin server, or an attacker may have intercepted the download attempt.

@BlaineEXE wrote:

Why is this changing? I don't see anything that appears related in the code changes.

To be honest, I am not quite certain. I was puzzled myself when I saw go.sum in the list of changed files. 🤷🏼

Here is my guess:

Earlier today, I wanted to build and unit-test locally and this was failing on go mod errors. So I tidied the dependencies and built/tested again, successfully. I assume that this has introduced the change to go.sum.

This also seems to be the reason why CI is failing.

Right, this seems plausible. Let me try and get rid of the go.sum change in the commit

Here's what I see when I look at the CI output:

... verifying github.com/portworx/sched-ops@v0.20.4-openstorage-rc3: checksum mismatch downloaded: h1:tXnHsjZT2wZ2BCXf8avDoya7zGyCgLNUC8Upt+WEQrY= go.sum: h1:46EZ+vYCJ3qmQolvgDCrGuPz8Tf0vIds41RuF0dqVEw= SECURITY ERROR This download does NOT match an earlier download recorded in go.sum. The bits may have been replaced on the origin server, or an attacker may have intercepted the download attempt.

Interestingly, when I locally revert the change to go.sum and run make cleanand go mod tidy, I get the same error:

verifying github.com/portworx/sched-ops@v0.20.4-openstorage-rc3: checksum mismatch downloaded: h1:46EZ+vYCJ3qmQolvgDCrGuPz8Tf0vIds41RuF0dqVEw= go.sum: h1:tXnHsjZT2wZ2BCXf8avDoya7zGyCgLNUC8Upt+WEQrY= SECURITY ERROR This download does NOT match an earlier download recorded in go.sum. The bits may have been replaced on the origin server, or an attacker may have intercepted the download attempt. For more information, see 'go help module-auth'.

so I am still unsure how to properly fix this.

I have undone the change to go.sum and mod-checkpasses in the ci now, but locally, make mod.checkstill fails.

@obnoxxx Can you revert this file? It seems to be causing build issues.

done. no changes to go.sumin the PR anymore, even though I can't build locally without them.

BlaineEXE · 2024-05-22T17:23:05Z

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

-		if !spec.IsMultus() && !spec.IsHost() {
+		if !spec.IsMultus() && spec.Provider != NetworkProviderHost {


I don't understand why this change is needed here.

I think that this condition no longer works correctly if the user has HostNetwork: true, because the legacy behavior is no longer also supported.

also note that the signature of IsHost()has changed to include an additional Error return

this has gone now.

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

BlaineEXE · 2024-05-22T17:25:48Z

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

@@ -670,7 +670,8 @@ func (c *Cluster) failoverMon(name string) error {
 		return errors.Wrap(err, "failed to place new mon on a node")
 	}

-	if c.spec.Network.IsHost() {
+	isHost, _ := c.spec.Network.IsHost()


We shouldn't make it a habit to ignore error return values.
Ditto for other similar changes as well.

@BlaineEXE wrote:

We shouldn't make it a habit to ignore error return values. Ditto for other similar changes as well.

ok, good point. Agreed. I will address this, but it might make the code flow more complicated and less natural.

The complication is that IsHost() returns an error, but I don't think it should. The flow will be much simpler if IsHost() only returns a bool. There can be a single check in the reconcile for valid network settings, instead of every time IsHost() is called. This will also allow you to revert a lot of changes in this PR.

@travisn wrote:

The complication is that IsHost() returns an error, but I don't think it should.

The decision to have IsHost return error on invalid network spec was a esult of the discussion between @BlaineEXE and myself in the original PR #13878
#13878 (comment)

The flow will be much simpler if IsHost() only returns a bool.

There can be a single check in the reconcile for valid network settings, instead of every time IsHost() is called.

Agreed.

This will also allow you to revert a lot of changes in this PR.

This sounds like it is desirable to have as few changes as possible in one PR.
Of course, fewer changes tend to be simpler and therefore less likely to break things. So I do see the point. 😄

While it does seem kind of sad to me to revert a major piece of the work put into this so far, I will do it as I see the logic behind the reasoning.

@travisn starting to undo the changes that add the eroor return to IsHost(), I am wondering if it would make sense to still validate the network spec in IsHost() but just issue an informational log message and return false for host network instead of erroring out. wdyt?

Since IsHost() is called from multiple locations, the log entry could easily be too verbose and repetitive. If there is an error condition to check for, I would recommend have a separate method that checks the error once during a reconcile, then IsHost() can assume there is no error condition and just return bool.

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

BlaineEXE · 2024-05-22T17:32:48Z

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

+	net := NetworkSpec{HostNetwork: false}
+
+	net.Provider = NetworkProviderHost
+	ret, err := net.IsHost()
+	assert.NoError(t, err)
+	assert.True(t, ret)
+
+	net.Provider = NetworkProviderDefault
+	net.HostNetwork = true
+	ret, err = net.IsHost()
+	assert.NoError(t, err)
+	assert.True(t, ret)


Good unit test hygiene is to make sure each test sets up its own inputs so that future test modifications don't have unintended effects. It is hard to read and modify unit tests when test inputs and side-effects are propagated from between test cases.

Please fully specify net = Network{} before each test to ensure tests are independent from each other.

obnoxxx · 2024-05-23T10:37:49Z

@BlaineEXE wrote:

One quick thing: The commit subject overflows the hard 70-character limit, and thus also overflows the recommended 50 char limit. Please reduce the commit subject title's length. IMO, "network: improve IsHost() function" is sufficient, as docs and unit tests are reasonable assumptions for any PR. Though you're free to title it as you like as long as it's strictly under 70 chars.

fixed, thanks!

parth-gr · 2024-05-23T11:27:06Z

I pulled the branch and ran make build

/home/rider/go/src/github.com/rook/rook/build/rbac
+ rm -rf /tmp/tmp.if5fFVidQV
+ popd
/home/rider/go/src/github.com/rook/rook
=== go vet
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go vet  -tags '' github.com/rook/rook/cmd/... github.com/rook/rook/pkg/... github.com/rook/rook/tests/integration
diff pkg/apis/ceph.rook.io/v1/network.go.orig pkg/apis/ceph.rook.io/v1/network.go
--- pkg/apis/ceph.rook.io/v1/network.go.orig
+++ pkg/apis/ceph.rook.io/v1/network.go
@@ -46,8 +46,8 @@
 		err = errors.Wrapf(err, "Unable to determine  host network setting from Invalid  network spec. not setting host network.")
 	} else {
 
-	ret = (n.HostNetwork && n.Provider == NetworkProviderDefault) || n.Provider == NetworkProviderHost
-}
+		ret = (n.HostNetwork && n.Provider == NetworkProviderDefault) || n.Provider == NetworkProviderHost
+	}
 	return ret, err
 }
 
make[1]: *** [build/makelib/golang.mk:162: go.fmt] Error 1
make: *** [Makefile:124: build.common] Error 2
[root@fedora rook]#

SO I think the error is coming from network API

parth-gr · 2024-05-23T11:27:47Z

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

+// which is incompatible with other network providers: HostNetwork set to true
+// together with an empty or unset network provider has the same effect as
+// network.Provider set to "host"
+func (n *NetworkSpec) IsHost() (ret bool, err error) {


I see there a lot of bugs with this implementation

func (n *NetworkSpec) IsHost() (bool, error) { ret := false err := ValidateNetworkSpec("", *n) if err != nil { return ret, errors.Wrapf(err, "Unable to determine host network setting from Invalid network spec. not setting host network.") } return (n.HostNetwork && n.Provider == NetworkProviderDefault) || n.Provider == NetworkProviderHost, err }

Fixing this make the make build worked

@parth-gr this has been fixed.

obnoxxx · 2024-05-23T14:02:17Z

@parth-gr wrote:

I pulled the branch and ran make build

/home/rider/go/src/github.com/rook/rook/build/rbac
+ rm -rf /tmp/tmp.if5fFVidQV
+ popd
/home/rider/go/src/github.com/rook/rook
=== go vet
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go vet  -tags '' github.com/rook/rook/cmd/... github.com/rook/rook/pkg/... github.com/rook/rook/tests/integration
diff pkg/apis/ceph.rook.io/v1/network.go.orig pkg/apis/ceph.rook.io/v1/network.go
--- pkg/apis/ceph.rook.io/v1/network.go.orig
+++ pkg/apis/ceph.rook.io/v1/network.go
@@ -46,8 +46,8 @@
 		err = errors.Wrapf(err, "Unable to determine  host network setting from Invalid  network spec. not setting host network.")
 	} else {
 
-	ret = (n.HostNetwork && n.Provider == NetworkProviderDefault) || n.Provider == NetworkProviderHost
-}
+		ret = (n.HostNetwork && n.Provider == NetworkProviderDefault) || n.Provider == NetworkProviderHost
+	}
 	return ret, err
 }
 
make[1]: *** [build/makelib/golang.mk:162: go.fmt] Error 1
make: *** [Makefile:124: build.common] Error 2
[root@fedora rook]#

SO I think the error is coming from network API

right, and that is code from this PR.

I see the same error locally now, but the confusing part is that this is a vet error
triggered from make build while make vetsucceeds locally for me.

And the error message shows which code is wrong but not what aspect. ...

obnoxxx · 2024-05-23T14:45:31Z

@parth-gr wrote:

I pulled the branch and ran make build

/home/rider/go/src/github.com/rook/rook/build/rbac
+ rm -rf /tmp/tmp.if5fFVidQV
+ popd
/home/rider/go/src/github.com/rook/rook
=== go vet
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go vet  -tags '' github.com/rook/rook/cmd/... github.com/rook/rook/pkg/... github.com/rook/rook/tests/integration
diff pkg/apis/ceph.rook.io/v1/network.go.orig pkg/apis/ceph.rook.io/v1/network.go
--- pkg/apis/ceph.rook.io/v1/network.go.orig
+++ pkg/apis/ceph.rook.io/v1/network.go
@@ -46,8 +46,8 @@
 		err = errors.Wrapf(err, "Unable to determine  host network setting from Invalid  network spec. not setting host network.")
 	} else {
 
-	ret = (n.HostNetwork && n.Provider == NetworkProviderDefault) || n.Provider == NetworkProviderHost
-}
+		ret = (n.HostNetwork && n.Provider == NetworkProviderDefault) || n.Provider == NetworkProviderHost
+	}
 	return ret, err
 }
 
make[1]: *** [build/makelib/golang.mk:162: go.fmt] Error 1
make: *** [Makefile:124: build.common] Error 2
[root@fedora rook]#

SO I think the error is coming from network API

right, and that is code from this PR.

I see the same error locally now, but the confusing part is that

this is a vet error triggered from make build

wrong it is in fact a gofmt error.

And the error message shows which code is wrong but not what aspect.

manually running go vetyields more information:

$ go vet pkg/apis/ceph.rook.io/v1/network.go
# command-line-arguments
vet: pkg/apis/ceph.rook.io/v1/network.go:31:10: undefined: NetworkSpec

I am surprised that it complains only about line 31: There are many more occurrences like this.

BlaineEXE · 2024-05-23T18:53:46Z

The go.sum file has been removed entirely in the commit. The build can't proceed without that

pkg/operator/ceph/cluster/mgr/spec.go

obnoxxx · 2024-05-27T08:17:51Z

@BlaineEXE wrote:

The go.sum file has been removed entirely in the commit. The build can't proceed without that

interesting.

For me, locally, the build would not proceed or even succeed with the checked-in go.sum file
It always produced a go module checksum mismatch error.
I had to remove the go.sum file and run go mod tidyand a few more commands before I could build locally. That's why I committed the removal of the file.

Now I was able to fix the problem locally and build again. I committed the resulting diff and updated the PR.

BlaineEXE · 2024-05-28T21:45:40Z

pkg/operator/ceph/file/mds/spec.go

-			HostNetwork:        c.clusterSpec.Network.IsHost(),
+			HostNetwork:        useHost,


If IsHost() is being reverted to only return a bool (even when there are configuration inconsistencies that Rook has to guess about), I don't think it makes sense to keep these changes that add useHost := ...IsHost(), as it's just code churn with no real changes. These should probably be reverted also.

At that point, the focus of the changes is better unit testing and docs, which is still valuable.

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

travisn · 2024-05-29T19:47:13Z

pkg/apis/ceph.rook.io/v1/zz_generated.deepcopy.go

@@ -1186,6 +1186,11 @@ func (in *CephFilesystemSubVolumeGroupList) DeepCopyObject() runtime.Object {
 func (in *CephFilesystemSubVolumeGroupSpec) DeepCopyInto(out *CephFilesystemSubVolumeGroupSpec) {
 	*out = *in
 	in.Pinning.DeepCopyInto(&out.Pinning)
+	if in.Quota != nil {


These changes under pkg/apis actually are from master, not needed for your changes to IsHost(). I'd suggest just add these generated files in a separate commit in this PR. It seems our CI action is not working to catch that this generated code is out of date.

travisn · 2024-05-29T19:47:51Z

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

@@ -670,7 +670,7 @@ func (c *Cluster) failoverMon(name string) error {
 		return errors.Wrap(err, "failed to place new mon on a node")
 	}

-	if c.spec.Network.IsHost() {
+	if  c.spec.Network.IsHost() {


revert this space, note that vscode would automatically remove these extra spaces for you, so you don't have to think about whitespace much in golang.

done (manually 😉 )

travisn · 2024-05-29T19:47:55Z

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

@@ -189,7 +189,7 @@ func (c *Cluster) Start(clusterInfo *cephclient.ClusterInfo, rookImage string, c
 	c.spec = spec

 	// fail if we were instructed to deploy more than one mon on the same machine with host networking
-	if c.spec.Network.IsHost() && c.spec.Mon.AllowMultiplePerNode && c.spec.Mon.Count > 1 {
+	if c.spec.Network.IsHost()  && c.spec.Mon.AllowMultiplePerNode && c.spec.Mon.Count > 1 {


revert this space

travisn · 2024-05-29T19:48:01Z

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

@@ -923,7 +923,7 @@ func (c *Cluster) assignMons(mons []*monConfig) error {
 			// placement is not being made. otherwise, the node choice will map
 			// directly to a node selector on the monitor pod.
 			var schedule *controller.MonScheduleInfo
-			if c.spec.Network.IsHost() || c.monVolumeClaimTemplate(mon) == nil {
+			if c.spec.Network.IsHost()  || c.monVolumeClaimTemplate(mon) == nil {


revert this space

travisn · 2024-05-29T19:48:09Z

pkg/operator/ceph/cluster/nodedaemon/pruner.go

@@ -108,7 +108,7 @@ func (r *ReconcileNode) createOrUpdateCephCron(cephCluster cephv1.CephCluster, c
 				getCrashPruneContainer(cephCluster, *cephVersion),
 			},
 			RestartPolicy:      corev1.RestartPolicyNever,
-			HostNetwork:        cephCluster.Spec.Network.IsHost(),
+			HostNetwork:       cephCluster.Spec.Network.IsHost(),


revert this space

travisn · 2024-05-29T19:48:17Z

pkg/operator/ceph/file/mds/spec.go

@@ -136,7 +135,7 @@ func (c *Cluster) makeMdsDaemonContainer(mdsConfig *mdsConfig, fsName string) v1
 		"--foreground",
 	)

-	if !c.clusterSpec.Network.IsHost() && !c.clusterSpec.Network.IsMultus() {
+	if !c.clusterSpec.Network.IsHost()  && !c.clusterSpec.Network.IsMultus() {


revert this space

travisn · 2024-05-29T19:48:34Z

pkg/operator/ceph/object/spec_test.go

@@ -707,7 +707,8 @@ func TestMakeRGWPodSpec(t *testing.T) {
 			if tt.isSet {
 				assert.Equal(t, *c.store.Spec.Gateway.HostNetwork, podTemplateSpec.Spec.HostNetwork)
 			} else {
-				assert.Equal(t, c.clusterSpec.Network.IsHost(), podTemplateSpec.Spec.HostNetwork)
+				isHost := c.clusterSpec.Network.IsHost()
+				assert.Equal(t, isHost, podTemplateSpec.Spec.HostNetwork)


you can revert this file as well

obnoxxx

addressed a few review requests and updated.

obnoxxx · 2024-05-23T18:33:34Z

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

+	net := NetworkSpec{HostNetwork: false}
+
+	net.Provider = NetworkProviderHost
+	ret, err := net.IsHost()
+	assert.NoError(t, err)
+	assert.True(t, ret)
+
+	net.Provider = NetworkProviderDefault
+	net.HostNetwork = true
+	ret, err = net.IsHost()
+	assert.NoError(t, err)
+	assert.True(t, ret)


obnoxxx · 2024-05-23T18:34:34Z

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

@@ -670,7 +670,8 @@ func (c *Cluster) failoverMon(name string) error {
 		return errors.Wrap(err, "failed to place new mon on a node")
 	}

-	if c.spec.Network.IsHost() {
+	isHost, _ := c.spec.Network.IsHost()


obnoxxx · 2024-05-29T10:19:56Z

pkg/operator/ceph/file/mds/spec.go

-			HostNetwork:        c.clusterSpec.Network.IsHost(),
+			HostNetwork:        useHost,


obnoxxx · 2024-05-29T11:34:58Z

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

-		if !spec.IsMultus() && !spec.IsHost() {
+		if !spec.IsMultus() && spec.Provider != NetworkProviderHost {


this has gone now.

obnoxxx · 2024-05-29T18:21:03Z

pkg/operator/ceph/file/mds/spec.go

-			HostNetwork:        c.clusterSpec.Network.IsHost(),
+			HostNetwork:        useHost,


obnoxxx · 2024-05-30T09:57:31Z

pkg/operator/ceph/cluster/nodedaemon/pruner.go

@@ -108,7 +108,7 @@ func (r *ReconcileNode) createOrUpdateCephCron(cephCluster cephv1.CephCluster, c
 				getCrashPruneContainer(cephCluster, *cephVersion),
 			},
 			RestartPolicy:      corev1.RestartPolicyNever,
-			HostNetwork:        cephCluster.Spec.Network.IsHost(),
+			HostNetwork:       cephCluster.Spec.Network.IsHost(),


obnoxxx · 2024-05-30T09:59:04Z

pkg/operator/ceph/file/mds/spec.go

@@ -136,7 +135,7 @@ func (c *Cluster) makeMdsDaemonContainer(mdsConfig *mdsConfig, fsName string) v1
 		"--foreground",
 	)

-	if !c.clusterSpec.Network.IsHost() && !c.clusterSpec.Network.IsMultus() {
+	if !c.clusterSpec.Network.IsHost()  && !c.clusterSpec.Network.IsMultus() {


obnoxxx · 2024-05-30T10:03:19Z

pkg/operator/ceph/object/spec_test.go

@@ -707,7 +707,8 @@ func TestMakeRGWPodSpec(t *testing.T) {
 			if tt.isSet {
 				assert.Equal(t, *c.store.Spec.Gateway.HostNetwork, podTemplateSpec.Spec.HostNetwork)
 			} else {
-				assert.Equal(t, c.clusterSpec.Network.IsHost(), podTemplateSpec.Spec.HostNetwork)
+				isHost := c.clusterSpec.Network.IsHost()
+				assert.Equal(t, isHost, podTemplateSpec.Spec.HostNetwork)


obnoxxx · 2024-05-30T10:08:17Z

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

@@ -670,7 +670,7 @@ func (c *Cluster) failoverMon(name string) error {
 		return errors.Wrap(err, "failed to place new mon on a node")
 	}

-	if c.spec.Network.IsHost() {
+	if  c.spec.Network.IsHost() {


done (manually 😉 )

obnoxxx · 2024-05-30T10:09:22Z

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

@@ -189,7 +189,7 @@ func (c *Cluster) Start(clusterInfo *cephclient.ClusterInfo, rookImage string, c
 	c.spec = spec

 	// fail if we were instructed to deploy more than one mon on the same machine with host networking
-	if c.spec.Network.IsHost() && c.spec.Mon.AllowMultiplePerNode && c.spec.Mon.Count > 1 {
+	if c.spec.Network.IsHost()  && c.spec.Mon.AllowMultiplePerNode && c.spec.Mon.Count > 1 {


travisn

LGTM, just a couple small whitespace changes that could be reverted if desired

travisn · 2024-05-30T13:47:11Z

pkg/operator/ceph/controller/network.go

@@ -49,7 +49,6 @@ func ApplyCephNetworkSettings(
 	clusterInfo *cephclient.ClusterInfo,
 ) error {
 	netSpec := clusterSpec.Network
-


could revert

travisn · 2024-05-30T13:47:21Z

pkg/operator/ceph/file/mds/spec.go

@@ -101,7 +101,6 @@ func (c *Cluster) makeDeployment(mdsConfig *mdsConfig, fsNamespacedname types.Na
 			},
 		},
 	}
-


could revert

obnoxxx · 2024-05-31T09:17:12Z

@travisn wrote:

LGTM, just a couple small whitespace changes that could be reverted if desired

Thanks! I addressed these

This change is a follow-up to PR rook#13651 . It is also meant to provide a continuation of the closed, stale PR rook#13878 . It is furthermore intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation, implementation, and unit testing for the method of the spec. Signed-off-by: Michael Adam <obnox@samba.org> Co-authored-by: Blaine Gardner <b.blaine.gardner@gmail.com> Co-authored-by: Travis Nielsen <tnielsen@redhat.com>

feedback addressed

obnoxxx requested review from BlaineEXE and parth-gr May 22, 2024 16:58

obnoxxx mentioned this pull request May 22, 2024

network: Improve IsHost implementation, documentation, and unit testing #13878

Closed

6 tasks

obnoxxx requested a review from travisn May 22, 2024 17:06

BlaineEXE requested changes May 22, 2024

View reviewed changes

obnoxxx force-pushed the improve-is-host branch from c8c79b3 to 27b0043 Compare May 23, 2024 10:31

obnoxxx changed the title ~~network: improve implementation, documentation, and unit testing of IsHost()~~ network: improve theIsHost() function May 23, 2024

parth-gr previously requested changes May 23, 2024

View reviewed changes

obnoxxx changed the title ~~network: improve theIsHost() function~~ network: improve the IsHost() function May 23, 2024

obnoxxx force-pushed the improve-is-host branch from 27b0043 to 54d5e78 Compare May 23, 2024 18:33

travisn requested changes May 23, 2024

View reviewed changes

pkg/operator/ceph/cluster/mgr/spec.go Outdated Show resolved Hide resolved

pkg/operator/ceph/cluster/mgr/spec.go Outdated Show resolved Hide resolved

obnoxxx force-pushed the improve-is-host branch 2 times, most recently from 896de79 to 3576ae3 Compare May 27, 2024 08:12

BlaineEXE reviewed May 28, 2024

View reviewed changes

obnoxxx force-pushed the improve-is-host branch 4 times, most recently from f1293d1 to 8b6f486 Compare May 29, 2024 19:35

travisn requested changes May 29, 2024

View reviewed changes

obnoxxx force-pushed the improve-is-host branch 4 times, most recently from c425cae to e4ba750 Compare May 30, 2024 10:03

obnoxxx force-pushed the improve-is-host branch 2 times, most recently from 81f99d5 to 7e09856 Compare May 30, 2024 10:09

obnoxxx commented May 30, 2024

View reviewed changes

obnoxxx force-pushed the improve-is-host branch 2 times, most recently from 6b23ceb to f6a3cd0 Compare May 30, 2024 10:29

travisn approved these changes May 30, 2024

View reviewed changes

obnoxxx force-pushed the improve-is-host branch from f6a3cd0 to 0444a87 Compare May 31, 2024 09:14

obnoxxx force-pushed the improve-is-host branch from 0444a87 to f7f12ec Compare May 31, 2024 10:54

obnoxxx requested review from parth-gr and BlaineEXE May 31, 2024 12:34

BlaineEXE approved these changes May 31, 2024

View reviewed changes

BlaineEXE merged commit ed2d110 into rook:master May 31, 2024
53 checks passed

obnoxxx deleted the improve-is-host branch June 3, 2024 09:55

		github.com/portworx/sched-ops v0.20.4-openstorage-rc3 h1:tXnHsjZT2wZ2BCXf8avDoya7zGyCgLNUC8Upt+WEQrY=
		github.com/portworx/sched-ops v0.20.4-openstorage-rc3 h1:46EZ+vYCJ3qmQolvgDCrGuPz8Tf0vIds41RuF0dqVEw=

		if !spec.IsMultus() && !spec.IsHost() {
		if !spec.IsMultus() && spec.Provider != NetworkProviderHost {

		HostNetwork: c.clusterSpec.Network.IsHost(),
		HostNetwork: useHost,

network: improve the IsHost() function #14253

network: improve the IsHost() function #14253

Conversation

obnoxxx commented May 22, 2024 • edited by BlaineEXE

BlaineEXE commented May 22, 2024

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

obnoxxx May 23, 2024 • edited

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

obnoxxx commented May 23, 2024

parth-gr commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obnoxxx commented May 23, 2024

obnoxxx commented May 23, 2024

BlaineEXE commented May 23, 2024

obnoxxx commented May 27, 2024

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

obnoxxx 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

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

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

obnoxxx commented May 31, 2024

obnoxxx commented May 22, 2024 •

edited by BlaineEXE

obnoxxx May 23, 2024 •

edited