fix rbd info when return warning information #75588

smileusd · 2019-03-22T09:59:08Z

What type of PR is this?
/kind bug
What this PR does / why we need it:
fix #75581
This pr clean rbd info command output of warning information. If not fix this, provider will parse the output error.
Special notes for your reviewer:
/sig storage
Does this PR introduce a user-facing change?:

Ceph RBD volume plugin now does not use any keyring (/etc/ceph/ceph.client.lvs01cinder.keyring,/etc/ceph/ceph.keyring,/etc/ceph/keyring,/etc/ceph/keyring.bin) for authentication. Ceph user credentials must be provided in PersistentVolume objects and referred Secrets.

k8s-ci-robot · 2019-03-22T09:59:16Z

Hi @smileusd. 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 /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.

smileusd · 2019-04-11T02:13:19Z

Has anyone to review this? @neolit123 @msau42 @cofyc

cofyc · 2019-04-11T03:55:44Z

/ok-to-test

cofyc · 2019-04-11T04:07:29Z

pkg/volume/rbd/rbd_util.go

+	if startJson > endJson {
+		return 0, fmt.Errorf("can't find json result")
+	}
+	if err := json.Unmarshal([]byte(string(output)[startJson: endJson+1]), &info); err != nil {


This way to filter out warning infos is too hacky. And mount.Exec does not provide a method to filter out stderr stream too.
How about using -c /dev/null to override ceph configuration? rbd command will use this empty configuration file, no warnings will be emitted.
Actually, for kubelet rbd plugin, all required information to access ceph cluster are provided from command line arguments.

Sorry to late reply. Actually it is to abort the configurations file all the rbd command if follow you suggested. Is it necessary?

rbd command in this plugin will search and use Ceph configuration if it exists on the host is a side-effect, it is not documented and officially supported I guess. I think we should remove it.

cc @rootfs and @jsafrane what's your opinion?

I don't understand Ceph enough to judge if it's a common thing to configure its client via config in /etc/ on every node. Are all options really configurable via Kubernetes PV? If we remove parsing of /etc/, how likely is some cluster going to break?

There should be at least some a warning in release notes + deprecation period.

It is not a common thing to configure its client via config in /etc on every node. It's hard to maintain (need to copy configurations to every new node), and easy to make mistakes. All required options are configurable via Kubernetes PV and client arguments will override configurations in /etc/ceph.

In fixing this bug, I don't it is necessary to warn users that we don't support reading configurations from /etc/ceph because we have all required arguments in the command line:

https://github.com/kubernetes/kubernetes/blob/ea0099103ea5532b49f5a9e5570d44f33b24bd67/pkg/volume/rbd/rbd_util.go#L702-L704

@cofyc @jsafrane Thank you and I make sense. If required arguments in the command line in code at beginning, it will not break exist cluster. And if we make sure all the arguments of the command will override the options of config file, it is safety to do following @cofyc says. But I will also make annotations to attention user not necessary to config /etc/ceph in code.

@cofyc I just came across this issue and I did a quick test in my environment, your suggestion to use -c /dev/null will still output the following line to stderr:

2019-06-14 11:23:20.741527 7f1d737e60c0 -1 auth: unable to find a keyring on /etc/ceph/ceph.client.kube-admin.keyring,/etc/ceph/ceph.keyring,/etc/ceph/keyring,/etc/ceph/keyring.bin,: (2) No such file or directory

If you go that way you should also include -k /dev/null -K /dev/null to point the keyrings to the empty file, this way only the rbd info will be outputted.

touch a empty /etc/ceph/ceph.conf and /etc/ceph/keyring files should be work for take ceph client warning message away.

@Symbianx You are right, but -K will cover the secret, i will change with -k /dev/null to cover it.

smileusd · 2019-09-18T08:53:42Z

@cofyc @jsafrane PTAL

cofyc · 2019-09-18T09:07:43Z

/lgtm

I think this makes sense. We don't need to search keyring file because we provided the user and secret via --id and --key flags.

jsafrane · 2019-09-26T11:55:11Z

@smileusd, can you please add release note to the first comment of this PR, so Ceph users are aware of this change?

k8s-ci-robot · 2019-10-09T09:39:26Z

@smileusd: The label(s) kind/storage cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

What type of PR is this?
/kind bug
/kind storage
What this PR does / why we need it:
fix #75581
This pr clean rbd info command output of warning information. If not fix this, provider will parse the output error.
Special notes for your reviewer:
Ceph users are aware of this change.
Does this PR introduce a user-facing change?:
None
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
None

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.

smileusd · 2019-10-09T09:40:47Z

@jsafrane updated.

k8s-ci-robot · 2019-10-09T09:43:08Z

@smileusd: The label(s) kind/storage cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

What type of PR is this?
/kind bug
/kind storage
What this PR does / why we need it:
fix #75581
This pr clean rbd info command output of warning information. If not fix this, provider will parse the output error.
Special notes for your reviewer:
Ceph users are not aware of this change because the warning information is useless.
Does this PR introduce a user-facing change?:
None
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
None

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.

jsafrane · 2019-10-15T08:57:08Z

/approve

k8s-ci-robot · 2019-10-15T08:57:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, smileusd

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:

~~pkg/volume/rbd/OWNERS~~ [jsafrane]

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

fejta-bot · 2019-10-15T14:17:17Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-10-15T17:26:19Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 22, 2019

k8s-ci-robot requested review from jingxu97 and rootfs March 22, 2019 10:00

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 Apr 11, 2019

cofyc reviewed Apr 11, 2019

View reviewed changes

fix rbd info when return warning information

e530420

smileusd force-pushed the rbd_info branch from ea00991 to e530420 Compare September 18, 2019 08:50

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2019

k8s-ci-robot assigned cofyc Sep 18, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2019

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 15, 2019

k8s-ci-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Oct 15, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2019

k8s-ci-robot merged commit 0a08798 into kubernetes:master Oct 15, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 15, 2019

smileusd mentioned this pull request Nov 26, 2024

REQUEST: New membership for smileusd kubernetes/org#5274

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix rbd info when return warning information #75588

fix rbd info when return warning information #75588

smileusd commented Mar 22, 2019 •

edited

Loading

k8s-ci-robot commented Mar 22, 2019

smileusd commented Apr 11, 2019 •

edited

Loading

cofyc commented Apr 11, 2019

cofyc Apr 11, 2019 •

edited

Loading

smileusd Apr 17, 2019 •

edited

Loading

cofyc Apr 23, 2019

jsafrane Apr 30, 2019

cofyc May 1, 2019 •

edited

Loading

smileusd May 20, 2019

Symbianx Jun 14, 2019 •

edited

Loading

Ranler Jul 2, 2019

smileusd Sep 18, 2019

smileusd commented Sep 18, 2019

cofyc commented Sep 18, 2019 •

edited

Loading

jsafrane commented Sep 26, 2019

k8s-ci-robot commented Oct 9, 2019

smileusd commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

jsafrane commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

fejta-bot commented Oct 15, 2019

fejta-bot commented Oct 15, 2019

fix rbd info when return warning information #75588

fix rbd info when return warning information #75588

Conversation

smileusd commented Mar 22, 2019 • edited Loading

k8s-ci-robot commented Mar 22, 2019

smileusd commented Apr 11, 2019 • edited Loading

cofyc commented Apr 11, 2019

cofyc Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

smileusd Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

cofyc Apr 23, 2019

Choose a reason for hiding this comment

jsafrane Apr 30, 2019

Choose a reason for hiding this comment

cofyc May 1, 2019 • edited Loading

Choose a reason for hiding this comment

smileusd May 20, 2019

Choose a reason for hiding this comment

Symbianx Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Ranler Jul 2, 2019

Choose a reason for hiding this comment

smileusd Sep 18, 2019

Choose a reason for hiding this comment

smileusd commented Sep 18, 2019

cofyc commented Sep 18, 2019 • edited Loading

jsafrane commented Sep 26, 2019

k8s-ci-robot commented Oct 9, 2019

smileusd commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

jsafrane commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

fejta-bot commented Oct 15, 2019

fejta-bot commented Oct 15, 2019

smileusd commented Mar 22, 2019 •

edited

Loading

smileusd commented Apr 11, 2019 •

edited

Loading

cofyc Apr 11, 2019 •

edited

Loading

smileusd Apr 17, 2019 •

edited

Loading

cofyc May 1, 2019 •

edited

Loading

Symbianx Jun 14, 2019 •

edited

Loading

cofyc commented Sep 18, 2019 •

edited

Loading