Bug 1831711: Fix GVK diff to work for special kinds with multiple groups #534
Conversation
|
Can one of the admins verify this patch? |
djwhatle
changed the title
|
\test |
|
✔️ Setup e2e environment |
pkg/gvk/gvk.go
Outdated
| if !resourceExist(resource, fingResourceList(srcList.GroupVersion, dst)) { | ||
| if cohabitator, found := r.CohabitatingResources[resource.Name]; found { | ||
| if cohabitator.seen { | ||
| // TODO: need to add log entry |
There was a problem hiding this comment.
@alaypatel07 you might be interested in learning about the controller logging package https://github.com/konveyor/mig-controller/blob/cac36386f9de729b65c452a453825254a6b9fe12/docs/devguide.md#logging
There was a problem hiding this comment.
In mig-controller the logging philosophy is essentially "less is more". We rely heavily on CR conditions to convey information as needed.
mig-controller only writes logs when:
- an error occurs
- it enters or exits stages in an active migration
- controller or watches start/stop
There was a problem hiding this comment.
In this case I would argue that the resulting MigPlan condition conveying GVK diff information would be sufficient, no log message required. Open to counter-argument.
There was a problem hiding this comment.
ack, not logging here makes sense in this case. Upstream Velero logs here, but I was not sure if we want our controller to log. I'll remove the TODO.
| @@ -261,7 +297,7 @@ func compareResources(src []*metav1.APIResourceList, dst []*metav1.APIResourceLi | |||
| return missingResources | |||
| } | |||
|
|
|||
| func fingResourceList(groupVersion string, list []*metav1.APIResourceList) []metav1.APIResource { | |||
| func findResourceList(groupVersion string, list []*metav1.APIResourceList) []metav1.APIResource { | |||
| } | ||
| } | ||
|
|
||
| func NewCohabitatingResources() map[string]*cohabitatingResource { |
There was a problem hiding this comment.
Does this need to be exported?
There was a problem hiding this comment.
Yes, I believe so because it is used in another package for instantiation[1].
alaypatel07
force-pushed
the
fix-multiple-kinds
branch
from
be4ae12
to
8eb7a28
Compare
|
updated as per review, PTAL whenever you get a chance @djwhatle |
| } | ||
|
|
||
| func NewCohabitatingResources() map[string]*cohabitatingResource { | ||
| return map[string]*cohabitatingResource{ |
There was a problem hiding this comment.
FYI for anyone looking on, this is borrowed from Velero source.
Our goal here is to accurately simulate the logic that Velero runs to choose which resource to back up when resources cohabitating in several API groups are present on a source cluster.
|
|
||
| // sortResources sources resources by moving extensions to the end of the slice. The order of all | ||
| // the other resources is preserved. | ||
| func sortResources(resources []*metav1.APIResourceList) { |
There was a problem hiding this comment.
Another piece borrowed from Velero to accurately simulate the logic used in Velero to determine which resources should be prioritized in a backup.
|
Closes #531 |
This implements the fix described here