[occm] ensure octavia monitor is always updated #2373
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
size/L
kayrus
force-pushed
the
octavia-ensure-monitor
branch
from
fa4be3c to
63aa237
Compare
|
/test openstack-cloud-csi-manila-sanity-test |
kayrus
force-pushed
the
octavia-ensure-monitor
branch
from
63aa237 to
279a66f
Compare
k8s-ci-robot
added
the
needs-rebase
kayrus
force-pushed
the
octavia-ensure-monitor
branch
from
279a66f to
dd4b681
Compare
k8s-ci-robot
removed
the
needs-rebase
kayrus
force-pushed
the
octavia-ensure-monitor
branch
from
dd4b681 to
699065f
Compare
kayrus
force-pushed
the
octavia-ensure-monitor
branch
from
699065f to
49e4784
Compare
|
/approve Question inline. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dulek 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
@dulek what do you mean? |
| createOpts := lbaas.buildMonitorCreateOpts(svcConf, port, name) | ||
| if createOpts.Type != monitor.Type { | ||
| klog.InfoS("Recreating health monitor for the pool", "pool", pool.ID, "oldMonitor", monitorID) | ||
| if err := openstackutil.DeleteHealthMonitor(lbaas.lb, monitorID, lbID); err != nil { | ||
| return err | ||
| } | ||
| return lbaas.createOctaviaHealthMonitor(createOpts, pool.ID, lbID) | ||
| } |
There was a problem hiding this comment.
Feels like you should be able to have just one line doing DeleteHealthMonitor() and one line doing createOctaviaHealthMonitor() to make it even more clean, but maybe you've already tried this?
There was a problem hiding this comment.
we need to check whether there is no error on the DeleteHealthMonitor stage before continuing.
There was a problem hiding this comment.
I've crafted something like this, haven't tested it. This seems to be quite closer to the previous version of the code, so I guess I liked it more.
The point here is that it makes sure there's only one place we call DeleteHealthMonitor() and createOctaviaHealthMonitor() from, which seems easier to maintain to me.
There was a problem hiding this comment.
I don't like this construction. I prefer to follow the https://medium.com/swlh/return-early-pattern-3d18a41bba8
For new users this code will be more readable and easy to understand.
There was a problem hiding this comment.
@dulek In addition, your logic doesn't cover a case, when a monitor must be recreated. You're deleting an old monitor without creating a new one. I think this is a good example on why long if blocks can confuse developers.
There was a problem hiding this comment.
Right, the DeleteOctaviaMonitor() should return there.
There was a problem hiding this comment.
@dulek From my point of view the ensureOctaviaHealthMonitor method must re-create the monitor in case when its type cannot be changed. The logic that you propose, just deletes an old monitor (DeleteOctaviaMonitor()) and a new monitor is created only on the next reconciliation loop.
|
/lgtm I am good with the change, leave a hold here so after solve above comments from @dulek we can unhold |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
/lgtm Okay, I've read this again, the diff is awful, the code is actually really easy to follow |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/retest |
What this PR does / why we need it:
The PR reconciles reconciles health monitors on cluster scale up/down events.
The fix also improves the readability of the
ensureOctaviaHealthMonitormethod and now reconciles the monitor name as well.Which issue this PR fixes(if applicable):
fixes #2370
Special notes for reviewers:
Release note: