-
Notifications
You must be signed in to change notification settings - Fork 50
update the logic for updating the k8s resourcces #752
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
Conversation
ddcf95d to
5491916
Compare
|
It is ready for merging. |
| } | ||
|
|
||
| // delete the existing k8s resource, some resources could not be updated after it has been created | ||
| if err := r.Client.Delete(ctx, &existingK8sRes); err != nil { |
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.
Will this line cause the k8s resource to get deleted in every reconcile loop?
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.
Yes, when ODLM is reconciling, and the resources is forced to update and label is correct. It will be deleted and re-created
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.
But even if we use the Update(), the resource will be updated in each reconcile loop except the resource could not be updated.
One solution is to compare the existing resource with the new template, and decide whether we should update it.
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.
Could I ask which k8s resouce, that can't be updated?
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.
like job, some of fields in template is immutable, so we could not update it.
ZhuoxiLi
left a comment
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.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Daniel-Fan, ZhuoxiLi 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 |
This reverts commit c840b9e.
Some k8s resources could not be updated after it is created.
Jobfor exampleSince we will overwrite the resources when it is forced to update. now the update logic will delete the resource and then create a new one.