Bug 1880902: automatically set the dnsPolicy with ClusterFirstWithHostNet when endpoint publishing strategy type is set to hostnetwork #492

miheer · 2020-11-17T12:47:57Z

Bug 1880902 - If the endpoint publishing strategy type is set to hostnetwork then
we should automatically set the dnsPolicy with ClusterFirstWithHostNet.

pkg/operator/controller/ingress/deployment.go

Miciah · 2020-11-17T17:40:58Z

We should add a unit test in TestDeploymentConfigChanged. Also, we probably should add deployment.Spec.Template.Spec.DNSPolicy = corev1.DNSClusterFirst for the non-HostNetworking case so we always set an explicit policy, which will prevent the comparison in deploymentConfigChanged from getting confused when the API sets a default value.

miheer · 2020-11-18T03:35:40Z

@Miciah PTAL

sgreene570

some small comments

sgreene570 · 2020-11-18T14:55:46Z

pkg/operator/controller/ingress/deployment_test.go

@@ -38,6 +38,8 @@ func checkDeploymentHasEnvVar(t *testing.T, deployment *appsv1.Deployment, name
 		actualValue string
 		foundValue  bool
 	)
+	//dnsPolicy := deployment.Spec.Template.Spec.DNSPolicy


Is this comment necessary?

sgreene570 · 2020-11-18T14:56:35Z

pkg/operator/controller/ingress/deployment_test.go

@@ -229,6 +231,10 @@ func TestDesiredRouterDeployment(t *testing.T) {
 		t.Error("expected host network to be false")
 	}

