Skip to content

Conversation

@GroceryBoyJr
Copy link
Contributor

@GroceryBoyJr GroceryBoyJr commented Jan 11, 2023

Status: In Development

Problem: This PR adds a new assembly documenting AWS Local Zone installation Account Information
Built using the assembly installing-aws-user-infra.adoc

Version(s): 4.12

Issue:
AWS Local Zones - Phase 0 IPI - Document options to create compute nodes on Day-0
https://issues.redhat.com/browse/OSDOCS-4347

Link to docs preview:

https://54535--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_aws/installing-aws-localzone.html

Additional information:
Marcos Entenza Garcia PM. QE: Jianli Wei, SME: Marco Braga

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 11, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Jan 11, 2023

🤖 Updated build preview is available at:
https://54535--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/7076

@GroceryBoyJr GroceryBoyJr changed the title PR#2 installing-aws-localzone-vpc created from cloudformation PR#2 Account Information: installing-aws-localzone created from cloudformation Jan 11, 2023
@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 12, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

This is supposed to be the public subnets, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, you must include the private subnet here too. Take a look in my suggestion below.

@kalexand-rh kalexand-rh changed the title PR#2 Account Information: installing-aws-localzone created from cloudformation OSDOCS-4347: Account Information: installing-aws-localzone created from cloudformation Jan 12, 2023
@kalexand-rh
Copy link
Contributor

kalexand-rh commented Jan 12, 2023

@mtulio, will you PTAL?

@yunjiang29, will you please review this late change? Marco said that you provided the review for his article about this installation method, and we'd like to have the downstream version available at GA if at all possible.

  • We've made a copy of the existing AWS UPI installation method and put in Marco's new VPC method and added his steps and CF template to create the subnets.
  • It looks like the user needs to create the VPC and subnet before they make the install-config.yaml file, so those steps are much earlier in the process than the creation of the other stacks.
  • I am not sure what to do about the manifest files for the workers. The existing UPI method has the user removing all the manifests for the control plane and compute nodes and creating the all of the nodes as separate stacks in AWS. However, Marco has a new manifest file for the workers. Should we remove the existing worker manifests and just use Marcos? Do we remove the instruction to create separate worker stacks?
  • It's easiest to see the changes that are specific to the new method if you look at the last commit.

Copy link
Contributor

@mtulio mtulio left a comment

Choose a reason for hiding this comment

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

@GroceryBoyJr @kalexand-rh :
Thanks for providing the documentation. This is my initial review. Overall looks good, I am focusing on removing the sections that we don't need for this guide. I will take a second look tomorrow focusing on the Local Zone details.

Addressing your questions:

  • I am not sure what to do about the manifest files for the workers. The existing UPI method has the user removing all the manifests for the control plane and compute nodes and creating the all of the nodes as separate stacks in AWS. However, Marco has a new manifest file for the workers. Should we remove the existing worker manifests and just use Marcos? Do we remove the instruction to create separate worker stacks?

I answered in detail in the specific line. We must remove the part that we are removing the machineset manifests. The installer must place those manifests to MAPI to create it. (default IPI)

Comment on lines 23 to 26
ClusterInfraId:
Type: String
Default: "unmanaged"
Description: ClusterInfraId used to tag required resources
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to review the unmanaged initial value for ClusterInfraId. The InfraID is a chicken-and-egg problem where we need the VPC created to place the subnets in install-config to get the generated InfraId. In the original article, I place an option to patch this. Let's keep this thread open until I review if it is a must or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let us know the best approach!

Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation of "Requirements for using your VPC" when installing a cluster in existing VPC, says The VPC must not use the kubernetes.io/cluster/.*: owned tag.. This is in consensus what we are doing here. You can skip this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

ClusterInfraId is not required while creating VPC like what original VPC cloudformation template does, tag kubernetes.io/cluster/<infra-id>: shared will be added by installer automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtulio @yunjiang29 Does this mean I should delete the four lines of "ClusterInfraId:" or leave them alone?

Copy link
Contributor

Choose a reason for hiding this comment

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

@GroceryBoyJr you can remove this parameter, please read my comment below on the default value that should be defined when removing this.

https://github.com/openshift/openshift-docs/pull/54535/files#r1070098476

cc @yunjiang29

Comment on lines +372 to +371
PublicRouteTableId:
Description: Public Route table ID
Value: !Ref PublicRouteTable
Copy link
Contributor

Choose a reason for hiding this comment

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

Those lines are the difference from the original VPC cloudformation template.

@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 12, 2023
@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 12, 2023
Comment on lines 134 to 139
Copy link
Contributor

@mtulio mtulio Jan 12, 2023

Choose a reason for hiding this comment

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

This is one of the most important pieces of documentation, this will be the unique part when installing in existing VPC, and there should have the subnetIDs for CloudFormation for VPC. So I would improve this example to keep closer to the real values.
Let me take some suggestions of what could be, feel free to adapt:

Suggested change
subnets: <1>
- subnet-1
- subnet-2
- subnet-3
----
<1> Add the `subnets` section and specify the `PublicSubnetIds` from the output of the CloudFormation template for the VPC.
subnets: <1>
- publicSubnetId-1
- publicSubnetId-2
- publicSubnetId-3
- privateSubnetId-1
- privateSubnetId-2
- privateSubnetId-3
----
<1> Add the `subnets` section and specify the `PrivateSubnetIds` and `PublicSubnetIds` from the outputs of the CloudFormation template for the VPC. You should not include the Local Zone subnets here.

Comment on lines 204 to 205
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Create the machineset for the nodes in your local zone.
.. Export a variable to define the instance type for your control plane machines by running the following command:
. Create the MachineSet manifests for the worker nodes in your Local Zone.
.. Export a variable to define the instance type for the worker machine to deploy on the Local Zone subnet by running the following command:

Copy link
Contributor

Choose a reason for hiding this comment

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

I've updated the step text but retained the styling of "machine set" to match the repo guidelines on API objects.

Copy link
Contributor

@mjpytlak mjpytlak left a comment

Choose a reason for hiding this comment

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

@kalexand-rh Just a handful of minor fixes. Well done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* You read the link:https://aws.amazon.com/about-aws/global-infrastructure/localzones/features/[Features] for each AWS Local Zones location
* You read the link:https://aws.amazon.com/about-aws/global-infrastructure/localzones/features/[Features] for each AWS Local Zones location.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a now thing, but let's be sure to coordinate with QE on reference a managed list, similar to that of line 37.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@kalexand-rh thanks for putting the hardcode here. According previous discussion, I tested c5d.2xlarge and t3.xlarge and get passed.

Now we have two options:

  1. Add c5d.2xlarge and t3.xlarge to the hardcode. (note, m5.2xlarge is NOT applicable )
  2. After auto-include list on installer side added, then update this doc to include them.

Both are acceptable for me.

cc @mtulio

Copy link
Contributor

@mtulio mtulio Jan 16, 2023

Choose a reason for hiding this comment

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

Hi @yunjiang29 , adding only c5d.2xlarge and t3.xlarge, we will exclude:

  • all Local Zones available on the regions eu-*, ap-*, me-*, sa-*[us-east-1-{bue,scl}-*], all outside US
  • will allow only "t3*" instances on the Los Angeles zones (the first announced Local Zone location with the newest resources, and currently the unique including two zones). t3 is also not recommended for production workloads

If we could add/test m5.2xlarge or c5.2xlarge we could cover all of those Local Zones in the mentioned regions, increasing the coverage to +14 zones in/outside US.

I just exported a new matrix of EC2 offering across those regions/zones: https://gist.github.com/mtulio/c98aa15128a7becb06a372f00d824c42

Copy link
Contributor

Choose a reason for hiding this comment

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

@mtulio m5.2xlarge and c5.2xlarge can be added to the list, they have been tested and get passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a statement that typically appears in UPI doc. Is the use of installer-provisioned infrastructure intentional?

Copy link
Contributor

@kalexand-rh kalexand-rh Jan 13, 2023

Choose a reason for hiding this comment

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

Yes. This is IPI not UPI. However, it does use some CF templates to make the VPC and subnets. Unlike the UPI methods, the installation program still stands up all the machines. The VPC and subnet data is added to a custom Kubernetes manifest for the workers to get it done. I think the note still holds.

Copy link
Contributor

Choose a reason for hiding this comment

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

General comment for this PR, as I see this approach elsewhere. We typically reference GitHub source for CloudFormation templates [1]. Is an update a day-2 item?

[1] Example: https://raw.githubusercontent.com/openshift/installer/release-4.12/upi/aws/cloudformation/01_vpc.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. @mtulio, do you have a separate task to track getting these added to the /installer repo? I can add a subtask to the docs follow-ups story, if you'd like.

Copy link
Contributor

@mtulio mtulio Jan 13, 2023

Choose a reason for hiding this comment

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

@katherinedube I opened a PR last year to minimal changes on the existing CloudFormation templates on installer, but it wasn't reviewed.

openshift/installer#6088

The Phase-1 of Local Zones development should be required and has installer changes. I will add it to the same PR as it is part of the same solution:

https://github.com/openshift/installer/pull/6371/files#diff-f2506b1921b9d250fcbb846a9c79caca23fab3ef60f245085d1389639e149c75

As the PR of the installer for Phase-1 is taking a long to review, feel free to share the needed to create a separate PR for that. (as we are delivering this now, I think it could be a good idea)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mjpytlak, do you have a suggestion for who we should tag on the installer team about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend the break?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I copied so many files for this and did not pay attention to whether or not the editor or the file was doing the breaking. (I mean, I can grant myself an exception, if you'd like. ;) )

Copy link
Contributor

Choose a reason for hiding this comment

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

Line break.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Confirm that the template components exist:
. Confirm that the template components exist by running the following command:

Copy link
Contributor

Choose a reason for hiding this comment

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

I shall fix it, but I blame this. We codified that style guidance after, and I missed it in the copied file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Launch the CloudFormation template to create a stack of AWS resources that represent the VPC:
. Launch the CloudFormation template to create a stack of AWS resources that represent the VPC by running the following command:

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Confirm that the template components exist:
. Confirm that the template components exist by running the following command:

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Change to the directory that contains the {product-title} installation program and generate the Kubernetes manifests for the cluster:
. Change to the directory that contains the {product-title} installation program and generate the Kubernetes manifests for the cluster by running the following command:

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The steps for performing an installer-provisioned infrastructure installation are provided as an example only. Installing a cluster with VPC you provide requires knowledge of the cloud provider and the installation process of {product-title}. The CloudFormation templates are provided to assist in completing these steps or to help model your own. You are also free to create the required resources through other methods; the templates are just an example.
The steps for performing an installer-provisioned infrastructure installation are provided as an example only. Installing a cluster with a VPC you provide requires knowledge of the cloud provider and the installation process of {product-title}. The CloudFormation templates are provided to assist in completing these steps or to help model your own. You are also free to create the required resources through other methods; the templates are just an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

This link redirects to https://docs.aws.amazon.com/cli/latest/userguide/cli-chap-welcome.html and best I can tell, this page does not have steps to install the AWS CLI.

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 going to use https://docs.aws.amazon.com/cli/latest/userguide/getting-started-install.html. This link is used in five other places in the repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`${REGION}-wl1-LID-wlz-[1-9]`:: Available Wavelength zones
`${REGION}-wl1-LID-wlz-[1-9]`:: Available Wavelength zones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<3> The VPC created resources will belong to.
<3> The VPC ID that the subnet will be created.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mtulio, do you mean something like "The VPC ID in which the cluster's subnet will be created?"

Copy link
Contributor

Choose a reason for hiding this comment

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

@kalexand-rh the subnet is part(belongs to) of VPC, so I think something like this could be better: "The VPC ID in which the Local Zone's subnet will be created".

Copy link
Contributor

Choose a reason for hiding this comment

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

When removing the unused parameter ClusterInfraId[1], we should set the value of this tag to unmanned.

This should something we need to ship in this version, the in-tree cloud-provider uses those tags to discover the subnets to create the default ingress controller. If the Local Zones subnets is discovered by the ingress operator on the install time, it's expected to the installation to fail. We got this issue on the begging on the research and we are working with upstream to address it on the better way, but the legacy code seems to be untouchable.

Please take a look at [1][2][3].

Suggested change
- Key: !Join [ "", [ "kubernetes.io/cluster/", !Ref ClusterInfraId ] ]
- Key: !Join [ "", [ "kubernetes.io/cluster/unmanaged" ] ]
  ClusterInfraId:
    Type: String
    Default: "unmanaged"
    Description: ClusterInfraId used to tag required resources

[1] tracking the discover bug https://issues.redhat.com/browse/OCPBUGSM-46513
[2] tracking card for ALB Operator requiring to set this tag https://issues.redhat.com/browse/OCPBUGSM-46513
[3] Upstream discussion: https://kubernetes.slack.com/archives/C718BPBQ8/p1672850778127529

Copy link
Contributor

@mtulio mtulio left a comment

Choose a reason for hiding this comment

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

@kalexand-rh @GroceryBoyJr @yunjiang29 LGTM for me. Thanks so much!!! 🎉
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023
@mtulio
Copy link
Contributor

mtulio commented Jan 13, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jan 13, 2023

New changes are detected. LGTM label has been removed.

@kalexand-rh kalexand-rh added the peer-review-done Signifies that the peer review team has reviewed this PR label Jan 13, 2023
@kalexand-rh
Copy link
Contributor

@yunjiang29, because Marco agrees that we have addressed your feedback and has validated updates, I am going to merge this PR so that localization can start work on it. If you have additional feedback, please leave it on this PR, and we will incorporate it immediately.

@kalexand-rh kalexand-rh merged commit cb6bd53 into openshift:main Jan 13, 2023
@kalexand-rh
Copy link
Contributor

/cherrypick enterprise-4.12

@openshift-cherrypick-robot

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

Details

In response to this:

/cherrypick enterprise-4.12

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.

@yunjiang29
Copy link
Contributor

@kalexand-rh one comment about structures, it looks like the current title level/structure are incorrect:

TOC
 |-Internet access for OpenShift Container Platform
   |-Tested instance types for AWS

I think we can update to the similar one as Installing a cluster on AWS into an existing VPC chapter:

  • Move Tested instance types for AWS from Internet access ... to Creating the installation files for AWS
  • Add Minimum resource requirements for cluster installation section, as it's mentioned in NOTE of Tested instance types for AWS.

The final structure would be:

TOC
 |-Internet access for OpenShift Container Platform
 <--SNIP-->
 |-Creating the installation files for AWS
   |-Minimum resource requirements for cluster installation
   |-Tested instance types for AWS
   |-Creating the installation configuration file

cc @mtulio

@mjpytlak
Copy link
Contributor

mjpytlak commented Jan 16, 2023

@mtulio @yunjiang29 I am taking over the final comments for @kalexand-rh and have opened a new PR to address the most recent feedback. [1]

@mtulio You can find the latest update for #54535 (comment) in the new PR.
@yunjiang29 I have updated the structure of the TOC and also updated the tested instance types topic. Let's be sure to circle back with each other when I can update Tested instance types for AWS with a link to a managed file [2]

[1] #54727
[2] #54535 (comment)

Copy link
Contributor

@mtulio mtulio left a comment

Choose a reason for hiding this comment

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

@mjpytlak @kalexand-rh suggested changes for #54727

+
[source,terminal]
----
$ aws cloudformation create-stack --stack-name <name> \ <1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to change to:

$ aws cloudformation create-stack --stack-name <subnet_stack_name> \ <1>

[1] Section: Creating the Kubernetes manifest files
Procedure.2: Create the machine set manifests for the worker nodes in your Local Zone.
Step: Store the subnet ID as a local variable by running the following command:

+
[source,terminal]
----
$ aws cloudformation describe-stacks --stack-name <name>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest change to to keep consistency on[1]:

$ aws cloudformation describe-stacks --stack-name <subnet_stack_name>

[1] Section: Creating the Kubernetes manifest files
Procedure.2: Create the machine set manifests for the worker nodes in your Local Zone.
Step: Store the subnet ID as a local variable by running the following command:

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

Labels

branch/enterprise-4.12 peer-review-done Signifies that the peer review team has reviewed this PR size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants