feat: create clusterResourceSnapshots #398
Conversation
pkg/controllers/clusterresourceplacement/clusterresourceplacement_controller.go
Outdated
Show resolved
Hide resolved
pkg/controllers/clusterresourceplacement/clusterresourceplacement_controller.go
Outdated
Show resolved
Hide resolved
| if err != nil { | ||
| return nil, err | ||
| } | ||
| latestResourceSnapshotHash := "" |
There was a problem hiding this comment.
If you are already retrieving latest resource snapshot and its index in the lookup function, why not retrieve hashes there? It would simplify the logic a little bit.
There was a problem hiding this comment.
The lookup func is little complicated for now, so prefer to get the hash outside.
Refactored the code. PTAL to see if the readability is better.
pkg/controllers/clusterresourceplacement/clusterresourceplacement_controller.go
Outdated
Show resolved
Hide resolved
pkg/controllers/clusterresourceplacement/clusterresourceplacement_controller.go
Show resolved
Hide resolved
pkg/controllers/clusterresourceplacement/clusterresourceplacement_controller.go
Show resolved
Hide resolved
pkg/controllers/clusterresourceplacement/clusterresourceplacement_controller.go
Outdated
Show resolved
Hide resolved
pkg/controllers/clusterresourceplacement/clusterresourceplacement_controller.go
Outdated
Show resolved
Hide resolved
| klog.ErrorS(err, "Failed to list active clusterResourceSnapshots", "clusterResourcePlacement", crpKObj) | ||
| return nil, -1, controller.NewAPIServerError(false, err) | ||
| } | ||
| if len(snapshotList.Items) == 1 { |
There was a problem hiding this comment.
do we handle the multiple resourceSnapshots objects per index?
There was a problem hiding this comment.
not now, will handle it in the second phase.
| } | ||
|
|
||
| // latestResourceSnapshotIndex should be -1 when there is no snapshot. | ||
| latestResourceSnapshot, latestResourceSnapshotIndex, err := r.lookupLatestResourceSnapshot(ctx, crp) |
There was a problem hiding this comment.
the resourceSnapshot and schedulingPolicySnapshot are generated following the same rule. Do we need to create two functions to look up the last? Will generic help unite them?
There was a problem hiding this comment.
the implementation will diverge when we handle the 1MB limit for the resourceSnapshot.
So prefer not to unify them, otherwise there will be too many "if else" conditions.
3 participants
Description of your changes
get or create cluster resource snapshots if needed.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Unit tests
Special notes for your reviewer