+	if deployment.Spec.Template.Spec.DNSPolicy != corev1.DNSClusterFirst {
+		t.Errorf("expected dnsPolicy to be #{corev1.DNSClusterFirst}")


Let's make the new t.Errorf(...) calls more verbose somehow.
ie:

Suggested change

t.Errorf("expected dnsPolicy to be #{corev1.DNSClusterFirst}")

t.Errorf("expected dnsPolicy to be %q, got %q", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)

sgreene570 · 2020-11-18T15:03:51Z

pkg/operator/controller/ingress/deployment_test.go

@@ -901,6 +930,7 @@ func TestDeploymentConfigChanged(t *testing.T) {
 						},
 					},
 					Spec: corev1.PodSpec{
+						DNSPolicy: corev1.DNSClusterFirstWithHostNet,


Would it make more sense to set the test deployment's DNSPolicy to corev1.DNSClusterFirst since that's the API defaulted value (and then mutate that value to corev1.DNSClusterFirstWithHostNet in the new test cases)?

miheer · 2020-11-23T09:35:14Z

After the changes -> Unit tests are passing

[miheer@miheer cluster-ingress-operator]$ GO111MODULE=on go test -v -run TestDesiredRouterDeployment ./pkg/operator/controller/ingress/
=== RUN TestDesiredRouterDeployment
--- PASS: TestDesiredRouterDeployment (0.01s)
PASS
ok github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress 0.017s
[miheer@miheer cluster-ingress-operator]$ GO111MODULE=on go test -v -run TestDeploymentConfigChanged ./pkg/operator/controller/ingress/
=== RUN TestDeploymentConfigChanged
--- PASS: TestDeploymentConfigChanged (0.01s)
PASS
ok github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress (cached)
[miheer@miheer cluster-ingress-operator]$

sgreene570 · 2020-11-23T15:01:24Z

pkg/operator/controller/ingress/deployment_test.go

+		},
+
+		{
+			description: "if dnsPolicy is not changed",


the descriptions for the 2 test cases you added are inaccurate, I believe

oh yes I will add them...just switched the code.

@sgreene570 done

sgreene570 · 2020-11-24T14:53:05Z

pkg/operator/controller/ingress/deployment_test.go

@@ -229,6 +229,10 @@ func TestDesiredRouterDeployment(t *testing.T) {
 		t.Error("expected host network to be false")
 	}

+	if deployment.Spec.Template.Spec.DNSPolicy != corev1.DNSClusterFirst {
+		t.Errorf("expected dnsPolicy to be %q, got %q", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)


Suggested change

t.Errorf("expected dnsPolicy to be %q, got %q", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)

t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)

sgreene570 · 2020-11-24T14:53:23Z

pkg/operator/controller/ingress/deployment_test.go

@@ -307,6 +311,9 @@ func TestDesiredRouterDeployment(t *testing.T) {
 	if deployment.Spec.Template.Spec.HostNetwork != false {
 		t.Error("expected host network to be false")
 	}
+	if deployment.Spec.Template.Spec.DNSPolicy != corev1.DNSClusterFirst {
+		t.Errorf("expected dnsPolicy to be #{corev1.DNSClusterFirst}")


Suggested change

t.Errorf("expected dnsPolicy to be #{corev1.DNSClusterFirst}")

t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)

sgreene570 · 2020-11-24T14:56:19Z

pkg/operator/controller/ingress/deployment_test.go

@@ -459,6 +466,11 @@ func TestDesiredRouterDeployment(t *testing.T) {
 	if deployment.Spec.Template.Spec.HostNetwork != true {
 		t.Error("expected host network to be true")
 	}
+
+	if deployment.Spec.Template.Spec.DNSPolicy != corev1.DNSClusterFirstWithHostNet {
+		t.Errorf("expected dnsPolicy to be #{corev1.DNSClusterFirstWithHostNet}")


Suggested change

t.Errorf("expected dnsPolicy to be #{corev1.DNSClusterFirstWithHostNet}")

t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirstWithHostNet, deployment.Spec.Template.Spec.DNSPolicy)

sgreene570 · 2020-11-24T14:58:42Z

pkg/operator/controller/ingress/deployment_test.go

@@ -875,6 +887,21 @@ func TestDeploymentConfigChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description: "if dnsPolicy is not changed",


Actually I think this case is covered by the "if nothing changes" test.

Miciah · 2020-11-26T05:16:15Z

pkg/operator/controller/ingress/deployment_test.go

-		t.Error("expected host network to be false")
+		t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)


Please revert this change.

… network then we should automatically set the dnsPolicy with ClusterFirstWithHostNet.

Miciah · 2020-11-30T04:29:57Z

/retitle Bug 1880902: automatically set the dnsPolicy with ClusterFirstWithHostNet when endpoint publishing strategy type is set to hostnetwork
/lgtm
Thanks!

openshift-ci-robot · 2020-11-30T04:30:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, miheer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-11-30T04:30:19Z

@miheer: This pull request references Bugzilla bug 1880902, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1880902: automatically set the dnsPolicy with ClusterFirstWithHostNet when endpoint publishing strategy type is set to hostnetwork

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2020-11-30T04:33:48Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-11-30T04:46:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-11-30T08:06:50Z

@miheer: All pull requests linked via external trackers have merged:

openshift/cluster-ingress-operator#492

Bugzilla bug 1880902 has been moved to the MODIFIED state.

In response to this:

Bug 1880902: automatically set the dnsPolicy with ClusterFirstWithHostNet when endpoint publishing strategy type is set to hostnetwork

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from frobware and knobunc November 17, 2020 12:48

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2020

miheer force-pushed the dnsPolicy branch from 1397150 to ce85eed Compare November 17, 2020 16:21

sgreene570 reviewed Nov 17, 2020

View reviewed changes

pkg/operator/controller/ingress/deployment.go Outdated Show resolved Hide resolved

miheer force-pushed the dnsPolicy branch 5 times, most recently from 5ea5f77 to 8a80513 Compare November 18, 2020 03:33

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2020

sgreene570 reviewed Nov 18, 2020

View reviewed changes

miheer force-pushed the dnsPolicy branch from 8a80513 to ab667cf Compare November 23, 2020 09:34

sgreene570 reviewed Nov 23, 2020

View reviewed changes

miheer force-pushed the dnsPolicy branch from ab667cf to ac69185 Compare November 24, 2020 01:42

sgreene570 reviewed Nov 24, 2020

View reviewed changes

miheer force-pushed the dnsPolicy branch from ac69185 to 52fc52e Compare November 26, 2020 01:41

Miciah reviewed Nov 26, 2020

View reviewed changes

Bug 1880902 - If the endpoint publishing strategy type is set to host…

0a8f8ea

… network then we should automatically set the dnsPolicy with ClusterFirstWithHostNet.

miheer force-pushed the dnsPolicy branch from 52fc52e to 0a8f8ea Compare November 27, 2020 02:20

openshift-ci-robot assigned Miciah Nov 30, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2020

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. labels Nov 30, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Nov 30, 2020

openshift-merge-robot merged commit 86adacb into openshift:master Nov 30, 2020

zhangguanzhang mentioned this pull request Dec 21, 2020

crd ingresscontrollers need dnsPlocy set for deloyment #464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1880902: automatically set the dnsPolicy with ClusterFirstWithHostNet when endpoint publishing strategy type is set to hostnetwork #492

Bug 1880902: automatically set the dnsPolicy with ClusterFirstWithHostNet when endpoint publishing strategy type is set to hostnetwork #492

miheer commented Nov 17, 2020

Miciah commented Nov 17, 2020

miheer commented Nov 18, 2020

sgreene570 left a comment

sgreene570 Nov 18, 2020

sgreene570 Nov 18, 2020

sgreene570 Nov 18, 2020

miheer commented Nov 23, 2020

sgreene570 Nov 23, 2020

miheer Nov 24, 2020

miheer Nov 24, 2020

sgreene570 Nov 24, 2020

sgreene570 Nov 24, 2020

sgreene570 Nov 24, 2020

sgreene570 Nov 24, 2020 •

edited

Miciah Nov 26, 2020

Miciah commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

	t.Errorf("expected dnsPolicy to be #{corev1.DNSClusterFirst}")
	t.Errorf("expected dnsPolicy to be %q, got %q", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)

	t.Errorf("expected dnsPolicy to be %q, got %q", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)
	t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)

		t.Error("expected host network to be false")
		t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy)

Bug 1880902: automatically set the dnsPolicy with ClusterFirstWithHostNet when endpoint publishing strategy type is set to hostnetwork #492

Bug 1880902: automatically set the dnsPolicy with ClusterFirstWithHostNet when endpoint publishing strategy type is set to hostnetwork #492

Conversation

miheer commented Nov 17, 2020

Miciah commented Nov 17, 2020

miheer commented Nov 18, 2020

sgreene570 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

miheer commented Nov 23, 2020

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

sgreene570 Nov 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

sgreene570 Nov 24, 2020 •

edited