-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
controller/v1beta2: Warn users when owned resources are changed. #273
controller/v1beta2: Warn users when owned resources are changed. #273
Conversation
89c4c91
to
082ba5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/controller/v1beta2/controller.go
Outdated
messageCMFailed = "Failed creating ConfigMap" | ||
messagePeerIPAdded = "Added peer IP to ConfigMap" | ||
messagePeerIPUpdated = "Updated peer IP in ConfigMap" | ||
messagePeerIPUpdatedWarn = "Readonly configuration updated. Update will be reverted" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name should be more generic, as this warning applies to multiple situtations. Something like messageReadOnlyResourceChanged
.
pkg/controller/v1beta2/controller.go
Outdated
@@ -488,6 +489,8 @@ func (hc *HabitatController) handleConfigMap(h *habv1beta1.Habitat) error { | |||
|
|||
level.Info(hc.logger).Log("msg", messagePeerIPUpdated, "name", cm.Name, "ip", leaderIP) | |||
hc.recorder.Event(h, apiv1.EventTypeNormal, cmUpdated, messagePeerIPUpdated) | |||
level.Warn(hc.logger).Log("msg", messagePeerIPUpdatedWarn, "name", cm.Name, "ip", leaderIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, it's the operator who has just updated the CM, so it's not one of the situations we should warn users about.
082ba5d
to
fa087f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicks that can be safely ignored.
pkg/controller/v1beta2/controller.go
Outdated
messageCMFailed = "Failed creating ConfigMap" | ||
messagePeerIPAdded = "Added peer IP to ConfigMap" | ||
messagePeerIPUpdated = "Updated peer IP in ConfigMap" | ||
messageReadOnlyResourceChanged = "Readonly configuration updated. Update will be reverted" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Maybe add the new variable at the end of the block instead of in the middle, unless it fits in some kind of an order?
4aaf3b1
to
286ecf6
Compare
hc.handleCM(newObj) | ||
} | ||
|
||
func (hc *HabitatController) handleCMDelete(obj interface{}) { | ||
level.Warn(hc.logger).Log("msg", messageReadOnlyResourceChanged) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if some kind of tag should be added to this message, just like that of the others. For example: level.Warn(hc.logger).Log("msg", messageReadOnlyResourceChanged, "name", pod.Name)
Please take a look again :) |
We should warn users when habitat's own resources are manually modified, because the operator will override these changes. Signed-off-by: Kosy Anyanwu <kosy@kinvolk.io>
286ecf6
to
a21ec6b
Compare
I'd say "no" for this PR at the moment. It produces a lot of output from the operator, even when I'm not doing anything after just creating a standalone example:
It is way too verbose. The verbosity maybe could be cut down if we somehow detected that nothing really happened, so nothing is going to change. When writing this message I got another bunch of lines:
|
@krnowak do you think we should close this PR and educate the users instead? |
Yes. I'm not sure what we should do to educate users, maybe add something to the design doc. |
The design doc already states that those resources are readonly. Maybe at some point we will get this functionality in kubernetes itself, which we can use. Closing. |
Fixes #252
We should warn users when habitat's own resources are manually modified,
because the operator will override these changes.