Skip to content

Conversation

@jeana-redhat
Copy link
Contributor

@jeana-redhat jeana-redhat commented Jun 6, 2022

Version(s):
4.11

Issue:
OSDOCS-3134 for OCPCLOUD-1353

Link to docs preview:
Machine sets that support using an Elastic Fabric Adapter (requires VPN)

Additional information:

@jeana-redhat jeana-redhat added this to the Future Release milestone Jun 6, 2022
@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 6, 2022
@jeana-redhat jeana-redhat changed the title [OSDOCS-3134]: AWS machineset support for EFA WIP [OSDOCS-3134]: AWS machineset support for EFA Jun 6, 2022
@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 Jun 6, 2022
@jeana-redhat jeana-redhat force-pushed the OSDOCS-3134-AWS-EFA-support branch from 4624cc3 to e90e91d Compare June 7, 2022 15:21
@jeana-redhat jeana-redhat force-pushed the OSDOCS-3134-AWS-EFA-support branch 2 times, most recently from be5e744 to 0802f8a Compare June 23, 2022 19:38
@jeana-redhat jeana-redhat force-pushed the OSDOCS-3134-AWS-EFA-support branch from 0802f8a to 52f26c1 Compare June 30, 2022 21:56
@jeana-redhat jeana-redhat changed the title WIP [OSDOCS-3134]: AWS machineset support for EFA [OSDOCS-3134]: AWS machineset support for EFA Jun 30, 2022
@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 Jun 30, 2022
@jeana-redhat jeana-redhat force-pushed the OSDOCS-3134-AWS-EFA-support branch 2 times, most recently from db6ccd5 to 0945d04 Compare July 1, 2022 10:44
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM

@jeana-redhat
Copy link
Contributor Author

@sunzhaohua2 This is ready for verification. I ended up separating out the steps that are supportive of the use case (as opposed to needed for turning the feature on) into a different task, hope that makes sense :) PTAL and LMK if you ave any questions.

@sunzhaohua2
Copy link

@huali9 PTAL, thanks!

@jeana-redhat jeana-redhat force-pushed the OSDOCS-3134-AWS-EFA-support branch from 0945d04 to c384222 Compare July 15, 2022 16:54
@jeana-redhat
Copy link
Contributor Author

Ok! refreshed with all feedback. @huali9, what do you think - is this ready to move along?

@huali9
Copy link

huali9 commented Jul 17, 2022

Thanks @jeana-redhat
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2022
@jeana-redhat jeana-redhat added the peer-review-needed Signifies that the peer review team needs to review this PR label Jul 18, 2022
Copy link
Contributor

@michaelryanpeter michaelryanpeter left a comment

Choose a reason for hiding this comment

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

Some small nits, a little ISG and a typo.

I am waiting for clarification on one more thing, but I wanted to give you a chance to look things over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @sferich888, is this an acceptable time to link to GitHub? AFAIK this is officially where it lives (@JoelSpeed correct me if I'm overlooking a more legit link please)

@jeana-redhat jeana-redhat force-pushed the OSDOCS-3134-AWS-EFA-support branch from c384222 to 314365d Compare July 18, 2022 21:00
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jul 18, 2022

New changes are detected. LGTM label has been removed.


After configuring a machine set to support the use of an AWS Elastic Fabric Adapter (EFA), you must install additional software to run MPI workloads.

For more information about using Kubeflow and the MPI Operator in {product-title} and an example, see link:https://cloud.redhat.com/blog/how-to-use-kubeflow-and-the-mpi-operator-on-openshift[How to use Kubeflow and the MPI Operator on OpenShift].
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using a blog; as the source of 'how to install' and get setup to use EFA capabilities'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we cloning this (from: https://github.com/kubeflow/mpi-operator); vs instilling it from Operator Hub?

@jeana-redhat jeana-redhat added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2022
@jeana-redhat
Copy link
Contributor Author

Threw a hold on the PR while discussion about the feature is ongoing.

@jeana-redhat
Copy link
Contributor Author

Closing this PR because when we do add this, we will want to do it a bit differently.

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

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. peer-review-needed Signifies that the peer review team needs to review this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants