gistfile1.txt

Edu's review:

The following section, “OpenShift SDN Plugins”
-> Make this a link

ovs-mulit-tenant
-> typo

1: https://docs.openshift.org/latest/admin_guide/managing_networking.html#admin-guide-networking-networkpolicy
-> Shall we use docs.openshift.com instead .org

Using the installation methods described in this document the registry is deployed using a S3 bucket. 
-> Remove AWS references

# az
-> Use regular user if root is not needed
I put comments in so people don't accidently send it if they are copying and pasting


for the deployment of OpenShift
-> use {rhocp}

# az network vnet create \
  --name openshiftvnet \
  --resource-group openshift \
  --subnet-name ocp \
  --address-prefix 10.0.0.0/16 \
  --subnet-prefix 10.0.0.0/24
-> I personally like nesting it like
$ az network vnet create \
    --name openshiftvnet \
    --resource-group openshift \
    --subnet-name ocp \
    --address-prefix 10.0.0.0/16 \
    --subnet-prefix 10.0.0.0/24
(+2 spaces for new lines)

# az network nsg rule create \
  -g openshift \
  --nsg-name bastion-nsg \
  -n bastion-nsg-ssh  \
  --priority 500 \
  --destination-port-ranges 22 \
  --access Allow --protocol Tcp \
  --description "SSH access from Internet"
-> I'd say not mix short and long flags (-g vs --group or whatever)

-> I miss an explanation on nsg priorities

# az network nsg rule create \
  -g openshift \
  --nsg-name master-nsg \
  -n node-kubectl  \
  --priority 650 \
  --source-address-prefixes VirtualNetwork \
  --destination-port-ranges 10250 \
  --access Allow --protocol Tcp \
  --description "kubectl"
--> It's just me being picky but the service is kubelet :)

2.5.3. Infrastructure Node Security Group
-> Duplicated ports? If the infra-nsg should include regular node-sg i'd say to add those to the table
I believe i fixed this. Azure cli wont allow two groups to be assigned to vms

-> 443 is opened twice?
Internal and to lb


2.6.1. Master Load Balancer
I miss some explanation on every step


2.6.2. Router Load Balancer
subs=+quotes]     

# for i in 1 2 3; do az network nic create \
  --resource-group openshift \
  --name ocp-master-${i}VMNic \
  --vnet-name openshiftvnet \
  --subnet ocp \
  --network-security-group master-nsg \
  --lb-name ocpMasterLB \
  --lb-address-pools masterAPIBackend \
  --internal-dns-name ocp-master-${i} \
  --public-ip-address ""; \
  done
-> Make nº instances a variable
-> az command in its own line
$ for i in $(seq 0 $((${MASTER_NODE_COUNT}-1))); do
      az ...
   done
https://gitlab.cee.redhat.com/Documentation-sysdeseng-reference-architectures-documentation/doc-openshift_master_repository/blob/preview_ocp_gcp_39/scripts/06-masters.sh#L3

cooktheryan: webbrowser search for lll (three lower case L's)...  BOOM!

cooktheryan: https://access.qa.redhat.com/documentation/en-us/reference_architectures/2018/html-single/deploying_and_managing_openshift_3.9_on_azure/index#service_catalog
why do we have this section if openshift_enable_service_catalog=false??
No storage class for azure at base install. So install fails with the etcd looking for persitent storage

cooktheryan: There seems to be a lot of '$ cat filename'...
Update to '$ cat << EOF > filename' to enable copy/pasting
        I think i resolved this


Is my installation supported if integrating with other technologies? Yes     
-> IDK what this means

public IPs to OpenShift 
-> {rhocp}

Azure Virtual Networks
-> I'll try to explain this better, it's quite confusing to me

1.1.7. Load Balancer
-> I'll add some stentence to clarify it load balances networking traffic (it is implicit but...)

ovs-mulitenant
-> typo

-> docs.openshift.com/latest doesn't work

1.10. Container-Native Storage (Optional)
-> openshift  vs {rhocp}

32GB of RAM per EC2 instance.     
-> Remove Amazon references

CNS ports
-> I'd say to add a note with the service listening

--name == Red Hat OpenShift Container Platform Instance Prerequisites
-> This section header is broken

The Azure cli can be used to deploy all of the components
-> I'm not sure on "all of the" vs "all the" I'll let you decide as I'm not native speaker :)

NSGs
-> I'd say to use `NSGs`

Duplicated 24224 port in masters table
udp and tcp


NGS naming:
bastion-nsg-ssh vs master-ssh (I prefer the last one)

Security groups source
-> I'm not sure about this but opening the port to the virtualnetwork will allow connectivity from any instance in that network to that service right? Also for load balanced ports, the source tag can be "AzureLoadBalancer" IIRC

I'd say to use "kubelet" in --description "kubectl"

SourceIPProtocol load balancing
-> I'll add a note explaining natted clients will be redirected to the same backend server

Load balancer heatlh checks
-> I'd say to invest some time in check if http probes can be used. Tcp probes are ok, but they just answer the port is working but not if the load balancer is working properly

Search for "rcooksouthcentral"

1.17.2. Master Instance Creation
-> etcd and openshift local disks are missing. local for infra and nodes also missing


Try --output tsv for an easy to view ip:
$ az network public-ip show -g eminguez-temp-vm -n azvm-ip --output tsv --query "{address: ipAddress}"
52.166.35.25

az network dns record-set 
-> Mixed short & long options
-> Wrong tab in bastion record

Bastion config
-> Shall we use ssh-agent instead?

"$3r3tR3gistry" as password I'm not sure if this is a leak

# az ad sp create-for-rbac --name openshiftcloudprovider \
     --password $3r3tR3gistry --role contributor
     --scopes /subscriptions/8227d1d9-c10c-4366-86cc-e3ddbbcbba1d/resourceGroups/openshift

I'm not sure if after contributor a \ + new line is missing or not

1.21.1. OpenShift Authentication
-> Mixing google authentication in azure doesn't seem right to me. Maybe use htpasswd as the example for all providers?
Ugh i just hate htpasswd it seems like such a hack especially since the file doesn't sync between the instances


Missing disabling swap
https://github.com/openshift/openshift-ansible-contrib/blob/master/reference-architecture/azure-ansible/3.7/node.sh#L26-L28
I think the installer does this by default now
Beware in Azure is different as there is no swap in /etc/fstab


there aren't etcd volumes?



Maybe "container_runtime_docker_storage_setup_device" variable can be set in the inventory file

Prerrequisites playbook without "-i"

{provder}

Remove variables if they are using default values in the inventory such as:
osm_cluster_network_cidr=172.16.0.0/16
ansible_ssh_user=cloud-user
ansible_become=true

The inventory as it is now will deploy some goodies by default such as service catalog, etc. and it will fail because there is not a storageclass defined by default. I'd say to add the "deploy_whatever=false" in the inventory and explain it.
Also, see here https://bugzilla.redhat.com/show_bug.cgi?id=1471718#c8 & https://github.com/openshift/openshift-ansible/tree/release-3.9/roles/openshift_default_storage_class as it seems storage class can be created at installation time


The diagram should be explained a bit. Currently it says:
The Red Hat OpenShift Container Platform instances:
...
and nothing else.

Section 2.6 table missing ports 49152-49664


# Do not uninstall service catalog until post installation. Needs storage class object
-> # Do not install service catalog until post installation. Needs storage class object
-> What about creating the storageclass at installation time as https://github.com/openshift/openshift-ansible/tree/release-3.9/roles/openshift_default_storage_class