[release/1.6 ] Add ArgsEscaped support for CRI #8247

kiashok · 2023-03-10T02:19:02Z

This PR cherry picks commit for argsEscaped support in CRI from containerd/main to release/1.6 branch + updates image-spec version to include the changes for ArgsEscaped support in OCI + git mod tidy/vendor + cherry-pick e0b817e which has some small test fixes (function name change) from main to release/1.6

This commit adds supports for the ArgsEscaped
value for the image got from the dockerfile.
It is used to evaluate and process the image
entrypoint/cmd and container entrypoint/cmd
options got from the podspec.

(cherry picked from commit 8137e41)

k8s-ci-robot · 2023-03-10T02:19:12Z

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

dcantah · 2023-03-13T07:19:07Z

Should we just cherry-pick these over to 1.6 as well? Then we'd get the image spec bump for free and would get Justin's work:

So Justin's two commits, and then your CRI work. wdyt?

pkg/cri/opts/spec_nonwindows.go

kzys

I agree with @dcantah, but I'm fine to have them in a separate PR.

kiashok · 2023-03-15T17:51:05Z

Should we just cherry-pick these over to 1.6 as well? Then we'd get the image spec bump for free and would get Justin's work:

de3d999

d4b9dad

So Justin's two commits, and then your CRI work. wdyt?

Sure I can send out separate PRs for these two commits

kiashok · 2023-03-21T18:14:05Z

@mikebrow could you please take a look at the changes if you have some time? :)

dims · 2023-03-29T01:56:02Z

/ok-to-test

samuelkarp · 2023-03-29T08:07:52Z

/ok-to-test

mikebrow

see comments

integration/common.go

mikebrow · 2023-03-29T19:56:32Z

integration/windows_hostprocess_test.go

 	action(t, cn, containerConfig)
 }
+
+func startAndTestContainer(t *testing.T, sb string, sbConfig *runtime.PodSandboxConfig, cnConfig *runtime.ContainerConfig) {


createStartStopAndRemoveTestContainer :-)

with no pause at all between the start and stop steps not sure if you'll be letting the container run or not.. will be time and scheduling dependent

Not sure I am able to repro the window after start container where the container is not already running and we have killed it. Looks like StartContainer() is a synchronous call as well. Can add an assert after run to make sure the state of container is expected. Thoughts @dcantah @kzys ?

is a pain for sure.. typically one needs to have a probe waiting on some known state value in the container or just pause sufficient time after start returns

I think the 5 sec delay added in PR #8359 is good enough for this test

integration/windows_hostprocess_test.go

mikebrow · 2023-03-29T20:07:21Z

apologies for code review against the cherry pick... maybe open issue on main and back port.. then cherry both to 1.6?

estesp · 2023-03-29T20:11:29Z

I'm guessing this PR should be closed? The cherry-pick from the already merged change in main is #8306 and is ready to merge for release/1.6 and does not have the test changes you commented on.

mikebrow · 2023-03-29T20:16:23Z

I'm guessing this PR should be closed? The cherry-pick from the already merged change in main is #8306 and is ready to merge for release/1.6 and does not have the test changes you commented on.

thx was just plowing through review requests and missed the replace change

dcantah · 2023-03-29T20:20:33Z

I think the status here is that this change solves this ArgsEscaped problem for CRI, whereas Justin had made some changes to fix this for containerd clients (client options, and in the OCI image spec) that would be nice to get in as well. I almost forgot how this played out.

One of two of Justin's changes has been backported already: #8273.

And @kiashok has an open change for the second (removes a workaround Justin had before he got this field into the image spec itself): #8306

@kiashok Let me know if that's not correct 🫠.

kiashok · 2023-03-29T20:59:34Z

I think the status here is that this change solves this ArgsEscaped problem for CRI, whereas Justin had made some changes to fix this for containerd clients (client options, and in the OCI image spec) that would be nice to get in as well. I almost forgot how this played out.

One of two of Justin's changes has been backported already: #8273.

And @kiashok has an open change for the second (removes a workaround Justin had before he got this field into the image spec itself): #8306

@kiashok Let me know if that's not correct 🫠.

Yes this is right! :) Thanks Danny!

dmcgowan · 2023-03-30T15:20:40Z

This needs a rebase

kiashok · 2023-04-05T22:39:36Z

apologies for code review against the cherry pick... maybe open issue on main and back port.. then cherry both to 1.6?

@mikebrow I submitted a PR against main branch to address the review comments -#8359 . Could you please take a look?

kiashok · 2023-04-14T17:01:21Z

This needs a rebase

This PR is waiting on #8359 to be checked in. Will bring in those changes as well and rebase once it goes in

kiashok · 2023-04-17T23:53:14Z

apologies for code review against the cherry pick... maybe open issue on main and back port.. then cherry both to 1.6?

Hi @mikebrow , changes were checked into main with commit e0b817e . Cherry-picked the same and updated this PR. Could you please take a look?

This commit adds supports for the ArgsEscaped value for the image got from the dockerfile. It is used to evaluate and process the image entrypoint/cmd and container entrypoint/cmd options got from the podspec. Signed-off-by: Kirtana Ashok <[email protected]> (cherry picked from commit 8137e41) Signed-off-by: Kirtana Ashok <[email protected]>

- Rename test name - Add a tag to the container image used in the tests instead of the latest tag - Add a 5 second delay between container start and stop to ensure that the container is fully initialized Signed-off-by: Kirtana Ashok <[email protected]> (cherry picked from commit e0b817e) Signed-off-by: Kirtana Ashok <[email protected]>

kiashok · 2023-04-17T23:57:44Z

This needs a rebase

This PR is waiting on #8359 to be checked in. Will bring in those changes as well and rebase once it goes in

Updated this PR. Could you please take a look @dmcgowan @kzys ?

kiashok · 2023-04-20T20:28:24Z

@mikebrow @dcantah @dmcgowan could you please take a look if you have some time?

kiashok · 2023-04-20T23:05:13Z

@estesp the test failure seen here is unrelated to the change being made -https://github.com/containerd/containerd/actions/runs/4726994245/jobs/8387174524?pr=8247 . Could you please restart the failing test?

containerd 1.6.21 Welcome to the v1.6.21 release of containerd! The twenty-first patch release for containerd 1.6 contains various fixes and updates. * **update runc binary to v1.1.7 ([containerd#8450](containerd#8450)) * **Remove entry for container from container store on error ([containerd#8456](containerd#8456)) * **oci: partially restore comment on read-only mounts for uid/gid uses ([containerd#8403](containerd#8403)) * **windows: Add ArgsEscaped support for CRI ([containerd#8247](containerd#8247)) * **oci: Use WithReadonlyTempMount when adding users/groups ([containerd#8357](containerd#8357)) * **archive: consistently respect value of WithSkipDockerManifest ([containerd#8345](containerd#8345)) See the changelog for complete list of changes Please try out the release binaries and report any issues at https://github.com/containerd/containerd/issues. * Derek McGowan * Sebastiaan van Stijn * Iceber Gu * Kirtana Ashok * Justin Chadwell * Phil Estes * Akihiro Suda * Djordje Lukic * Kazuyoshi Kato * Mike Brown * Wei Fu * kiashok <details><summary>26 commits</summary> <p> * [release/1.6] Prepare release notes for v1.6.21 ([containerd#8463](containerd#8463)) * [`9226c362a`](containerd@9226c36) Add release notes for v1.6.21 * [release/1.6] update go to go1.19.9 ([containerd#8469](containerd#8469)) * [`39566aade`](containerd@39566aa) [release/1.6] update go to go1.19.9 * [release/1.6] fix the task setting the runtime path ([containerd#8454](containerd#8454)) * [`e8840f688`](containerd@e8840f6) skip TestContainerStartWithAbsRuntimePath if the runtime is v1 * [`75ab094de`](containerd@75ab094) integration: add container start test using abs runtime path * [`f49254f0b`](containerd@f49254f) WithRuntimePath uses the TaskInfo.RuntimePath field * [release/1.6 backport] update runc binary to v1.1.7 ([containerd#8450](containerd#8450)) * [`ccb51ff26`](containerd@ccb51ff) update runc binary to v1.1.7 * [release/1.6] Remove entry for container from container store on error ([containerd#8456](containerd#8456)) * [`95d31551d`](containerd@95d3155) Remove entry for container from container store on error * [release/1.6 backport] oci: partially restore comment on read-only mounts for uid/gid uses ([containerd#8403](containerd#8403)) * [`c33eb574d`](containerd@c33eb57) oci: partially restore comment on read-only mounts for uid/gid uses * [release/1.6 ] Add ArgsEscaped support for CRI ([containerd#8247](containerd#8247)) * [`bc2e01303`](containerd@bc2e013) Fix argsEscaped tests * [`8b81d5acc`](containerd@8b81d5a) Add ArgsEscaped support for CRI * [release/1.6 backport] update runc binary to v1.1.6 ([containerd#8385](containerd#8385)) * [`57d953482`](containerd@57d9534) update runc binary to v1.1.6 * [release/1.6 backport] oci: Use WithReadonlyTempMount when adding users/groups ([containerd#8357](containerd#8357)) * [`fb5e663d0`](containerd@fb5e663) oci: Use WithReadonlyTempMount when adding users/groups * [release/1.6] update go to go1.19.8 ([containerd#8353](containerd#8353)) * [`26efb8fd5`](containerd@26efb8f) [release/1.6] update go to go1.19.8 * [release/1.6] archive: consistently respect value of WithSkipDockerManifest ([containerd#8345](containerd#8345)) * [`ec13b497e`](containerd@ec13b49) export: add test for WithSkipDockerManifest * [`d1f3771c4`](containerd@d1f3771) archive: consistently respect value of WithSkipDockerManifest </p> </details> This release has no dependency changes Previous release can be found at [v1.6.20](https://github.com/containerd/containerd/releases/tag/v1.6.20)

containerd 1.6.21 Welcome to the v1.6.21 release of containerd! The twenty-first patch release for containerd 1.6 contains various fixes and updates. * **update runc binary to v1.1.7 ([#8450](containerd/containerd#8450)) * **Remove entry for container from container store on error ([#8456](containerd/containerd#8456)) * **oci: partially restore comment on read-only mounts for uid/gid uses ([#8403](containerd/containerd#8403)) * **windows: Add ArgsEscaped support for CRI ([#8247](containerd/containerd#8247)) * **oci: Use WithReadonlyTempMount when adding users/groups ([#8357](containerd/containerd#8357)) * **archive: consistently respect value of WithSkipDockerManifest ([#8345](containerd/containerd#8345)) See the changelog for complete list of changes Please try out the release binaries and report any issues at https://github.com/containerd/containerd/issues. * Derek McGowan * Sebastiaan van Stijn * Iceber Gu * Kirtana Ashok * Justin Chadwell * Phil Estes * Akihiro Suda * Djordje Lukic * Kazuyoshi Kato * Mike Brown * Wei Fu * kiashok <details><summary>26 commits</summary> <p> * [release/1.6] Prepare release notes for v1.6.21 ([#8463](containerd/containerd#8463)) * [`9226c362a`](containerd/containerd@9226c36) Add release notes for v1.6.21 * [release/1.6] update go to go1.19.9 ([#8469](containerd/containerd#8469)) * [`39566aade`](containerd/containerd@39566aa) [release/1.6] update go to go1.19.9 * [release/1.6] fix the task setting the runtime path ([#8454](containerd/containerd#8454)) * [`e8840f688`](containerd/containerd@e8840f6) skip TestContainerStartWithAbsRuntimePath if the runtime is v1 * [`75ab094de`](containerd/containerd@75ab094) integration: add container start test using abs runtime path * [`f49254f0b`](containerd/containerd@f49254f) WithRuntimePath uses the TaskInfo.RuntimePath field * [release/1.6 backport] update runc binary to v1.1.7 ([#8450](containerd/containerd#8450)) * [`ccb51ff26`](containerd/containerd@ccb51ff) update runc binary to v1.1.7 * [release/1.6] Remove entry for container from container store on error ([#8456](containerd/containerd#8456)) * [`95d31551d`](containerd/containerd@95d3155) Remove entry for container from container store on error * [release/1.6 backport] oci: partially restore comment on read-only mounts for uid/gid uses ([#8403](containerd/containerd#8403)) * [`c33eb574d`](containerd/containerd@c33eb57) oci: partially restore comment on read-only mounts for uid/gid uses * [release/1.6 ] Add ArgsEscaped support for CRI ([#8247](containerd/containerd#8247)) * [`bc2e01303`](containerd/containerd@bc2e013) Fix argsEscaped tests * [`8b81d5acc`](containerd/containerd@8b81d5a) Add ArgsEscaped support for CRI * [release/1.6 backport] update runc binary to v1.1.6 ([#8385](containerd/containerd#8385)) * [`57d953482`](containerd/containerd@57d9534) update runc binary to v1.1.6 * [release/1.6 backport] oci: Use WithReadonlyTempMount when adding users/groups ([#8357](containerd/containerd#8357)) * [`fb5e663d0`](containerd/containerd@fb5e663) oci: Use WithReadonlyTempMount when adding users/groups * [release/1.6] update go to go1.19.8 ([#8353](containerd/containerd#8353)) * [`26efb8fd5`](containerd/containerd@26efb8f) [release/1.6] update go to go1.19.8 * [release/1.6] archive: consistently respect value of WithSkipDockerManifest ([#8345](containerd/containerd#8345)) * [`ec13b497e`](containerd/containerd@ec13b49) export: add test for WithSkipDockerManifest * [`d1f3771c4`](containerd/containerd@d1f3771) archive: consistently respect value of WithSkipDockerManifest </p> </details> This release has no dependency changes Previous release can be found at [v1.6.20](https://github.com/containerd/containerd/releases/tag/v1.6.20)

k8s-ci-robot added the needs-ok-to-test label Mar 10, 2023

mikebrow reviewed Mar 13, 2023

View reviewed changes

pkg/cri/opts/spec_nonwindows.go Outdated Show resolved Hide resolved

kiashok force-pushed the port-gracefulterminationFix branch from 4a43b1f to f2cbd7a Compare March 13, 2023 22:27

kzys approved these changes Mar 14, 2023

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 29, 2023

mikebrow reviewed Mar 29, 2023

View reviewed changes

dcantah mentioned this pull request Mar 29, 2023

[release/1.6] Prepare release notes for v1.6.20 #8310

Merged

kiashok mentioned this pull request Apr 5, 2023

Fix argsEscaped tests #8359

Merged

Kirtana Ashok added 2 commits April 17, 2023 16:56

kiashok force-pushed the port-gracefulterminationFix branch from b1b3ace to bc2e013 Compare April 17, 2023 23:56

dmcgowan approved these changes Apr 20, 2023

View reviewed changes

dcantah approved these changes Apr 20, 2023

View reviewed changes

estesp approved these changes Apr 21, 2023

View reviewed changes

estesp merged commit 97243ae into containerd:release/1.6 Apr 21, 2023

kiashok mentioned this pull request May 1, 2023

[release/1.6] Prepare release notes for v1.6.21 #8463

Merged

kiashok deleted the port-gracefulterminationFix branch June 13, 2023 22:14

kiashok restored the port-gracefulterminationFix branch June 13, 2023 22:16

[release/1.6 ] Add ArgsEscaped support for CRI #8247

[release/1.6 ] Add ArgsEscaped support for CRI #8247

Uh oh!

Conversation

kiashok commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 10, 2023

Uh oh!

dcantah commented Mar 13, 2023

Uh oh!

Uh oh!

kzys left a comment

Choose a reason for hiding this comment

Uh oh!

kiashok commented Mar 15, 2023

Uh oh!

kiashok commented Mar 21, 2023

Uh oh!

dims commented Mar 29, 2023

Uh oh!

samuelkarp commented Mar 29, 2023

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikebrow Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

mikebrow Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

kiashok Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

mikebrow Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

kiashok Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikebrow commented Mar 29, 2023

Uh oh!

estesp commented Mar 29, 2023

Uh oh!

mikebrow commented Mar 29, 2023

Uh oh!

dcantah commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiashok commented Mar 29, 2023

Uh oh!

dmcgowan commented Mar 30, 2023

Uh oh!

kiashok commented Apr 5, 2023

Uh oh!

kiashok commented Apr 14, 2023

Uh oh!

kiashok commented Apr 17, 2023

Uh oh!

kiashok commented Apr 17, 2023

Uh oh!

kiashok commented Apr 20, 2023

Uh oh!

kiashok commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kiashok commented Mar 10, 2023 •

edited

Loading

dcantah commented Mar 29, 2023 •

edited

Loading