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.

Sign up for GitHub

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

Fix status comparison in Openshift #1136

Closed
wants to merge 4 commits into from
Closed

Fix status comparison in Openshift #1136

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
55b8742
Keep nil value in status
barkbay Jun 24, 2019
f3ce995
Add comment
barkbay Jun 24, 2019
8e4b5ed
Merge remote-tracking branch 'origin/master' into fix-status-openshift
barkbay Jun 24, 2019
538cf2a
Ensure map is nil if empty
barkbay Jun 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion operators/pkg/controller/elasticsearch/remotecluster/elasticsearch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ func UpdateRemoteCluster(
}
}
// Update state
reconcileState.UpdateRemoteClusters(currentRemoteClusters)
// If the map is empty then it must be stored as nil, not an empty map. It is retrieved as a nil value from the API
// and it could lead to some reconcile loops that never converge on Openshift.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the comment a bit misleading. We say it must be stored as nil, but that's not what we actually do.
What we do here IIUC is update the state only if there are some remote clusters. We ignore all cases where the map is either nil or empty.

What if we have 3 remote clusters at some point, then do some updates and get to 0 remote clusters: does it mean the reconcileState never gets updated to match we now have 0 remote clusters?

if len(currentRemoteClusters) > 0 {
reconcileState.UpdateRemoteClusters(currentRemoteClusters)
}
return nil
}

Expand Down