[WIP DO NOT MERGE] Handle update events #40
Conversation
Since the operator sdk does not natively handles update events we have to check for differences in the ClusterIngress structure by getting its md5 signatureits This commit is work in progress does not handle updates on the deployment and service elements of the ClusterIngress. Code cleanup
|
PTAL @openshift/sig-network-edge @ironcladlou @Miciah @pravisankar @knobunc |
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: imcsk8 If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
See topic 7 for some discussion on this, as well as the upstream controller guidelines on managing resources with For now, I'd be happy if we just unconditionally re-built the resource and issued a |
|
Yeah, I believe we want to use the following pattern: if ci.Generation > ci.Status.ObservedGeneration {
// ci.spec was changed; synchronize stuff.
}
ci.Status.ObservedGeneration = ci.Generation#38 adds |
|
@ironcladlou @tnozicka will know the most about generations. |
pkg/stub/handler.go
Outdated
|
|
||
| func (h *Handler) Handle(ctx context.Context, event sdk.Event) error { | ||
| if ci_md5 == nil { | ||
| ci_md5 = make(map[string][16]byte) |
There was a problem hiding this comment.
Where is this used? probably stale code
There was a problem hiding this comment.
Difference in MD5 checksum of the object to denote change in custom resource may not be useful in our case.
Once we populate status field, status is updated periodically and md5 checksum on the object will change even though there is no change in CR spec.
I think we should store old spec (as annotation or something mechanism) and then compare with new spec to determine if update is needed. This mechanism will also provide what exactly has changed and can react accordingly.
We could get more pointers from oc apply implementation.
There was a problem hiding this comment.
sorry this coude slipped the patch, should not be here
| logrus.Infof("created router daemonset %s/%s", ds.Namespace, ds.Name) | ||
| } else if !errors.IsAlreadyExists(err) { | ||
| } else if errors.IsAlreadyExists(err) { | ||
| err = sdk.Update(ds) |
There was a problem hiding this comment.
How do we know this is an update event and not resync event? (may be we want to update anyway during resync?)
There was a problem hiding this comment.
When @Miciah PR gets merged we will know when there's an update. I think this pattern is good for now.
pkg/stub/handler.go
Outdated
| logrus.Infof("created router service %s/%s", service.Namespace, service.Name) | ||
| } else if !errors.IsAlreadyExists(err) { | ||
| } else if errors.IsAlreadyExists(err) { | ||
| err = sdk.Update(service) |
There was a problem hiding this comment.
I don't think we need to update service, none of the spec changes in CR will change service object.
There was a problem hiding this comment.
you're right, removing this code
openshift-ci-robot
added
size/XS
openshift-bot
added
the
needs-rebase
|
@imcsk8: PR needs rebase. DetailsInstructions 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. |
|
Needs redesigned per #49; should probably close this one |
|
/close |
|
@ironcladlou: Closing this PR. DetailsIn response to this:
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. |
7 participants
Since the operator sdk does not natively handles update
events we have to check for differences in the ClusterIngress
structure by getting its md5 signatureits
This commit is work in progress does not handle updates on the
deployment and service elements of the ClusterIngress.
Code cleanup