Skip to content

Conversation

@maxwelldb
Copy link
Contributor

@maxwelldb maxwelldb added this to the Future Release milestone Aug 17, 2020
@maxwelldb maxwelldb self-assigned this Aug 17, 2020
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 17, 2020
@maxwelldb
Copy link
Contributor Author

@iamemilio Not at all sure if I read the upstream changes for this correctly--especially WRT machinesets. Can you review?

FYI @pierreprinetti

@openshift-docs-preview-bot

The preview will be available shortly at:

@maxwelldb
Copy link
Contributor Author

Added 807633f due to @pierreprinetti's feedback.

@maxwelldb maxwelldb changed the title Draft of param changes for OSP AZ support Param changes for OSP AZ support Aug 18, 2020
@pierreprinetti
Copy link
Member

LGTM 👍

@maxwelldb
Copy link
Contributor Author

maxwelldb commented Aug 19, 2020 via email

@maxwelldb maxwelldb marked this pull request as ready for review August 19, 2020 16:09
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 19, 2020
@luis5tb
Copy link
Contributor

luis5tb commented Aug 28, 2020

This is missing a couple of important points:

  • It must highlight that it is availability zones just for nova, not for cinder or other components that may needed (octavia, manila?)
  • Related to Octavia LoadBalancer VMs, we need to highlight that when deploying with Kuryr, as Octavia does not support availability zones, that means that event though all the OCP VMs will be in the desired AZs, the Load Balancers will not. And there is 2 cases for Kuryr. One where ovn-octavia provider is used, and thus only one amphora VM is there not maching the AZ. And the other case where amphora provider is used, thus any OCP service will be backed up by an amphora VM which may not be running on the stated AZ.

@luis5tb
Copy link
Contributor

luis5tb commented Aug 28, 2020

This is missing a couple of important points:

* It must highlight that it is availability zones just for nova, not for cinder or other components that may needed (octavia, manila?)

* Related to Octavia LoadBalancer VMs, we need to highlight that when deploying with Kuryr, as Octavia does not support availability zones, that means that event though all the OCP VMs will be in the desired AZs, the Load Balancers will not. And there is 2 cases for Kuryr. One where ovn-octavia provider is used, and thus only one amphora VM is there not maching the AZ. And the other case where amphora provider is used, thus any OCP service will be backed up by an amphora VM which may not be running on the stated AZ.

And for the record, I sent this to openshift/installer docs: openshift/installer#4109

@maxwelldb
Copy link
Contributor Author

@luis5tb c5d23e3 adds Compute/Nova to the parameters.

As to this:

Related to Octavia LoadBalancer VMs, we need to highlight that when deploying with Kuryr, as Octavia does not support availability zones, that means that event though all the OCP VMs will be in the desired AZs, the Load Balancers will not. And there is 2 cases for Kuryr. One where ovn-octavia provider is used, and thus only one amphora VM is there not maching the AZ. And the other case where amphora provider is used, thus any OCP service will be backed up by an amphora VM which may not be running on the stated AZ.

Would you agree that this information should be added to the existing body of Kuryr + load balancing content, or is it important enough to add it to this more general list of parameters?

@maxwelldb maxwelldb requested a review from luis5tb September 1, 2020 19:13
@luis5tb
Copy link
Contributor

luis5tb commented Sep 15, 2020

@luis5tb c5d23e3 adds Compute/Nova to the parameters.

As to this:

Related to Octavia LoadBalancer VMs, we need to highlight that when deploying with Kuryr, as Octavia does not support availability zones, that means that event though all the OCP VMs will be in the desired AZs, the Load Balancers will not. And there is 2 cases for Kuryr. One where ovn-octavia provider is used, and thus only one amphora VM is there not maching the AZ. And the other case where amphora provider is used, thus any OCP service will be backed up by an amphora VM which may not be running on the stated AZ.

Would you agree that this information should be added to the existing body of Kuryr + load balancing content, or is it important enough to add it to this more general list of parameters?

Sounds good!

@openshift-ci-robot openshift-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 18, 2020
@maxwelldb
Copy link
Contributor Author

@mandre @morenod Any updates about this?

@maxwelldb
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 8, 2020
@maxwelldb maxwelldb removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2020
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 9, 2020
Copy link

@morenod morenod left a comment

Choose a reason for hiding this comment

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

Lgtm

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2020
@maxwelldb maxwelldb force-pushed the osp-azs-osdocs1277 branch 2 times, most recently from d11a82c to fc60f3c Compare October 9, 2020 16:31
@maxwelldb maxwelldb added the peer-review-needed Signifies that the peer review team needs to review this PR label Oct 9, 2020
@codyhoag codyhoag self-requested a review October 9, 2020 17:21
Copy link
Contributor

@codyhoag codyhoag left a comment

Choose a reason for hiding this comment

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

A couple nits; otherwise, the changes look good!

Make sure to squash your commits before merging 🙂

@codyhoag codyhoag added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Oct 9, 2020
@maxwelldb maxwelldb merged commit 66f1270 into openshift:master Oct 9, 2020
@maxwelldb
Copy link
Contributor Author

/cherry-pick enterprise-4.6

@maxwelldb maxwelldb deleted the osp-azs-osdocs1277 branch October 9, 2020 17:43
@openshift-cherrypick-robot

@maxwelldb: #24834 failed to apply on top of branch "enterprise-4.6":

Applying: Add availability zone support for ShiftStack
Using index info to reconstruct a base tree...
M	modules/installation-configuration-parameters.adoc
Falling back to patching base and 3-way merge...
Auto-merging modules/installation-configuration-parameters.adoc
CONFLICT (content): Merge conflict in modules/installation-configuration-parameters.adoc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add availability zone support for ShiftStack
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick enterprise-4.6

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.