Skip to content

Add annotations to task update request api#4647

Merged
mxpv merged 1 commit intocontainerd:masterfrom
katiewasnothere:task_update_annotations_upstream
Nov 18, 2020
Merged

Add annotations to task update request api#4647
mxpv merged 1 commit intocontainerd:masterfrom
katiewasnothere:task_update_annotations_upstream

Conversation

@katiewasnothere
Copy link
Copy Markdown

@katiewasnothere katiewasnothere commented Oct 23, 2020

This PR is a companion to an update in cri-api that I made recently kubernetes/kubernetes#95741. This PR adds annotations to UpdateTaskRequest and UpdateTaskInfo so when the cri-api change is merged, containerd can use it and pass along to shims. While waiting for that PR, I'd like to get feedback here and hopefully merge this.

As mentioned in the above PR, the annotations field on task Update allows for experimentation with updating various resources or passing along update options.

Signed-off-by: Kathryn Baldauf [email protected]

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Oct 23, 2020

Hi @katiewasnothere. 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.

Details

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/test-infra repository.

@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Oct 23, 2020

@kevpar @dcantah @ambarve fyi

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Oct 23, 2020

Build succeeded.

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Oct 26, 2020

/ok-to-test

mxpv
mxpv approved these changes Oct 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mxpv mxpv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@katiewasnothere katiewasnothere force-pushed the task_update_annotations_upstream branch from b79c5a7 to 7c6543e Compare October 27, 2020 20:42
@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Oct 27, 2020

Updated based on local testing to actually pass the annotations in a shim request :P

Will need to retest @mxpv

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Oct 27, 2020

Build succeeded.

@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Nov 5, 2020

@mxpv PTAL when you can and merge if it looks good :)

@mxpv
Copy link
Copy Markdown
Member

mxpv commented Nov 5, 2020

Changes LGTM. This needs one more LGTM in order to be merged :)

AkihiroSuda
AkihiroSuda reviewed Nov 6, 2020
View reviewed changes
Comment thread task.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Nov 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for experimentation?

Copy link
Copy Markdown
Author

@katiewasnothere katiewasnothere Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update this. Really annotations are for arbitrary AND experimental resource constraint information. The goal is that we can pass in any resource information for use and for testing now, then if we determine there are specific resources or other fields that would be useful upstream or in some other way, we can make additional PRs to opencontainer's runtime spec to update WindowsResources.

@katiewasnothere katiewasnothere force-pushed the task_update_annotations_upstream branch from 7c6543e to 64e0e1b Compare November 9, 2020 21:44
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Nov 9, 2020

Build succeeded.

@katiewasnothere katiewasnothere force-pushed the task_update_annotations_upstream branch from 64e0e1b to 95ba6e9 Compare November 9, 2020 22:13
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Nov 9, 2020

Build succeeded.

@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Nov 10, 2020

@AkihiroSuda PTAL when you can :)

@kevpar @dcantah @ambarve @anmaxvl fyi

@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Nov 11, 2020

@mxpv and @AkihiroSuda is there any concern about compat issues with this?

@katiewasnothere katiewasnothere mentioned this pull request Nov 11, 2020
Closed
@kevpar
Copy link
Copy Markdown
Member

kevpar commented Nov 11, 2020

This looks okay to me. Adding a field to the proto shouldn't be a breaking change, and using the functional options approach on the task API with WithAnnotations means that shouldn't break either.

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Nov 12, 2020

@dmcgowan PTAL

estesp
estesp approved these changes Nov 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Nov 16, 2020

@AkihiroSuda @estesp @dmcgowan is this ready to be merged?

mikebrow
mikebrow approved these changes Nov 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mxpv mxpv merged commit 0d47346 into containerd:master Nov 18, 2020
@katiewasnothere katiewasnothere mentioned this pull request Nov 18, 2020
Closed
@katiewasnothere katiewasnothere deleted the task_update_annotations_upstream branch November 18, 2020 23:54
@katiewasnothere katiewasnothere mentioned this pull request Nov 20, 2020
Merged
@katiewasnothere katiewasnothere mentioned this pull request Sep 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mxpv mxpv mxpv approved these changes

@estesp estesp estesp approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees

No one assigned

Labels

impact/changelog kind/enhancement ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@katiewasnothere @k8s-ci-robot @AkihiroSuda @mxpv @kevpar @estesp @mikebrow