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

resmgr: disallow talking to untested runtimes by default. #821

Merged
merged 1 commit into from Jun 14, 2022

Conversation

klihub
Copy link
Contributor

@klihub klihub commented Jun 2, 2022

Disallow talking to/proxying for unknown runtimes, unless explicitly told to do so on the command line. This could save us quite a bit of time from chasing ghosts.

Inspired by an idea from @jukkar over a lunch discussion. Thanks !

jukkar
jukkar previously approved these changes Jun 2, 2022
marquiz
marquiz previously approved these changes Jun 2, 2022
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Heh 👌
/lgtm

@askervin
Copy link
Contributor

askervin commented Jun 2, 2022

A bit mixed feelings about this... a warning would have been easier for me, but at the same time I know that nobody would notice the warning, except for us when we are after a ghost already.

And on the other hand the scenario where we should to take back this change and allow docker by default, seems to be highly unlikely, says the crystal ball.

As a downside, we will need to add a couple of sentences about docker in our documentation, yet getting rid of it completely would be even nicer.

So after all, I suppose this is ok.

@klihub klihub dismissed stale reviews from marquiz and jukkar via a5c68a5 June 2, 2022 14:56
@klihub klihub force-pushed the fixes/reject-docker-by-default branch from bc172a1 to a5c68a5 Compare June 2, 2022 14:56
@klihub
Copy link
Contributor Author

klihub commented Jun 2, 2022

A bit mixed feelings about this... a warning would have been easier for me, but at the same time I know that nobody would notice the warning, except for us when we are after a ghost already.

There are mainly two reasons I chose to go with this approach:

  1. What you also state above: nobody seems to read logs/warnings and if they do, they don't seem to pay attention.
  2. We really want usage of untested runtimes to be opt-in. It should not work out of the box.

And on the other hand the scenario where we should to take back this change and allow docker by default, seems to be highly unlikely, says the crystal ball.

As a downside, we will need to add a couple of sentences about docker in our documentation, yet getting rid of it completely would be even nicer.

I think we should remove all documentation reference to docker. And add a single paragraph stating that:

  1. Using untested runtimes is highly discouraged.
  2. People should not expect feature parity with containerd/crio when using untested runtimes.
  3. --allow-untested-runtimes must be passed on the command line to cri-resmgr, otherwise it will refuse to talk to untested runtimes

So after all, I suppose this is ok.

I added a mention of the necessary command line option to the rejection error message.

@klihub klihub force-pushed the fixes/reject-docker-by-default branch 5 times, most recently from e39a07d to 28a38c3 Compare June 2, 2022 20:54
@klihub
Copy link
Contributor Author

klihub commented Jun 2, 2022

As a downside, we will need to add a couple of sentences about docker in our documentation, yet getting rid of it completely would be even nicer.

I wiped out all existing instructions related to docker (I could identify with a simple git grep) from our documentation and added a single paragraph along the lines I outlined above above.

jukkar
jukkar previously approved these changes Jun 3, 2022
@klihub klihub force-pushed the fixes/reject-docker-by-default branch from 28a38c3 to 0360534 Compare June 6, 2022 08:01
@klihub klihub changed the title resmgr: disallow docker-based runtimes by default. resmgr: disallow talking to untested runtimes by default. Jun 6, 2022
@klihub klihub force-pushed the fixes/reject-docker-by-default branch from 0360534 to aa7fa29 Compare June 6, 2022 08:07
Refuse to talk to untested runtimes unless explicitly told
to do so using the --allow-untested-runtimes command line
option.
@klihub klihub force-pushed the fixes/reject-docker-by-default branch from aa7fa29 to bc17218 Compare June 6, 2022 08:27
@klihub klihub merged commit f140ae9 into intel:master Jun 14, 2022
@klihub klihub deleted the fixes/reject-docker-by-default branch June 14, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants