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

Set pre_start_required to false in get_device_plugin_options #554

Merged

Conversation

johnsonshih
Copy link
Contributor

@johnsonshih johnsonshih commented Feb 9, 2023

Signed-off-by: johnsonshih jshih@microsoft.com

What this PR does / why we need it:
Set pre_start_required to false in get_device_plugin_options to match the setting in register().

We set pre_start_required to false when akri instance device plugin registers itself to kubelet but kubelet still calls get_device_plugin_options to decide if to call device plugin's pre_start_container() In get_device_plugin_options(), we return true for pre_start_required so kubelet calls pre_start_container(). We don't expect the pre_start_container() to be called so we dump an error message and return empty option. Since we don't want the pre_start_container() to be called, we should set pre_start_required to false in get_device_plugin_options().

Special notes for your reviewer:

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

Signed-off-by: johnsonshih <jshih@microsoft.com>
@kate-goldenring
Copy link
Contributor

kate-goldenring commented Feb 22, 2023

@johnsonshih can you provide some context on why this change is desired and the anticipated impact?

@johnsonshih
Copy link
Contributor Author

johnsonshih commented Feb 27, 2023

@johnsonshih can you provide some context on why this change is desired and the anticipated impact?

We set pre_start_required to false when akri instance device plugin registers itself to kubelet but kubelet still calls get_device_plugin_options to decide if to call device plugin's pre_start_container() In get_device_plugin_options(), we return true for pre_start_required so kubelet calls pre_start_container(). We don't expect the pre_start_container() to be called so we dump an error message and return empty option. Since we don't want the pre_start_container() to be called, we should set pre_start_required to false in get_device_plugin_options().

The impact is an unnecessary error message shown the might cause confusion.

@johnsonshih
Copy link
Contributor Author

/add-same-version-label

@github-actions
Copy link
Contributor

👋 Added [same version] label :)!

@github-actions
Copy link
Contributor

👋 Added [same version] label :)!

@yujinkim-msft yujinkim-msft merged commit 6fe39ec into project-akri:main Apr 4, 2023
33 checks passed
@johnsonshih johnsonshih deleted the user/jshih/no-pre-start-required branch April 5, 2023 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants