[cinder-csi-plugin] Allow multiple config files #1476
Conversation
k8s-ci-robot
added
release-note
|
Welcome @mdbooth! |
|
Hi @mdbooth. 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. |
k8s-ci-robot
added
the
size/M
|
@mdbooth: GitHub didn't allow me to request PR reviews from the following users: mandre. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: 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. |
|
Build failed.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
Build failed.
|
|
Build failed.
|
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@mdbooth can you help to open an issue and put the reason/purpose of this PR there? |
mdbooth
force-pushed
the
multiple-config-files
branch
from
c85cd64
to
f86cfc3
Compare
|
Build succeeded.
|
|
Build succeeded.
|
|
Build failed.
|
|
Build succeeded.
|
|
Build failed.
|
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@mdbooth: The specified target(s) for
Use In response to this:
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. |
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@lingxiankong: The specified target(s) for
Use In response to this:
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. |
|
Build failed.
|
| This will be added as metadata to every Cinder volume created by this plugin. | ||
| </dd> | ||
| </dl> | ||
|
|
There was a problem hiding this comment.
Could you convert those using markdown? The pure HTML would make it harder to maintain.
There was a problem hiding this comment.
I can't, because AFAICT this is the 'supported' way to do description lists. I don't like it either, but I couldn't find anything better.
Do you know a better way to do this?
There was a problem hiding this comment.
|
I can confirm the failed jobs have nothing to do with this PR, from the log, the cinder-csi components are all up and running. The failure is due to the CI cloud provider issue. @jichenjc @ramineni @chrigl we can also consider using the similar CI job I did for occm, so we could have more control of the cloud resources (in devstack) created, also benefit from the latest version Cinder features. For this PR, please leave your comments, we shouldn't let the contributors wait for so long for the reasons they couldn't control. |
k8s-ci-robot
added
the
lgtm
|
updated description to link correct issue. |
Will do, although it will be a few days before I get back to this. |
|
@mdbooth code looks good to me. Please add alternate manifests with examples in seperate PR. |
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@ramineni: The specified target(s) for
Use In response to this:
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. |
|
Build succeeded.
|
|
/test cloud-provider-openstack-multinode-csi-migration-test |
|
@ramineni: The specified target(s) for
Use In response to this:
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. |
|
Build succeeded.
|
|
/approve @mdbooth Please update docs in seperate PR |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ramineni 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 |
k8s-ci-robot
added
the
approved
|
Yesterday csi cinder plugin was able to retrieve instance id using metadata service or config drive, today it seems that nodeid is mandatory. did i miss something?
|
|
What do you mean by "nodeid is mandatory"? From #1565 I can see it's deprecated and shouldn't be needed. |
|
@artpej Yes, Its not a mandatory/required argument and the value is not used anywhere. Please share the logs |
|
@lingxiankong @ramineni , thanks for your response, I just ran another try and everything works as expected (without the need to specify the However the documentation in this PR specify that the Finally thanks for your opensource work i really appreciate, and sorry for my poorly english speaking 😉 . |
@artpej Thanks for pointing. Raised a PR to fix the doc. #1573 |
* [cinder-csi-plugin] Allow multiple --cloud-config (kubernetes#1481) This change allows cinder-csi-plugin to be invoked with multiple config files. These will be parsed sequentially in the order specified. Values in later config files override values in earlier config files. This makes 'layered' configurations simpler to manage. For example a base configuration containing authentication information and a per-node override configuration which applies only to a single node. Keeping these in separate files simplifies the task of changing the base configuration for all nodes. * [cinder-csi-plugin] Document command-line arguments
This change allows cinder-csi-plugin to be invoked with multiple config
files. These will be parsed sequentially in the order specified. Values
in later config files override values in earlier config files.
This makes 'layered' configurations simpler to manage. For example a
base configuration containing authentication information and a per-node
override configuration which applies only to a single node. Keeping
these in separate files simplifies the task of changing the base
configuration for all nodes.
What this PR does / why we need it:
Which issue this PR fixes(if applicable):
fixes #1481
Special notes for reviewers:
Release note: