Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store bootstrap parameters in sandbox metadata #9736

Merged
merged 5 commits into from May 2, 2024

Conversation

abel-von
Copy link
Contributor

@abel-von abel-von commented Feb 2, 2024

  1. Sandbox controller returns endpoint information including version, protocol and address when Start sandbox.
  2. Endpoint information is stored Extensions of the Sandbox in db, then shim get the endpoint information from Sandbox Extensions and restore bootstrap parameters.

This is the first PR of the sandboxed Task demonstrated in draft #9574

@k8s-ci-robot
Copy link

Hi @abel-von. 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/test-infra repository.

@abel-von
Copy link
Contributor Author

abel-von commented Feb 2, 2024

/cc @fuweid @dmcgowan @mxpv

@abel-von
Copy link
Contributor Author

@fuweid @mxpv @dmcgowan Could you please take a look at this PR?

@abel-von
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link

@abel-von: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@mxpv
Copy link
Member

mxpv commented Mar 20, 2024

Should we add an endpoint to query shim's address directly from the running shim instance?
I'm not sure if mixing runtime data and sandbox metadata is the right approach here (strictly speaking we store metadata in database to have enough information to restore runtime representation, but we should not store runtime specific bits in the database).

@abel-von
Copy link
Contributor Author

I think we may have to consider that every sandbox needs an endpoint for containerd to connect, no matter it is for Task API call, or other API(for example, Streaming API in #9965 ), It is because the shim sandbox controller implements by shim process so that the runtime related data like address becomes part of the sandbox metadata. For other customized sandbox controllers(may not implemented by shim), we may have no shim process, but we still serves some APIs so that containerd can manage Tasks or other stuff in sandbox, so maybe we can make this endpoint information part of sandbox's internal attributes. @mxpv

@mxpv
Copy link
Member

mxpv commented Mar 22, 2024

@abel-von I don't object the need of this feature, there are many use cases we can benefit from this. I'm just not sure that going through metadata store as the right way to go. Why not to query shim endpoint directly from running shim instance?

@abel-von
Copy link
Contributor Author

Why not to query shim endpoint directly from running shim instance?

I think we have to get the endpoint first and then we can call the API of the running shim. Maybe we can add the address in the response of Status API of sandbox controller?

@abel-von
Copy link
Contributor Author

abel-von commented Mar 27, 2024

Removed the store of endpoint in sandbox db, please take a look again. @mxpv

@Burning1020
Copy link
Member

/ok-to-test

@abel-von
Copy link
Contributor Author

/retest

Copy link
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.

Looks good!

@mxpv
Copy link
Member

mxpv commented Apr 23, 2024

This is an impactful change, would want a few more LGTMs for awareness.
@dmcgowan @AkihiroSuda @mikebrow PTAL?

Signed-off-by: Abel Feng <fshb1988@gmail.com>
Signed-off-by: Abel Feng <fshb1988@gmail.com>
Signed-off-by: Abel Feng <fshb1988@gmail.com>
Signed-off-by: Abel Feng <fshb1988@gmail.com>
Signed-off-by: Abel Feng <fshb1988@gmail.com>
@fuweid fuweid added this to the 2.0 milestone May 2, 2024
@fuweid fuweid added this pull request to the merge queue May 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 2, 2024
@fuweid fuweid added this pull request to the merge queue May 2, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 2, 2024
sandbox: Store bootstrap parameters in sandbox metadata and shim get them from sandbox metadata rather than other shim's bootstrap.json file.
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 2, 2024
@fuweid fuweid added this pull request to the merge queue May 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 2, 2024
@fuweid fuweid added this pull request to the merge queue May 2, 2024
Merged via the queue into containerd:main with commit a91b05d May 2, 2024
48 checks passed
@dmcgowan dmcgowan added the area/runtime Runtime label May 3, 2024
@dmcgowan dmcgowan changed the title sandbox: Store bootstrap parameters in sandbox metadata and shim get them from sandbox metadata rather than other shim's bootstrap.json file. Store bootstrap parameters in sandbox metadata May 3, 2024
Copy link
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.

Is there a desire to add convenience functions for endpoints? some structures and code paths are using the fields of the endpoint separately some are not.

Thoughts?

func (s *shim) Version() int {
return s.version
func (s *shim) Endpoint() (string, int) {
return s.address, s.version
Copy link
Member

Choose a reason for hiding this comment

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

nit.. seems like there is a desire to have endpoints be their own class.. but have not gone all the way with it ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants