address review comments

Author: arshadd-b

docs/proposal/controller-and-user-tags.md

@@ -3,15 +3,19 @@
 
 ## Motivation
 PowerVS cluster creation supports both creating infrastructure and using existing resources required for cluster creation.
-PowerVS cluster reconciler sets controllercreated field whenever resource is created by controller, which was initially introduced to allow proper cleanup of newly created resource vs the use of existing resources.
+PowerVS cluster reconciler sets [controllerCreated](https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/blob/48aebb99c3cd8ce65b95dcfceee8f52daf3d5a31/api/v1beta2/ibmpowervscluster_types.go#L181) field whenever resource is created by controller, which was initially introduced to allow proper cleanup of newly created resource vs the use of existing resources. ControllerCreated field will be set for respective resource under ibmpowervscluster.status
 
 Though its working as expected and fulfills the purpose, we see some drawbacks.
-1. The field is initially set to true during the first reconciliation cycle when the resource is being created. In subsequent reconciliation loops, the field is not updated because the resource already exists in the cloud. This behavior introduces non-idempotency in the controller logic. As a result, if the initial reconciliation event is missed, the controller exhibits inconsistent behavior. Its against k8s principle of reconcilation of having level trigger rather than edge triggered.
-2. The status is expected to be created from spec, considering the scenario of backup and recover. If we move the spec to fresh management cluster which is setting the status, the controller created will be set as false as the resource already exists in cloud but it was created during its previous concilation.
+1. The field is initially set to true during the first reconciliation cycle when the resource is being created. In subsequent reconciliation loops, the field is not updated because the resource already exists in the cloud(created during first reconciliation). This behavior introduces non-idempotency in the controller logic. As a result, if the initial reconciliation event is missed, the controller exhibits inconsistent behavior. Its against Kubernetes principle of reconciliation of having level trigger rather than edge triggered.
+2. The Status subresource in a resource object is expected to be created from spec, considering the scenario of backup and recover. If we move the spec to fresh management cluster which is setting the status, the controller created will be set as false as the resource already exists in cloud but it was created during its previous reconciliation.
 
 ## Goal
-1. This proposal aims to tag the PowerVS cluster's cloud resources and delete the resources created by controller based on tag.
-2. Provide user ability to set custom tags to cloud resources.
+1. Tag newly created PowerVS Cluster's cloud resources and delete the resources based on tag.
+2. Provide ability to set custom tags to cloud resources.
+
+## Non-Goals
+1. Deprecate and remove controllerCreated flag.
+2. Tag user provided resources.
 
 ## Proposal
 This proposal presents adding two kinds of tags to the resources created by controller
@@ -25,7 +29,7 @@ A tag of format`powervs.cluster.x-k8s.io-resource-owner:<cluster_name>` will be
 
 #### Following resources will be getting tagged 
 1. [PowerVS workspace](https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-creating-power-virtual-server)
-2. [PowerVS Network](https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-configuring-subnet) [DHCP service]
+2. [PowerVS Network](https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-configuring-subnet) [DHCP server]
 3. [VPC](https://cloud.ibm.com/docs/vpc?topic=vpc-about-vpc)
 4. [VPC Subnet](https://cloud.ibm.com/docs/vpc?topic=vpc-about-networking-for-vpc)
 5. [VPC Security Groups](https://cloud.ibm.com/docs/vpc?topic=vpc-security-in-your-vpc)
@@ -34,8 +38,8 @@ A tag of format`powervs.cluster.x-k8s.io-resource-owner:<cluster_name>` will be
 8. [COS Instance](https://www.ibm.com/products/cloud-object-storage)
 
 #### Note 
-- Currently transit gateway connections and DHCP server don't support tagging. We will handle their deletion using the VPC and network tag respectively.
-
+- When TransitGateway is tagged we can delete connections. But there is case when TransitGateway is not newly created but connections are newly created. But we connot delete connections since it doesn't support tagging. So to delete VPC connection have to check VPC subnet is tagged and to delete PowerVS connection have to check DHCP network is tagged.
+- To handle deletion DHCP server, have to tag DHCP Network. DHCP server doesn't support tagging.
 
 ### User tags
 User can add tags to resources when creating PowerVS cluster.
@@ -48,14 +52,20 @@ UserTags field will contain list of tags that will be attached to resources.
 // IBMPowerVSClusterSpec defines the desired state of IBMPowerVSCluster.
 type IBMPowerVSClusterSpec struct {
 
-	// UserTags contains list of tags needs to be attached to resources
-	UserTags []string `json:"tags,omitempty"`
+	// Tags contains list of tags needs to be attached to resources
+	Tags []Tag `json:"tags,omitempty"`
 	.
 	.
 	.	
 
 }
 
+// Tag defines single tag in pair
+type Tag struct {
+	Key string  `json:"key,omitempty"`
+	Value string `json:"value,omitempty"`
+}
+
 ```