-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Extend node e2e test suite with containerized Kubelet #56250
Extend node e2e test suite with containerized Kubelet #56250
Conversation
5bb16e6
to
1636395
Compare
How relevant is it to implement the non-systemd variant? |
/test pull-kubernetes-unit |
test/e2e_node/services/kubelet.go
Outdated
"-v", "/etc/machine-id:/etc/machine-id:ro", | ||
"-v", "/etc/systemd/system:/host-etc/systemd/system", | ||
"-v", filepath.Dir(kubeconfigPath)+":/etc/kubernetes", | ||
"-v", "/lib/modules:/lib/modules", |
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.
loading kernel modules?
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.
I got inspired by the openshift-node service file. If not needed, I can remove it.
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.
I think we can remove these two (at least).
test/e2e_node/services/kubelet.go
Outdated
"-e HOST=/rootfs", "-e HOST_ETC=/host-etc", | ||
"-v", "/etc/localtime:/etc/localtime:ro", | ||
"-v", "/etc/machine-id:/etc/machine-id:ro", | ||
"-v", "/etc/systemd/system:/host-etc/systemd/system", |
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.
what is this needed for?
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.
I got inspired by the openshift-node service file. If not needed, I can remove it.
What specifically makes this systemd only? |
If the systemd-run binary is present, the systemd service is created. Otherwise, a binary on a background is run. So it is needed by the non-systemd OSes. |
Ah I see. So you are just adding support for the systemd path in this PR atm. How much trouble would it be to implement the non-systemd path as well? This might help our case for upstream containerized kubelet testing. |
3554f6f
to
607e863
Compare
/sig node |
/release-note-none |
@dchen1107 PTAL |
/unassign @dims |
this looks fine to me. /lgtm |
/approve no-issue |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, ingvagabund Associated issue requirement bypassed by: derekwaynecarr The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/retest |
/retest Review the full test history for this PR. |
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 with minor nit comments :)
@@ -93,6 +97,10 @@ const ( | |||
// startKubelet starts the Kubelet in a separate process or returns an error | |||
// if the Kubelet fails to start. | |||
func (e *E2EServices) startKubelet() (*server, error) { | |||
if kubeletContainerized && hyperkubeImage == "" { | |||
return nil, fmt.Errorf("the --hyperkube-image option must be set") |
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.
nit: the --hyperkube-image option must be set if we run kubelet in a docker container
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.
The error message pops up only when the --containerized
flag is set. Later, the --hyperkube-image
can point to a container of non-docker container runtime. There is always a space for ambiguity. However, the hyperkube
+ image
connection is currently strongly voting for containerized run, so the confusion will be minimal. But yeah, updating the message would help to make the statement clearer :).
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.
SGTM.
/test all [submit-queue is verifying that this PR is safe to merge] |
@ingvagabund: The following test failed, say
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. |
Automatic merge from submit-queue (batch tested with PRs 56250, 56809, 56812, 56792, 56724). If you want to cherry-pick this change to another branch, please follow the instructions here. |
…ntal-on-containerized-kubelet Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Containerized kubelet is no longer experimental Blocked by #56250 Given the node e2e Conformance tests over containerized ``Kubelet`` are already running and are published at https://k8s-testgrid.appspot.com/sig-node-kubelet#kubelet-containerized-conformance-aws-e2e-rhel, we can remove all mentions of the "experimental" keyword. Are there any other place where the "containerized Kubelet" is mentioned as experimental? ```release-note NONE ```
How to run it?