Skip to content

Conversation

@kalexand-rh
Copy link
Contributor

https://bugzilla.redhat.com/show_bug.cgi?id=1738456

@wking, are you ok with the way I translated this change?

@kalexand-rh kalexand-rh added this to the Future Release milestone Oct 2, 2019
@kalexand-rh kalexand-rh self-assigned this Oct 2, 2019
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 2, 2019
@openshift-docs-preview-bot

The preview will be available shortly at:

Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

Looks good to me. Needs to go into GCP UPI docs too, but maybe we don't have those yet.

@kalexand-rh
Copy link
Contributor Author

@adellape, please pick up this change in your draft of the GCP UPI docs.

@wking wking mentioned this pull request Oct 7, 2019
@kalexand-rh kalexand-rh added the peer-review-needed Signifies that the peer review team needs to review this PR label Oct 15, 2019
@kalexand-rh
Copy link
Contributor Author

@openshift/team-documentation PTAL

@bergerhoffer bergerhoffer self-requested a review October 15, 2019 12:59
@bergerhoffer bergerhoffer 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 15, 2019
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Oct 15, 2019
@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 15, 2019
@kalexand-rh kalexand-rh force-pushed the BZ1738456 branch 2 times, most recently from c9af188 to d275527 Compare October 15, 2019 19:44
@adellape
Copy link
Contributor

Just merged #17043 which is likely the conflict culprit here.

@kalexand-rh kalexand-rh force-pushed the BZ1738456 branch 3 times, most recently from 06a5b9b to abb54d2 Compare October 16, 2019 01:28
Because you create your own compute machines later in the installation process,
you can safely ignore this warning.

ifdef::aws,gcp[]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jianlinliu, we didn't have a step to remove these manifest files for vSphere or bare metal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After I took another look at the files that I'd need to change to apply this step to vSphere and bare metal, I realized that it would be easier to do it on this PR. For these methods, the 4.1 steps (eg vSphere) skip generating the manifests and just have you generate the Ignition configs. With this change, you need to generate the manifests too (proposed 4.2 change).

The way the conditionals are set up, the 4.2 docs still won't make you remove the control plane or compute manifests.

link:https://github.com/kubernetes/kubernetes/issues/65618[Kubernetes limitation],
router Pods running on control plane machines will not be reachable by the
ingress load balancer.
. Modify the `manifests/cluster-scheduler-02-config.yml` Kubernetes manifest file to prevent Pods from being scheduled on the control plane machines:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jianlinliu, in the original GCP PR, this was marked as an optional step. Will you please double-confirm that we always need to change this setting on GCP?

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@jianlinliu jianlinliu Oct 16, 2019

Choose a reason for hiding this comment

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

Per my test result, I do not think it is a optional step when we are guiding user set worker replica to 0. Now I think everywhere keep consistent now, it is a MUST (from comments in bugzilla).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adellape, FYI ^

@kalexand-rh
Copy link
Contributor Author

Jianlin approved this change on the bug, so I'm going to merge. Thanks!

@kalexand-rh kalexand-rh merged commit 26bda16 into openshift:master Oct 16, 2019
@kalexand-rh
Copy link
Contributor Author

/cherrypick enterprise-4.2

@kalexand-rh kalexand-rh deleted the BZ1738456 branch October 16, 2019 12:22
@kalexand-rh
Copy link
Contributor Author

/cherrypick enterprise-4.3

@openshift-cherrypick-robot

@kalexand-rh: new pull request created: #17364

Details

In response to this:

/cherrypick enterprise-4.2

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.

@openshift-cherrypick-robot

@kalexand-rh: new pull request created: #17365

Details

In response to this:

/cherrypick enterprise-4.3

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.2 branch/enterprise-4.3 peer-review-done Signifies that the peer review team has reviewed this PR 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.

8 participants