Handle ingress domain updates to default cluster ingress #48

pravisankar · 2018-10-15T23:32:40Z

Delete duplicate finalizers section in default cluster ingress cr.
Current code fetches needed info from installer configmap and tries to create default cluster ingress cr and ignores in case of already exists error. It won't handle the case where default cluster ingress cr exists but the ingress domain got changed. This pr handles this case.

openshift-ci-robot · 2018-10-15T23:32:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pravisankar
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: ironcladlou

If they are not already assigned, you can assign the PR to them by writing /assign @ironcladlou in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

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

pravisankar · 2018-10-15T23:33:04Z

@openshift/sig-network-edge PTAL

ironcladlou · 2018-10-15T23:44:13Z

pkg/stub/handler.go

+		}
+		return false, true, nil
+	}
+	if oldci.Spec.IngressDomain != ci.Spec.IngressDomain {


I think you need to deference the pointers to do what you mean here: https://play.golang.org/p/N1-AT4yXgH-

yes, thanks

pravisankar · 2018-10-15T23:48:10Z

@ironcladlou updated, ptal

ironcladlou · 2018-10-15T23:53:42Z

How was it tested?

ramr

changes look good - should handle both create/update cases ... though am not certain if the installer supports an update case.

ramr · 2018-10-16T01:17:44Z

/hold

pravisankar · 2018-10-16T19:17:37Z

Tested both create and update cases on 4.0 cluster
/hold cancel

pravisankar · 2018-10-16T19:17:55Z

@openshift/sig-network-edge updated, PTAL

ironcladlou · 2018-10-16T19:20:03Z

pkg/stub/handler.go

+		}
+		return false, nil, nil
+	}
+	if *oldci.Spec.IngressDomain != *ci.Spec.IngressDomain {


If either of this is nil will the function panic?

Tried this https://play.golang.org/p/i0W49iA8Gjh and I didn't see an issue.

Let me fix this

ironcladlou · 2018-10-16T23:18:59Z

Would like to test this once #51 lands

imcsk8 · 2018-10-30T07:54:16Z

pkg/stub/handler.go

-		if err == nil {
-			logrus.Infof("created default cluster ingress %s/%s", ci.Namespace, ci.Name)
+	changed, nci, err := checkClusterIngress(ci)
+	if err != nil {


I think this can use: clusterIngressStatusEqual
from PR: #38

Maybe not clusterIngressStatusEqual, but the part that compares .loadBalancer.Ingress could be factored into a function that could be used both by checkClusterIngress and by clusterIngressStatusEqual. Now that #47 is merged, I'll rebase #38 Tuesday and factor out a LoadBalancerStatusEqual function. No need for that to block this PR though; I can change checkClusterIngress to use LoadBalancerStatusEqual if #48 merges before #38 is ready.

Err, actually, checkClusterIngress is comparing only .spec.ingressDomain (a string) whereas clusterIngressStatusEqual is comparing .status.loadBalancer (a slice of a struct that has two string values), so there is really no opportunity for code re-use here as far as I see.

I might be confused but if the LoadBalancerIngress struct are different as being checked in your PR [1] it means there's an update it should be enough for this purpouse.

[1] https://github.com/openshift/cluster-ingress-operator/pull/38/files#diff-9ae69f8dcadd8df44e0b10265f3a91a8R107

We're talking about

cluster-ingress-operator/pkg/stub/handler.go

Lines 111 to 117 in 3f8735c

if ci.Spec.IngressDomain == nil {

return false, nil, fmt.Errorf("invalid ingress domain for default cluster ingress %s/%s", ci.Namespace, ci.Name)

}

if oldci.Spec.IngressDomain == nil {

oldci.Spec.IngressDomain = new(string)

}

if *oldci.Spec.IngressDomain != *ci.Spec.IngressDomain {

, right? There, we are comparing *string values in order to determine whether the old ClusterIngress and new ClusterIngress specify the same ingress domain. clusterIngressStatusEqual compares []LoadBalancerIngress values, where LoadBalancerIngress is struct { IP, Hostname string }, in order to determine whether the old ClusterIngressStatus and new ClusterIngressStatus have the same ingresses.

openshift-bot · 2018-11-06T10:22:58Z

@pravisankar: PR needs rebase.

Details

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 · 2018-12-05T04:18:40Z

@pravisankar: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/rhel-images	`3f8735c`	link	`/test rhel-images`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

ironcladlou · 2018-12-19T15:58:43Z

Superseded by #48

ironcladlou · 2018-12-19T15:58:51Z

/close

openshift-ci-robot · 2018-12-19T15:58:52Z

@ironcladlou: Closed this PR.

Details

In response to this:

/close

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.

Miciah · 2018-12-19T16:57:46Z

Superseded by #48

For the record, I think that was supposed to be #89.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 15, 2018

openshift-ci-robot requested review from ironcladlou and knobunc October 15, 2018 23:32

ironcladlou reviewed Oct 15, 2018

View reviewed changes

pravisankar force-pushed the handle-default-cr-fixes branch from 0f3cc77 to 2c7012e Compare October 15, 2018 23:46

ramr approved these changes Oct 16, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2018

pravisankar force-pushed the handle-default-cr-fixes branch from 2c7012e to 46d0d0c Compare October 16, 2018 19:15

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2018

ironcladlou reviewed Oct 16, 2018

View reviewed changes

Ravi Sankar Penta added 2 commits October 16, 2018 12:51

Delete duplicate finalizers section in default cluster ingress cr

db1e1b2

Handle ingress domain updates to default cluster ingress

3f8735c

pravisankar force-pushed the handle-default-cr-fixes branch from 46d0d0c to 3f8735c Compare October 16, 2018 19:56

imcsk8 reviewed Oct 30, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2018

openshift-ci-robot closed this Dec 19, 2018

	if ci.Spec.IngressDomain == nil {
	return false, nil, fmt.Errorf("invalid ingress domain for default cluster ingress %s/%s", ci.Namespace, ci.Name)
	}
	if oldci.Spec.IngressDomain == nil {
	oldci.Spec.IngressDomain = new(string)
	}
	if oldci.Spec.IngressDomain != ci.Spec.IngressDomain {

Handle ingress domain updates to default cluster ingress #48

Handle ingress domain updates to default cluster ingress #48

Conversation

pravisankar commented Oct 15, 2018

Uh oh!

openshift-ci-robot commented Oct 15, 2018

Uh oh!

pravisankar commented Oct 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pravisankar commented Oct 15, 2018

Uh oh!

ironcladlou commented Oct 15, 2018

Uh oh!

ramr left a comment

Choose a reason for hiding this comment

Uh oh!

ramr commented Oct 16, 2018

Uh oh!

pravisankar commented Oct 16, 2018

Uh oh!

pravisankar commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Nov 6, 2018

Uh oh!

openshift-ci-robot commented Dec 5, 2018

Uh oh!

ironcladlou commented Dec 19, 2018

Uh oh!

ironcladlou commented Dec 19, 2018

Uh oh!

openshift-ci-robot commented Dec 19, 2018

Uh oh!

Miciah commented Dec 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants