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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Add existence check for runtime_path #1965

Merged
merged 1 commit into from Jan 5, 2019
Merged

Add existence check for runtime_path #1965

merged 1 commit into from Jan 5, 2019

Conversation

saschagrunert
Copy link
Member

Hey there 馃憢, during my experiments with the Runtimes configuration variable I noticed that there is no check for existence of the runtime_path. This pull request contains an additional sanity check as well as a debug output to see which runtimes got registered during server instantiation.

For later improvements I would suggest to move the configuration validation into the config.go files for a better test-ability. Would you agree to that? 馃槆

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 14, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @saschagrunert. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes 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.

@openshift-ci-robot
Copy link

Hi @saschagrunert. Thanks for your PR.

I'm waiting for a kubernetes-sigs or openshift 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.

@saschagrunert
Copy link
Member Author

saschagrunert commented Dec 14, 2018 •
edited

/assign @rhatdan

@k8s-ci-robot
Copy link
Contributor

@saschagrunert: GitHub didn't allow me to assign the following users: cyphar.

Note that only kubernetes-sigs members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @rhatdan
/assign @cyphar

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.

@giuseppe
Copy link
Member

/test all

@giuseppe
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 14, 2018
@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 14, 2018
@giuseppe
Copy link
Member

/test all

@saschagrunert
Copy link
Member Author

It seems that runc is not available on the travis machine, which we now need for a proper execution. Do we want to add it as dependency there?

@mrunalp
Copy link
Member

mrunalp commented Dec 14, 2018

I think this check makes more sense at runtime rather than config generation. A config could be generated anywhere and then copied to the nodes where crio will run so the machine where it is generated shouldn't require the runtime.

@rhatdan
Copy link
Contributor

rhatdan commented Dec 15, 2018

I agree this should fail when starting CRI-O as a daemon, but not when creating the cri-o config file for example.

@saschagrunert
Copy link
Member Author

I agree this should fail when starting CRI-O as a daemon, but not when creating the cri-o config file for example.

Alright, I will modify it as you suggested and continue here as a work in progress.

@saschagrunert saschagrunert changed the title Add existence check for runtime_path WIP: Add existence check for runtime_path Dec 17, 2018
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 17, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 17, 2018
@saschagrunert saschagrunert force-pushed the runtime-path-validation branch 3 times, most recently from 4d8017a to 288f634 Compare December 17, 2018 11:41
@saschagrunert saschagrunert changed the title WIP: Add existence check for runtime_path Add existence check for runtime_path Dec 17, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 17, 2018
@saschagrunert
Copy link
Member Author

saschagrunert commented Dec 17, 2018 •
edited

Please have a look again, I changed the implementation to consider the actual server execution vs a simple config generation. I also increased the test coverage for the new and existing configuration validations.

@rhatdan
Copy link
Contributor

rhatdan commented Dec 21, 2018

/test all
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 21, 2018
@saschagrunert
Copy link
Member Author

/retest

@rhatdan
Copy link
Contributor

rhatdan commented Dec 24, 2018

/test e2e_rhel

@rhatdan
Copy link
Contributor

rhatdan commented Dec 24, 2018

LGTM

@mrunalp
Copy link
Member

mrunalp commented Jan 3, 2019

This looks fine but the commits need to be cleaned up. The first commit adds code that is subsequently moved in the second commit.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2019
@saschagrunert
Copy link
Member Author

This looks fine but the commits need to be cleaned up. The first commit adds code that is subsequently moved in the second commit.

@mrunalp You're right, I rebased on top of the latest master and squashed them together.

@saschagrunert
Add existence check for runtime_path
334fca7 
This commit contains an additional sanity check as well as a debug
output to see which runtimes got registered during server instantiating.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 3, 2019
@mrunalp
Copy link
Member

mrunalp commented Jan 3, 2019

/test all

@mrunalp
Copy link
Member

mrunalp commented Jan 4, 2019

/test e2e

@mrunalp
Copy link
Member

mrunalp commented Jan 4, 2019

/test critest_rhel

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 4, 2019 •
edited

@saschagrunert: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/kata-jenkins 334fca7 link /test kata-containers

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@mrunalp
Copy link
Member

mrunalp commented Jan 4, 2019

/test e2e_rhel

@mrunalp
Copy link
Member

mrunalp commented Jan 5, 2019

lgtm

@mrunalp mrunalp merged commit 67a11b2 into cri-o:master Jan 5, 2019
@saschagrunert saschagrunert deleted the runtime-path-validation branch January 5, 2019 11:07
@saschagrunert saschagrunert mentioned this pull request Mar 21, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp Awaiting requested review from mrunalp

@runcom runcom Awaiting requested review from runcom

@giuseppe giuseppe Awaiting requested review from giuseppe

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@saschagrunert @k8s-ci-robot @openshift-ci-robot @giuseppe @mrunalp @rhatdan