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

SPLAT-811 - Documentation updates related to the Provider Review #23

Merged
merged 1 commit into from
Dec 6, 2022
Merged

SPLAT-811 - Documentation updates related to the Provider Review #23

merged 1 commit into from
Dec 6, 2022

Conversation

mtulio
Copy link
Contributor

@mtulio mtulio commented Oct 26, 2022 •
edited

Documentation targeting to update the OPCT guides with inputs from the Provider Review.

Related cards:

Indirect related (mentioned on the documentation):

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022
@openshift-ci
Copy link

openshift-ci bot commented Oct 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtulio

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2022
@mtulio
Copy link
Contributor Author

mtulio commented Oct 26, 2022

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2022
bostrt
bostrt reviewed Oct 26, 2022
View reviewed changes
docs/dev.md Outdated Show resolved Hide resolved
@mtulio mtulio removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2022
bostrt
bostrt reviewed Oct 26, 2022
View reviewed changes
docs/support-guide.md Outdated Show resolved Hide resolved
bostrt
bostrt reviewed Oct 26, 2022
View reviewed changes
docs/support-guide.md Outdated Show resolved Hide resolved
bostrt
bostrt reviewed Oct 26, 2022
View reviewed changes
docs/support-guide.md Outdated Show resolved Hide resolved
bostrt
bostrt reviewed Oct 26, 2022
View reviewed changes
docs/support-guide.md Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2022
mtulio
mtulio commented Oct 26, 2022
View reviewed changes
docs/user.md Show resolved Hide resolved
bostrt
bostrt reviewed Oct 31, 2022
View reviewed changes
docs/support-guide.md Outdated Show resolved Hide resolved
docs/user-installation-review.md Outdated Show resolved Hide resolved
docs/user-installation-review.md Outdated

- Flavor/Size

> TODO: Need to check if we have any information in our documentation. I am considering the current deployments of Alibaba and NLB (auto-scaling)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have platform agnostic documentation on LB here. Are you looking for something specifically about throughput?

https://docs.openshift.com/container-platform/4.11/installing/installing_platform_agnostic/installing-platform-agnostic.html#installation-load-balancing-user-infra_installing-platform-agnostic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't find anything related to "minimum throughput required for the Load Balancer" in the official documentation. It's something interesting as the default used on our review was lower than the utilization in reference cloud (AWS internal NLB), for that reason I created that section.

docs/user-installation-review.md Outdated Show resolved Hide resolved
docs/user-installation-review.md Outdated Show resolved Hide resolved
@mtulio mtulio force-pushed the doc-review-results branch 2 times, most recently from e74498b to 9698ac0 Compare November 1, 2022 13:41
@bostrt bostrt mentioned this pull request Nov 2, 2022
Merged
@mtulio mtulio mentioned this pull request Nov 3, 2022
Merged
@mtulio mtulio removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2022
@mtulio mtulio changed the title wip | Documentation updates related to the Provider Review Documentation updates related to the Provider Review Nov 5, 2022
@mtulio mtulio marked this pull request as ready for review November 5, 2022 02:26
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022
bostrt
bostrt reviewed Nov 8, 2022
View reviewed changes
docs/support-guide.md Outdated Show resolved Hide resolved
bostrt
bostrt reviewed Nov 9, 2022
View reviewed changes
docs/support-guide.md Outdated Show resolved Hide resolved
bostrt
bostrt reviewed Nov 9, 2022
View reviewed changes
docs/support-guide.md Outdated Show resolved Hide resolved
bostrt
bostrt reviewed Nov 10, 2022
View reviewed changes
docs/user-installation-review.md
- [OpenShift Docs: Disk partitioning](https://docs.openshift.com/container-platform/4.11/installing/installing_bare_metal/installing-bare-metal.html#installation-user-infra-machines-advanced_disk_installing-bare-metal)
- [KCS: Mounting separate disk for OpenShift 4 etcd](https://access.redhat.com/solutions/5840061)

### Image Registry <a name="components-imageregistry"></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this information is redundant with the e2e tests. If the image registry does not function properly or you cannot push/build/pull images then the e2e tests will fail. There's no grey area with the image registry like there is with etcd performance so I'm not entirely sure we still need this section. In other words, why have the user manually repeat tests that will be run in the e2e tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point to add this here is to make sure the partner tested it before taking time running, collecting data, investigating, sending it to support.. As this is a helper document, it was created as a reminder.

wdyt?

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 append this:

(...) otherwise, the e2e tests will be reported as failed.

to the line

You should be able to access the registry and make sure you can push and pull images on it, otherwise, the e2e tests will be reported as failed.

@bostrt
Copy link
Contributor

bostrt commented Nov 10, 2022

@mtulio I'm done with my review

@mtulio mtulio changed the title Documentation updates related to the Provider Review SPLAT-811 - Documentation updates related to the Provider Review Nov 11, 2022
@mtulio
Copy link
Contributor Author

mtulio commented Nov 16, 2022

@rvanderp3 ptal too?

@mtulio
Copy link
Contributor Author

mtulio commented Nov 18, 2022

@bostrt PR reviewed. I left open questions unresolved from your last comments. Thanks for reviewing it. ptal?

@faermanj
Copy link
Contributor

Looks good to me :)
@mtulio what do we need to merge?

@bostrt
Copy link
Contributor

bostrt commented Nov 29, 2022

@mtulio what do we need to merge?

I have suggested some changes here mtulio#1 but after that I think it's nearly good-to-go!

@mtulio
docs: add structure of support review process
ce9f6b0 
docs: add structure of support review process

docs: update dev-guide with filters for process cmd

doc: review support and install guides

doc: PR review; review support guide and formatting

doc: creating troubleshooting document and migrating from user guide

doc: review install-review guide

doc: overall review

doc: @rborst PR review

docs/review: update mkdocs and dev ToC after rebase

docs: review - ready for final review

doc/support-guide: review checklist reference

doc/support-guide: PR review for @bostrt

doc/support-guide: add insights cmdline

Dedicated mode now default and baseline results download (#1)

* doc: dedicated mode is now default

* doc/support-guide: steps on downloading baseline results

* Update docs/user.md

Co-authored-by: Marco Braga <braga@mtulio.eng.br>

* Update docs/user.md

Co-authored-by: Marco Braga <braga@mtulio.eng.br>

* docs: remove development env guidance from user guide

* docs: no longer need aws CLI and can reference HTML webpage hosted in S3

* docs: remove requirement for AWS access key

Co-authored-by: Marco Braga <braga@mtulio.eng.br>
@bostrt
Copy link
Contributor

bostrt commented Dec 6, 2022

/unhold
/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 6, 2022
@openshift-merge-robot openshift-merge-robot merged commit 0f1fe98 into redhat-openshift-ecosystem:main Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bostrt bostrt bostrt left review comments

@rvanderp3 rvanderp3 Awaiting requested review from rvanderp3

Assignees

@bostrt bostrt

@rvanderp3 rvanderp3

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mtulio @bostrt @faermanj @openshift-merge-robot @rvanderp3