LOG-4612: Service/secret is not deleted when the inputs.receiver.http is removed #2216
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@Clee2691: This pull request references LOG-4612 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.8.0" version, but no target version was set. In response to this:
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. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest |
internal/k8shandler/collection.go
Outdated
| @@ -93,6 +95,18 @@ func (clusterRequest *ClusterLoggingRequest) CreateOrUpdateCollection() (err err | |||
| return err | |||
| } | |||
|
|
|||
| var httpInputs []string | |||
There was a problem hiding this comment.
I would move this newly added code into ReconcileInputServices(), since what it is doing is reconciling input services and as it is, it breaks the pattern in this function of a sequence of Reconcile.
There was a problem hiding this comment.
This wasn't new code, I just moved it around as it was later in this function. However, I have now moved this to where I think the appropriate location should be.
internal/k8shandler/collection.go
Outdated
| if input.Receiver != nil && input.Receiver.HTTP != nil { | ||
| if err := clusterRequest.RemoveInputService(input.Name); err != nil { | ||
| // Find all http input services by label | ||
| labelSelector, _ := labels.Parse("component=http-input-service") |
There was a problem hiding this comment.
labelSelector, _ := labels.Parse("component=http-input-service")
&client.ListOptions{LabelSelector: labelSelector}
can be lifted to the package level as they don't depend on anything inside this function.
There was a problem hiding this comment.
Moved this to k8shandler/service.go and made it more generic so it can be reused in the future.
internal/network/service.go
Outdated
| @@ -60,6 +60,8 @@ func ReconcileInputService(er record.EventRecorder, k8sClient client.Client, nam | |||
| constants.AnnotationServingCertSecretName: certSecretName, | |||
| } | |||
|
|
|||
| desired.Labels["component"] = "http-input-service" | |||
There was a problem hiding this comment.
It would be nice to make "component" and "http-input-service" constants and use them here and in labels.Parse().
internal/k8shandler/collection.go
Outdated
| @@ -93,6 +95,18 @@ func (clusterRequest *ClusterLoggingRequest) CreateOrUpdateCollection() (err err | |||
| return err | |||
| } | |||
|
|
|||
| var httpInputs []string | |||
There was a problem hiding this comment.
httpInputs := sets.NewString()
...
httpInputs.Insert(input.Name)
? Make the collection of HTTP inputs a set from the start and avoid copying in RemoveInputServices()?
internal/k8shandler/collection.go
Outdated
| // Find all http input services by label | ||
| labelSelector, _ := labels.Parse("component=http-input-service") | ||
| httpServices := corev1.ServiceList{} | ||
| if err := clusterRequest.Client.List(context.TODO(), &httpServices, &client.ListOptions{LabelSelector: labelSelector}); err != nil { |
There was a problem hiding this comment.
Not sure if it's possible, but can we include the owner reference into the query so we only get services owned by currOwner and with "component=http-input-service" label?
There was a problem hiding this comment.
Unfortunately it is not possible to query on OwnerRefs
internal/k8shandler/service.go
Outdated
| @@ -40,3 +43,13 @@ func (clusterRequest *ClusterLoggingRequest) RemoveInputService(serviceName stri | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // GetServiceList returns a list of services based on a key/value label | |||
| func (clusterRequest *ClusterLoggingRequest) GetServiceList(key string, val string) (*core.ServiceList, error) { | |||
There was a problem hiding this comment.
Since we're only going to be looking at the services owned by a specific CLF, would it make sense to query here only for services in the same namespace as the CLF?
There was a problem hiding this comment.
Yes that is a good idea so as to reduce the number of services listed. I will make the change.
|
/lgtm |
|
holding for 5.8 z-stream |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Clee2691, jcantrill The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@Clee2691: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/cherry-pick release-5.8 |
|
@Clee2691: new pull request created: #2273 In response to this:
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. |
Description
This PR addresses removing stale
HTTPinput services when theClusterLogForwarder(CLF) is modified.Originally when the
inputs.receiver.httpwas removed from the CLF and updated, NOT deleted and recreated, any previously definedHTTPinput services were not garbage collected.The approach was to label all
HTTPinput services withhttp-input-serviceand upon CLF reconciliation, check if the service is defined in the CLF as well as if the reconciling CLF owns the specific service. If the service is not defined, remove it./cc @syedriko @vparfonov
/assign @jcantrill
Links