Let kOps take ownership of any field currently owned by kubectl #14362
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
olemarkus
requested review from
justinsb
and removed request for
mikesplain and
KashifSaadat
k8s-ci-robot
added
the
size/L
| @@ -141,7 +144,24 @@ func (a *ApplySet) ApplyOnce(ctx context.Context) (*ApplyResults, error) { | |||
| return nil, fmt.Errorf("unknown scope for gvk %s: %q", gvk, restMapping.Scope.Name()) | |||
| } | |||
|
|
|||
| j, err := json.Marshal(obj) | |||
| currentObj, err := dynamicResource.Get(ctx, name, v1.GetOptions{}) | |||
There was a problem hiding this comment.
I do wonder whether we should put this into a separate method, because (I would hope) this isn't going to be the common path.
One possible approach .... when we apply, we get the "after" object. We could inspect that to see if field ownership changes are needed. It's slightly less efficient in this case, because I'd imagine we'd just reprocess every object, but I would expect this case also to be the rare case.
| @@ -103,11 +106,11 @@ func (a *ApplySet) ApplyOnce(ctx context.Context) (*ApplyResults, error) { | |||
|
|
|||
| for i := range trackers.items { | |||
| tracker := &trackers.items[i] | |||
| obj := tracker.desired | |||
| expectedObject := tracker.desired | |||
There was a problem hiding this comment.
Nit: should probably match the name of the tracker field. So I would argue either to call this desiredObj or to rename track.desired to tracker.expected.
(Personally I think desired better captures the idea that this is what the user wants, but open either way ... we used e for expected in all our kOps tasks)
| @@ -1,128 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Do we want to delete the test? If the test doesn't pass I'd rather t.Skip() it with a reason, and then we can try to make our mocks better? I'm trying to incorporate mockkubeapiserver (and applyset) into kubebuilder-declarative-pattern here kubernetes-sigs/kubebuilder-declarative-pattern#248 . I can keep copying and pasting back and forth, but I'm also trying to establish a go.mod there as that might be a good place to maintain it (eventually!)
| return nil, fmt.Errorf("failed to marsal %q into json: %w", currentObject.GetName(), err) | ||
| } |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
I think this looks great - one open question on whether we want to remove the test, and I think we should think about whether we want this in the common path (I would prefer it wasn't, for performance). But I think the logic is sound, and I'd be in favor of merging and then I can see if the (slightly more complicated) code we have internally to do this handles any cases we are missing. |
|
This one started flaking after this PR: https://testgrid.k8s.io/kops-misc#kops-aws-addon-resource-tracking It seems to be hitting the validation timeout. Or that there is some race condition that makes things fail. It's at least not entirely fit for production. |
No description provided.