update should support annotations

[zzzzzzzzzy9]

update should support annotations

[k8s-ci-robot]

Hi @zzzzzzzzzy9. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[zzzzzzzzzy9]

@mxpv Could you please review this pr? Thanks.

[klihub]

@zzzzzzzzzy9 @mxpv @mikebrow I think it's a bit unclear whether this is the intended semantics for annotations in UpdateContainerResourceRequest ? The comments in the protocol definition literally state

// Unstructured key-value map holding arbitrary additional information for
// container resources updating. This can be used for specifying experimental
// resources to update or other options to use when updating the container.
map<string, string> annotations = 4;

That seems to imply that these are supposed to optionally annotate the update with extra information instead of optionally updating the container's annotations themselves.

[zzzzzzzzzy9]

Thanks.

func (t *task) Update(ctx context.Context, opts ...UpdateTaskOpts) error {
	request := &tasks.UpdateTaskRequest{
		ContainerID: t.id,
	}
	var i UpdateTaskInfo
	for _, o := range opts {
		if err := o(ctx, t.client, &i); err != nil {
			return err
		}
	}
	if i.Resources != nil {
		any, err := typeurl.MarshalAny(i.Resources)
		if err != nil {
			return err
		}
		request.Resources = protobuf.FromAny(any)
	}
	if i.Annotations != nil {
		request.Annotations = i.Annotations
	}
	_, err := t.client.TaskService().Update(ctx, request)
	return errdefs.FromGRPC(err)
}

I think there has been an annotations in containerd's task.Update interface, therefore, the update of annotations should not be stripped in the CRI interface of containerd. Whether or not annotations should be filled in values, or what should be included in annotations, should be decided by the upper layer of the containerd (e.g. k8s), not by the containerd itself.
@klihub @mxpv @mikebrow

[klihub]

@zzzzzzzzzy9 And what would be the current mechanism to trigger such an update with annotations included considering that these are the existing pieces of related code in K8s and kubelet:

https://github.com/kubernetes/kubernetes/blob/670b98bf9229bb1f193571949871f794f7b11ec3/pkg/kubelet/cm/cpumanager/cpu_manager_others.go#L35
https://github.com/kubernetes/kubernetes/blob/e0b17379aaf5805dea2714ef2f1e47c862b7f172/staging/src/k8s.io/cri-client/pkg/remote_runtime.go#L471

Don't get me wrong, I'd really like to see annotation (and label) updates propagate down to the runtime. I just don't think this is going to help much... unless you plan to write your own CRI tooling which is bypassing K8s, and directly connecting to the runtime and doing an update... which you probably are not planning.

For pods, annotation (and label) updates are clearly possible, for instance by a simple kubectl annotate pod command. And here is a very recent related discussion of propagating those down to the runtime. I think that and the linked related KEP there would be a good first start for pods.

For containers, I'm not sure how would you even update dynamically the annotation of a Kubernetes container today, even from within a control plane component.

[zzzzzzzzzy9]

In the code of k8s, https://github.com/kubernetes/kubernetes/blob/e0b17379aaf5805dea2714ef2f1e47c862b7f172/staging/src/k8s.io/cri-client/pkg/remote_runtime.go#L471 there is an Annotations member variable in the UpdateContainerResourcesRequest struct, but k8s does not pass this parameter. This is OK because k8s is the upper user layer and k8s has the right to decide which parameters to pass. However, containerd is neither a decision-maker nor an action executor in the update interface, and is only used as a parameter transfer tool, and should not decide whether a certain parameter should be passed. What containerd should do is just pass parameters.

The Annotations field is not a new field, and theoretically, containerd itself should have the capabilities of this field, regardless of how the upper layer uses the field.

Containerd should be adapted to this field first, not when k8s needs it. I think this is something that is missing from the CRI of containerd, not something that should be merged after it has been called in the k8s code.
@klihub

[klihub]

@zzzzzzzzzy9 Okay, I guess it's totally fine, if you want to fix this in containerd first and only then in kubelet.