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

Check for iptables file before determining container is running #8565

Merged
merged 2 commits into from
Jun 26, 2020

Conversation

priyawadhwa
Copy link

@priyawadhwa priyawadhwa commented Jun 26, 2020

Both #8179 and #8163 are happening because /var/lib/dpkg/alternatives/iptables does not exist in the container -- the kic entrypoint.sh file tries to access it & exits with exit code 2 when it isn't there. This is likely caused by a race condition between mounting the minikube volume to /var & the entrypoint file trying to access the iptables file

The correct fix for this problem is to address the race condition, but that will be a bigger task (as described here by @afbjorklund: #8509 (comment))

Until we fix the main issue, I think it would be nice to just check for the file so that we can quickly determine if the container will fail because the file doesn't exist and automatically restart the container (that seems to fix the issue every time I've experienced it in Cloud Shell)

I'm very open to suggestions if someone can think of a better idea for this :)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: priyawadhwa

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 Jun 26, 2020
@priyawadhwa
Copy link
Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 26, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
docker Driver

pkg/drivers/kic/oci/oci.go Outdated Show resolved Hide resolved
pkg/drivers/kic/oci/oci.go Show resolved Hide resolved
@medyagh
Copy link
Member

medyagh commented Jun 26, 2020

Is there evidence that this would make that bug go away ?

Is there away we can mass test this to see if the failure goes way with this PR vs before this PR ?

I am thinking when the failure happens it's in entry point and makes the container to exit...so maybe this won't fix.

But I could be wrong

@priyawadhwa
Copy link
Author

@medyagh this doesn't fix the bug, it just tries to catch it and restarts the container if it happens.

It's a temporary fix to get this working in cloud shell, but the real fix would be to fix the /var mounting issue

@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [64.847612872 67.171161587 63.079373188000005]
Average time for minikube: 65.03271588233332

Times for Minikube (PR 8565): [62.660223935000005 60.649324478000004 62.955862078]
Average time for Minikube (PR 8565): 62.08847016366667

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 8565) |
+--------------------------------+-----------+--------------------+
| * minikube v1.12.0-beta.0 on   |  0.069087 |           0.057993 |
| Debian 9.11                    |           |                    |
| * Using the kvm2 driver based  |  0.019968 |           0.018851 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.005000 |           0.011584 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 40.526073 |          40.620894 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.3 | 22.493130 |          19.902680 |
| on Docker 19.03.8 ...          |           |                    |
| * Verifying Kubernetes         |  1.389454 |           1.275304 |
| components...                  |           |                    |
| * Enabled addons:              |  0.440151 |           0.131518 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.086436 |           0.065788 |
| configured to use "minikube"   |           |                    |
|                                |  0.003416 |           0.003858 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [25.710086553 25.869257782000002 27.137798699000005]
Average time for minikube: 26.239047678000002

Times for Minikube (PR 8565): [25.578442887 26.010349755999997 25.513576100999998]
Average time for Minikube (PR 8565): 25.700789581333336

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 8565) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.12.0-beta.0 on           |  0.076495 |           0.071624 |
| Debian 9.11                            |           |                    |
| * Using the docker driver              |  0.002608 |           0.002466 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.057269 |           0.056016 |
| minikube in cluster minikube           |           |                    |
| * Creating docker container            |  7.465206 |           7.508883 |
| (CPUs=2, Memory=3700MB) ...            |           |                    |
| * Preparing Kubernetes v1.18.3         |  0.117468 |           0.117897 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 17.573013 |          17.013768 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Verifying Kubernetes                 |  0.833888 |           0.804901 |
| components...                          |           |                    |
| * Enabled addons:                      |  0.046829 |           0.055542 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.063394 |           0.061832 |
| configured to use "minikube"           |           |                    |
|                                        |  0.002878 |           0.007861 |
+----------------------------------------+-----------+--------------------+

@medyagh
Copy link
Member

medyagh commented Jun 26, 2020

@medyagh this doesn't fix the bug, it just tries to catch it and restarts the container if it happens.

It's a temporary fix to get this working in cloud shell, but the real fix would be to fix the /var mounting issue
@priyawadhwa
I dont think the code restart its,
the code only waits for the status to be running, it wont try to restart

	// retry up to up 13 seconds to make sure the created container status is running.
	if err := retry.Expo(checkRunning, 13*time.Millisecond, time.Second*13); err != nil {
		LogContainerDebug(p.OCIBinary, p.Name)
		_, err := DaemonInfo(p.OCIBinary)
		if err != nil {
			return errors.Wrapf(ErrDaemonInfo, "container name %q", p.Name)
		}
		return errors.Wrapf(ErrExitedUnexpectedly, "container name %q", p.Name)
	}

Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

This PR is harmless and might help with debugging, but please see the comment that it does not restart the container if doesnt there

@priyawadhwa priyawadhwa merged commit 7d79241 into kubernetes:master Jun 26, 2020
@priyawadhwa priyawadhwa deleted the update-alternatives branch June 26, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants