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.

Sign up for GitHub

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

Jump to bottom

[cinder-csi-plugin] support ignore-volume-az #1307

Merged
merged 2 commits into from
Nov 19, 2020
Merged

[cinder-csi-plugin] support ignore-volume-az #1307

merged 2 commits into from
Nov 19, 2020

Conversation

ramineni
Copy link
Contributor

@ramineni ramineni commented Nov 5, 2020 •
edited
Loading

What this PR does / why we need it:

Which issue this PR fixes(if applicable):
fixes #1300

Special notes for reviewers:

Release note:

[cinder-csi-plugin] support `ignore-volume-az` config parameter.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 5, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 5, 2020
@ramineni ramineni mentioned this pull request Nov 5, 2020
Closed
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 5, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 5, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 5, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 5, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 5, 2020

Build succeeded.

@zetaab
Copy link
Member

zetaab commented Nov 5, 2020

@ramineni @jichenjc there are now two prs that are trying to solve this same issue. However, this PR uses reading the ignore-volume-az value from config, which means that we are backwards compatbile. So if this PR works, I would prefer merging this PR

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 5, 2020

Build failed.

@jichenjc
Copy link
Contributor

jichenjc commented Nov 5, 2020

@ramineni @zetaab works for me if this is better , I can close #1300 when this is merged

@zetaab
Copy link
Member

zetaab commented Nov 5, 2020

I have verified this PR against openstack which compute zones does not match to volume zone, and it works. However, I will still verify this in openstack which compute zones match to volume zones (and using topology).

zetaab
zetaab approved these changes Nov 5, 2020
View reviewed changes
Copy link
Member

@zetaab zetaab left a comment

Choose a reason for hiding this comment

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

This works like should! awesome, thank you for both of you @ramineni and @jichenjc but like I said already: this PR do have backward compatibility using cloud config

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2020
@zetaab
Copy link
Member

zetaab commented Nov 5, 2020

/retest cloud-provider-openstack-multinode-csi-migration-test

@k8s-ci-robot
Copy link
Contributor

@zetaab: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/retest cloud-provider-openstack-multinode-csi-migration-test

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 k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 5, 2020
@zetaab
Copy link
Member

zetaab commented Nov 5, 2020

how to restart that one failing test?

@jichenjc
Copy link
Contributor

jichenjc commented Nov 5, 2020

/test pull-cloud-provider-openstack-test

@zetaab due to theopenlab/openlab#603 , we are blocked now
I think we can overwrite this error and force merge as it's not reltaed issue.. let me know your opinion

@jichenjc
Copy link
Contributor

jichenjc commented Nov 5, 2020

/retest

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 6, 2020

Build succeeded.

@jichenjc
Copy link
Contributor

jichenjc commented Nov 6, 2020

we can try following to overwrite, but I am not sure it's a good way ..
override openlab/cloud-provider-openstack-acceptance-test-csi-cinder

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 6, 2020

Build succeeded.

@jichenjc
Copy link
Contributor

jichenjc commented Nov 6, 2020

/lgtm

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 6, 2020

Build failed.

lingxiankong
lingxiankong suggested changes Nov 8, 2020
View reviewed changes
Copy link
Contributor

@lingxiankong lingxiankong left a comment

Choose a reason for hiding this comment

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

/approve cancel

docs/cinder-csi-plugin/using-cinder-csi-plugin.md Outdated
@@ -67,6 +67,8 @@ These configuration options pertain to block storage and should appear in the `[
Optional. To configure maximum volumes that can be attached to the node. Its default value is `256`.
* `rescan-on-resize`
Optional. Set to `true`, to rescan block device and verify its size before expanding the filesystem. Not all hypervizors have a /sys/class/block/XXX/device/rescan location, therefore if you enable this option and your hypervizor doesn't support this, you'll get a warning log on resize event. It is recommended to disable this option in this case. Defaults to `false`
* `ignore-volume-az`
Optional. Set to `true`, incase volume AZ is different from node AZ and to avoid populating accessible topologies to volume AZ.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a more detailed description of this option? I'm a little confused about how to configure this option properly, e.g. What's the default value? In which case it should be set to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lingxiankong @zetaab updated. Let me know if its bit more clear now. Thanks.

@lingxiankong lingxiankong removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2020
@zetaab
Copy link
Member

zetaab commented Nov 11, 2020

@ramineni can you add more detailed description to documentation? You can perhaps take a look legacy cinder if that contains helpful information

@ramineni ramineni force-pushed the support-ignore-volume-az branch from 4575be7 to 06184a2 Compare November 12, 2020 06:35
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2020
@jichenjc
Copy link
Contributor

the wording looks good to me
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2020
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 12, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 12, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 12, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 12, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 12, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 12, 2020

Build succeeded.

@lingxiankong
Copy link
Contributor

/lgtm

@zetaab Please approve if it looks good to you as well.

@jichenjc
Copy link
Contributor

this has been here for a while and already tested works well
so we can use follow up to fix any remaining issue
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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 Nov 19, 2020
@k8s-ci-robot k8s-ci-robot merged commit 05badc9 into kubernetes:master Nov 19, 2020
@chnyda chnyda mentioned this pull request May 19, 2021
Merged
powellchristoph pushed a commit to powellchristoph/cloud-provider-openstack that referenced this pull request Jan 19, 2022
@ramineni
[cinder-csi-plugin] support ignore-volume-az (kubernetes#1307)
a17c7b4 
* [cinder-csi-plugin] support ignore-volume-az

* Update using-cinder-csi-plugin.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lingxiankong lingxiankong lingxiankong requested changes

@zetaab zetaab zetaab approved these changes

@chrigl chrigl Awaiting requested review from chrigl

@ricolin ricolin Awaiting requested review from ricolin

Assignees

@lingxiankong lingxiankong

@jichenjc jichenjc

@zetaab zetaab

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[cinder-csi-plugin] IgnoreVolumeAZ does not work for volumes with Topology
5 participants
@ramineni @zetaab @jichenjc @k8s-ci-robot @lingxiankong