Removes storage_driver option from /etc/docker/daemon.json #16235
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
cncf-cla: yes
|
Hi @x7upLime. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: x7upLime The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/XS
|
This was the workaround.. I'm wondering about linux distros that are not using systemd.. |
|
Can one of the admins verify this patch? |
|
The situation is as follows: 1st storage-driveris set by minikube indiscriminately when configuring the docker cruntime() 2nd storage-driveris set as an override for the systemd service file, by libmachine code, that minikube relias upon... with every other driver we call minikube code for provisioning of the machine, with ssh/none driver, we default to the provisioner of libmachine. |
|
minikube implements 2 libmachine-compatible provisioners: Our storage-driver gets set by the provisioner's method |
|
solutions I can think of..
But: "changing a struct (Config) and related initializer, that gets called across all the codebase?"
|
|
or... |
It would make more sense to have the none and ssh drivers use the minikube provisioner, instead of mixing/matching. I'm not sure where this 10-machine.conf comes from, and we should probably not set it up in the first place ?
The minikube provisioner is currently broken (it doesn't provision docker), but it should be improved instead. I think it only needs to support the ubuntu and the rhel systems, and not all the other system provisioners. |
afbjorklund
requested changes
Apr 11, 2023
There was a problem hiding this comment.
I think the provisioner should be changed instead, and leave cruntime for now...
The /etc/docker/daemon.json comes from the old kubernetes installation docs
sudo mkdir /etc/docker
cat <<EOF | sudo tee /etc/docker/daemon.json
{
"exec-opts": ["native.cgroupdriver=systemd"],
"log-driver": "json-file",
"log-opts": {
"max-size": "100m"
},
"storage-driver": "overlay2",
"storage-opts": [
"overlay2.override_kernel_check=true"
]
}
EOF
If i remember correctly, that comes from the systemd provisioner of the docker-machine lib. That is the default behavior of docker-machine's provisioners: it is the provisioner that sets the storage-driver as a .service ExecStart= override.
If I understand correctly, you're suggesting that in order to address this issue and the kind, we should first refactor the provisioner code for the none/generic driver (minikube side). |
|
I can see that inside the minikube codebase, we're using the miniProvisioner interface: instead of the Provisioner interface of docker-machine: the former being compatible with the latter.. that's why we're able to use docker-machine provisioners. |
|
I can see that to adjust the provisioner for minikube, to make it really provision machines, with more than a container runtime, would be a great effort and a major rework.. which I would be more than happy to take a shot at 😜 But I'm wondering.. is this something that could be merged soon? @medyagh @spowelljr what do you think about it? |
|
Fixing the minikube Provisioner would be a major effort, agreed. And yes, I meant creating a new provisioner for none and generic |
x7upLime
force-pushed
the
fix-cr_docker_config
branch
from
828bc08
to
47586dc
Compare
k8s-ci-robot
added
size/S
|
taking into considerations all the opinions collected between zoom/slack/github. |
|
as @spowelljr pointed out, there may be calls to the provisioner between |
|
Cluster seems to continue running between And the journalctl logs:I can see that at least for a while, there are two storage-driver directives.. But then I cannot see the storage-driver inside /etc/docker/daemon.json. How bad is that?
|
|
ok-to-build-iso |
|
Hi @x7upLime, we have updated your PR with the reference to newly built ISO. Pull the changes locally if you want to test with them or update your PR further. |
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
k8s-ci-robot
removed
the
needs-ok-to-test
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
x7upLime
force-pushed
the
fix-cr_docker_config
branch
from
6b1ad09
to
3129299
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The code for the docker runtime (*Docker).setCGroup used to configure the storage driver as well.. which could be seen as a wrong behaviour. Also we're removing the storage-driver directive from the daemon.json that are baked inside the iso.. However we want to see it.. it is not optimal to bake a config inside the iso used by the vm driver; to build a new iso is painful
x7upLime
force-pushed
the
fix-cr_docker_config
branch
from
3129299
to
38ee82a
Compare
|
/retest |
|
/retest-this-please |
|
kvm2 driver with docker runtime Times for minikube (PR 16235) start: 53.4s 53.6s 51.7s 52.6s 52.1s Times for minikube ingress: 27.8s 25.8s 30.3s 24.7s 28.2s docker driver with docker runtime Times for minikube ingress: 22.0s 21.0s 23.0s 21.0s 81.5s Times for minikube start: 21.6s 23.4s 22.2s 25.1s 22.8s docker driver with containerd runtime Times for minikube start: 20.5s 20.7s 23.5s 20.5s 20.4s Times for minikube ingress: 32.0s 47.6s 32.5s 31.5s 47.6s |
|
These are the flake rates of all failed tests.
To see the flake rates of all tests by environment, click here. |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
Seeing the same issue in debian... Any progress on this fix? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This setting is applied by minikube in both /etc/docker/daemon.json and as an override to the service file in
/etc/systemd/system/docker.service.d/10-machine.conf, during provision phase.
If two such directives are set, docker.service will not start
fixes #16231