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.

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

Fix for Issue #2141 #2155

Merged
merged 4 commits into from
Jan 24, 2018
Merged

Fix for Issue #2141 #2155

merged 4 commits into from
Jan 24, 2018

Conversation

brutus333
Copy link
Contributor

This is a fix for issue: #2141

The solution was to configure a bit more permissive default scheduler policy, by removing the NoVolumeZoneConflict predicate from default policy hosted on kubernetes repo.

One possible improvement is to gather somehow the default policy file and remove only this predicate.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 12, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 12, 2018
@ArchiFleKs
Copy link
Contributor

@brutus333 Looks good, where did you find the default policy file ?

@brutus333
Copy link
Contributor Author

@ArchiFleKs
Copy link
Contributor

ci check this

{
"kind" : "Policy",
"apiVersion" : "v1",
"predicates" : [
Copy link
Contributor

@bradbeam bradbeam Jan 17, 2018

Choose a reason for hiding this comment

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

It feels like these are things that would be worthwhile to abstract into variables, but I'm not super familiar with the use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if there are of any use outside Openstack deployment scenario. In Openstack you may have different AZ setup for compute and storage, not sure if there is any other cloud with the same behavior.
And usually you don't want to disable default scheduler restrictions, do you?

@ArchiFleKs
Copy link
Contributor

ArchiFleKs commented Jan 18, 2018 via email

@cristicalin
Copy link
Contributor

@ArchiFleKs while the best practice is indeed to disallow cross-AZ attachments the fact of the matter is this is not the out of the box behavior of openstack and there are customers out there which have a single large storage system (CEPH especially) that they don't carve out separately between AZs thus the need to allow this kind of behavior.

@brutus333
Copy link
Contributor Author

Ok, in this case I can add a variable for this. What about disable_volume_zone_conflict?
I can push the change if you agree with naming.

@ArchiFleKs
Copy link
Contributor

ArchiFleKs commented Jan 18, 2018

@cristicalin I agree, I was not saying that the feature is unneeded, we just need a way to enable/disable it even for OpenStack

@cristicalin
Copy link
Contributor

@brutus333 maybe allow_openstack_volume_zone_conflict is a better name?
To make it clear this should only apply to OpenStack environments.

@brutus333
Copy link
Contributor Author

@cristicalin: Yes, maybe it's a better name. But it depends whether we will keep disabling the constraint only for openstack (as done in the current patch) or we will allow disabling this constraint on all clouds.
@ArchiFleKs : what do you think? Which approach is better?

@ArchiFleKs
Copy link
Contributor

ArchiFleKs commented Jan 22, 2018

@brutus333 I think it is a bit confusing. In nova.conf the feature is called something az_cross_attachment and I think it would be less confusing to have a variable more generic (without openstack in it) like volume_cross_zone_attachment (or anything else we agree on). if True would mean that is enable and false (which is the case in major provider) would be the default.

@brutus333
Copy link
Contributor Author

@ArchiFleKs: I agree with volume_cross_zone_attachment.
@cristicalin: Do you agree with this new naming?

@cristicalin
Copy link
Contributor

@brutus333 looks ok to me

…ne_attachment and removed cloud provider condition; fix identation
sangwook added a commit to sangwook/kubespray that referenced this pull request Sep 27, 2018
k8s-ci-robot pushed a commit that referenced this pull request Sep 28, 2018
…ion (#2980)

* Better fix for openstack cinder zone issue[1][2]
using ignore-volume-az option[3].
[1]: #2155
[2]: #2346
[3]: kubernetes/kubernetes#53523

* Remove kube-scheduler-policy.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants