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
agent:image: Support different pause image in the guest for guest pull #9369
Conversation
@littlejawa Could you please have a test using the okd-pause-image? Unfortunately, I don’t have the setup to test okd-pause-image myself. However, I’ve verified the code with the k8s pause image (registry.k8s.io/pause:3.9), and it works correctly. |
src/agent/src/image.rs
Outdated
.context("load image config file")?; | ||
|
||
let image_oci_process = image_oci.process.ok_or_else(|| { | ||
anyhow!("The image config file does not contain a process specification.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here: "The guest pause image config does not contain a process specification. Please check the pause image" to be consistent with the message below when args.len()
checker fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/agent/src/image.rs
Outdated
// Ensure that the args vector is not empty before accessing its elements. | ||
let args = image_oci_process.args; | ||
// Check the number of arguments. | ||
if args.len() != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the registry.k8s.io/pause
and quay.io/bpradipt/okd-pause
images there is only one args
(the pause executable file). However, is it always the case? It seems not unlikely to pass additional arguments to the pause executable so that args.len() >= 1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I have fixed it.
Hi @ChengyuZhu6 , I'm not exactly an expert on the subject but I think I understood this change, so I left just a couple of comments but some might not make sense (please ignore if the case). |
2b41b7e
to
1bf4b52
Compare
@wainersm thanks for your comments! I've actions your comments. Please double check when you get a chance. |
1bf4b52
to
7e58c4b
Compare
@ChengyuZhu6, I was able to verify your patches on a single node OCP cluster, that used the OCP's pause image by default, and it works as expected. Nice work! |
src/agent/src/image.rs
Outdated
info!(sl(), "pause_rootfs {:?}", pause_rootfs); | ||
|
||
copy_if_not_exists(&guest_pause_config, &pause_bundle.join(CONFIG_JSON))?; | ||
for pause_arg in args { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ChengyuZhu6 !
Thanks for addressing my comments but I'm afraid I confused you, sorry.
It should not raise if len(args)
> 1 as it you did before, however, we still want to copy only args[0]
(which is the pause binary file).
Let's use a hypothetical config.json
below, where len(args) > 1
:
{
"ociVersion": "1.0.0",
"process": {
"terminal": true,
"user": {
"uid": 65535,
"gid": 65535
},
"args": [
"/usr/bin/pod",
"arg1",
"arg2"
],
"env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"TERM=xterm"
],
arg1
and arg2
are really arguments to /usr/bin/pod
binary, it will fail if it tries to copy these arguments :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, apologies for the misunderstanding. Thanks for bringing this.
Support different pause images in the guest for guest-pull, such as k8s pause image (registry.k8s.io/pause) and openshift pause image (quay.io/bpradipt/okd-pause). Fixes: kata-containers#9225 -- part III Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>
7e58c4b
to
8c897f8
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thanks @ChengyuZhu6
LGTM, I will wait for @wainersm approval! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks once again @ChengyuZhu6 !
Support different pause images in the guest for guest-pull, such as k8s pause image (registry.k8s.io/pause) and openshift pause image (quay.io/bpradipt/okd-pause).
The primary distinction between the k8s pause image and the OKD pause image lies in the arguments specified in the
config.json
file:For the k8s pause image, the arguments are set as follows:
Conversely, for the OKD pause image, the arguments are different:
Fixes: #9225 -- part III