fc: Drop multipath.conf snippet #36698
Conversation
|
Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot 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 will still work. Regular contributors should join the org to skip this step.
If you have questions or suggestions related to this bot's behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
|
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
@rootfs Could you please review this patch. |
|
I signed the CNCF CLA as an employee, and my email address I used for submission is also associated to my github account. Let me know what else I should check to get this recognized by the bot. |
k8s-github-robot
added
size/S
|
@k8s-bot ok to test |
|
@fabiand can you please leave a message for the bot exactly with the words "I signed it!" in a new comment? |
|
I signed it! |
|
@kubernetes/test-infra-admins Can you please check why we still have cla:no after following all instructions from the bot |
|
@foxish can you have a look? |
|
multipath.conf is used for devicemapper. If it is not there, devicemapper multipath won't start properly. |
|
Right - But who is taking care to bring up multipathd once the file is written? Also: I wonder if not the OS is responsible for configuring multipath. |
|
Yes, FC hosts are expected to have multipathd systemd service enabled and started. |
|
Even if multipathd.service is enabled and started, then it still has no effect, because multipathd must be reloaded/-started to pickup the new |
|
Besides that - The kubelet should not write multipath conf. Depending on the default settings of multipath conf it could wreck an OS - the device names could change on a next reboot, which could lead to an unbootable host. |
|
No, the code doesn't overwrite existing multipath.conf. It creates one only if none is there. |
|
I would object if I can :) I think it's just wrong to do it from the kubelet. Instead of modifying it I'd then suggest to rather raise a warning or alike. multipath.conf is really something to the host, and not something an application like the kubelet should manage or modify. Also: If you run the kubelet in a container - then any multipath configuration will not have any effect. |
|
ok, I buy your containerized kubelet point :) |
|
@dims We still haven't transitioned to the CNCF CLA and require the Google CLA at this time. The transition is likely to happen during this week or the next. |
|
@foxish Ack. thanks! |
|
@rootfs I've added an README to cover the multipath configuration which applies to both (or at least) iSCSI and FC. I actually wonder if multipath is really working for iSCSI today .. |
k8s-github-robot
added
size/M
|
|
@fabiand I don't have an iscsi multipath to verify either. |
|
@rootfs fhe failure seems to be unrelated. Can it be retriggered? |
k8s-github-robot
added
release-note
|
Yes - I'll get to it again! |
|
/approve |
|
... and nothing happened, interesting. Anyway, our Jenkins complains: Please run hack/update-munge-docs.sh and force-push this branch so it can be merged, thanks. |
k8s-github-robot
added
approved
A minimalistic multipath.conf got written, but it was useless, as it is unclear if multipathd is running and there was also no config reload triggered. This patch drops this snippet. In general it's probably a better idea to leave the multipath.conf to the component managing the host. Signed-off-by: Fabian Deutsch <[email protected]>
Add some lines about how to enable multipath for block storage. A new README was added, because multipath is relevant for at least FC and iSCSI. Signed-off-by: Fabian Deutsch <[email protected]>
|
Fabian has signed CLA and google-bot knows about it, as it can be seen e.g. in #41408. There is something wrong in this PR, I'm labeling it as cla-yes manually. |
jsafrane
added
the
cncf-cla: yes
|
I've rebased the patch and force-pushed it to the branch. Let's see what the automation comes up now with. |
fabiand
force-pushed
the
no-mpathconf
branch
from
95d8e64
to
2367d7d
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: fabiand, jsafrane Needs approval from an approver in each of these OWNERS Files:
We suggest the following people: |
|
@k8s-bot gce etcd3 e2e test this |
|
@jsafrane anything I need to provide? or just the usual test cycles? |
|
@fabiand, Kubernetes is frozen due to 1.6 release, merge queue should open later today. This PR is in the right state, prepared for merging, no action is needed. |
|
Automatic merge from submit-queue |
What this PR does / why we need it:
Removes multipath.conf - The code does not make use of it - or ensure s that it's getting used - and it should in addition be handled elsewehre.
Which issue this PR fixes (optional, in
fixes #<issue number>(, #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note:
A minimalistic multipath.conf got written, but it was useless, as
it is unclear if multipathd is running and there was also no
config reload triggered.
This patch drops this snippet. In general it's probably a better idea
to leave the multipath.conf to the component managing the host.
Signed-off-by: Fabian Deutsch [email protected]
This change is