Clean Up Objects in App Mesh #12
Conversation
|
WIP, will push the rest tomorrow morning. |
nckturner
requested review from
stefanprodan,
mogren,
christopherhein and
jqmichael
| return | ||
| } else { | ||
| for _, obj := range objects { | ||
| vnode, ok := obj.(*appmeshv1alpha1.VirtualNode) |
There was a problem hiding this comment.
Ah, this just filters out the VirtualNodes from the list of objects. What can be returned by c.virtualNodeIndex.ByIndex("meshName", name) besides VirtualNodes?
There was a problem hiding this comment.
Probably nothing for now, but its a pattern I saw in upstream k8s. As you pointed out in offline discussion today, potentially if we bump the version we would need to filter out objects with previous versions.
pkg/controller/mesh.go
Outdated
|
|
||
| // This is not a delete, add the deletion finalizer if it doesn't exist | ||
| if yes, _ := containsFinalizer(mesh, meshDeletionFinalizerName); !yes { | ||
| _ = addFinalizer(mesh, meshDeletionFinalizerName) |
There was a problem hiding this comment.
I didn't think there would ever be a case that the object would have no meta, but I guess we should at least log since it shouldn't ever happen.
pkg/controller/virtualservice.go
Outdated
| return c.updateVServiceCondition(vservice, appmeshv1alpha1.VirtualServiceInactive, status) | ||
| } | ||
|
|
||
| func (c *Controller) updateVServiceDeleted(vservice *appmeshv1alpha1.VirtualService, status api.ConditionStatus) error { |
There was a problem hiding this comment.
Worth making a helper for updateVServiceMarkForDeletion() as well?
func (c *Controller) updateVServiceMarkForDeletion(vservice *appmeshv1alpha1.VirtualService) error {
return c.updateVServiceCondition(vservice, appmeshv1alpha1.VirtualServiceMeshMarkedForDeletion, api.ConditionTrue);
}
There was a problem hiding this comment.
Only two questions, see below for the first and the second why are the Spec and Status for each type pointers to the accompanying structs in
aws-app-mesh-controller-for-k8s/pkg/apis/appmesh/v1alpha1/types.go
Lines 21 to 24 in 604a36e
| @@ -51,11 +51,13 @@ spec: | |||
| type: string | |||
| enum: | |||
| - Active | |||
| - Inactive | |||
| - Deleted | |||
| status: | |||
| type: string | |||
There was a problem hiding this comment.
Should we be using a boolean, and converting for our backend?
There was a problem hiding this comment.
We could definitely start with just Active
There was a problem hiding this comment.
for status? I was confused by status being a string with "True" and "False" I can see people thinking it's an actual bool and trying true or false w/o quotes.
There was a problem hiding this comment.
@christopherhein I intended to use pointers only for optional values, as the API guidelines recommend. status is optional because a custom resource can be created without it by a user. spec could probably not be a pointer.
There was a problem hiding this comment.
My above comment ^ doesn't make sense in the context. I was trying to respond to your overall review comment.
for status? I was confused by status being a string with "True" and "False" I can see people thinking it's an actual bool and trying true or false w/o quotes.
You're talking about the allowed values of api.ConditionStatus? Its not something people should set, rather its to be used by the controller to convey information to the user. Also its an upstream dependency so I can't change it without re-defining it.
There was a problem hiding this comment.
I've removed Inactive and Deleted as conditions for now, and we just have Active, which can be True, False or Unknown. This is a Kubernetes convention, currently we are only setting the values to Active=True once the resources are created. We can make it smarter in the future.
| serviceDiscoveryType: | ||
| type: string | ||
| enum: | ||
| - cloudmap-http | ||
| - dns |
There was a problem hiding this comment.
We are starting with just DNS support. At this point this field doesn't do anything, but we will need it later in order to enable Cloud Map http service discovery by creating a Cloud Map namespace.
* When custom resource is deleted, clean up corresponding objects in App Mesh. * Uses a finalizer to implement a delete hook. * When a mesh is deleted, all objects in App Mesh in the mesh that have corresponding custom resources are deleted. * Remove pointers from spec and status fields of all custom resources * Convert App Mesh statuses to a single Active status that is true, false, or Unknown.
|
Updated and addressed most of Claes's and Chris's comments, going to merge and create issues for the important stuff thats left on the checklist. |
Issue #, if available:
#10, #11
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.