sveltosctl icon indicating copy to clipboard operation
sveltosctl copied to clipboard

extend log command to set severity for components running in managed clusters

Open gianlucam76 opened this issue 3 years ago • 6 comments

Is your feature request related to a problem? Please describe. Currently, using sveltosctl, it is possible to change log severity for components running in the management cluster. There is though no support to change log severity for components running in managed clusters.

Describe the solution you'd like Extend sveltosctl log-settings command to set log severity for components running in managed cluster. Cluster namespace/name must be requested to user.

Describe alternatives you've considered Alternative today is to use kubectl and post DebuggingConfiguration default instance in managed clusters.

gianlucam76 avatar Jan 18 '23 23:01 gianlucam76

Hi @gianlucam76,

This is somewhat of a grey area for me, however, I was wondering if it's possible to create a Custom Resource Definition (CRD) that allows users to specify the log severity for components in managed clusters? I think you would also need to create some controller & logic controller(s) with it to help handle what gets updated in that CRD.

AdamSadek avatar Sep 02 '23 13:09 AdamSadek

Hi @AdamSadek thanks for your comment.

what you are saying is correct and sveltos has part of it.

For instance there is this CRD where customer can specify:

  1. Component
  2. log severity

And then each Sveltos components uses this method to watch for DebuggingConfiguration instance changes and dynamically adjust the log verbosity accordingly. Logic to watch and react to change is implemented in the method. So all components need to simply invoked it. For instance, sveltos-agent does it here.

Today, sveltosctl can set DebuggingConfiguration default instance in the management cluster and the Sveltos components running in the management cluster adjust their log verbosity.

Sveltos though has also components running in the managed cluster (sveltos-agent and drift-detection-manager). But sveltosctl has no capability to update DebuggingConfiguration in the managed clusters.

The goal of this enhancement is to modify sveltosctl so to configure DebuggingConfiguration also in managed cluster. That means for instance changing set to accept cluster namespace and cluster name as optional args. If specified, instead of setting DebuggingConfiguration default instance in the management cluster, set it in the managed cluster specified.

Let me know what you think. Thank you again

gianlucam76 avatar Sep 02 '23 15:09 gianlucam76

Thank you for providing more context @gianlucam76. This is the approach I'm currently seeing:

  1. Modify the log-level set command to include optional arguments as you mentioned for cluster namespace and cluster name, i.e:
  • sveltosctl log-level set --component=<name> (--info|--debug|--verbose) [--namespace=<namespace>] [--clusterName=<cluster-name>]
  • Also updating the Options
  1. We also need to parse those new args:
namespace := parsedArgs["--namespace"].(string)
clusterName:= parsedArgs["--clusterName"].(string)
  1. At the end where you return the updateDebuggingConfiguration, we will add a conditional statement that will check whether we should configure DebuggingConfiguration in the management cluster or the specified managed cluster. i.e:
if namespace != "" && clusterName != "" {
    return updateDebuggingConfigurationInManaged(ctx, logSeverity, component, namespace, clusterName) // Need to create this func - see (4.)
}
return updateDebuggingConfiguration(ctx, logSeverity, component)
  1. If it's a managed cluster I guess you would need some type of new implementation to be able to update the DebuggingConfiguration resource in those namespace of the managed cluster? - I guess this is the hard part :D
  • I'm guessing for this part that you would need to interact with the Kubernetes/svelto API of the managed cluster using the provided parameters that we added (namespace and clusterName)
  • Most likely you'd need to connect to the server of managed cluster itself (might need some credential set up as well, possibly with kubeconfig🤔)
  • Create a k8's client to communicate with the managed cluster's API server? (not sure if Go has the appropriate libs to do that, I think most likely yes) - it looks like you might've already set all that up?
  • Finally updating the DebuggingConfiguration resource in the specified namespace of the managed cluster, with the client that has been created. Like here
  • i.e
dc := &libsveltosv1alpha1.DebuggingConfiguration{
        ObjectMeta: metav1.ObjectMeta{
            ClusterName:   clusterName,
            Namespace: namespace,
        },
    }

This is a very brief idea of what I had in mind - not really sure if it will work, and I hope I haven't missed anything in my examples. It might also be a bit all over the place, but hopefully useful in some way. Let me know what you think! :)

AdamSadek avatar Sep 02 '23 17:09 AdamSadek

Thank you @AdamSadek, what you suggested looks great.

Few comments.

Since sveltos manages two different type of managed clusters:

  • clusterAPI powered cluster
  • clusters registered with sveltos

we need to ask also clusterType (defined here) on top of cluster namespace/name.

Regarding point 5, libsveltos has this method that allows to get client to talk to API Server in the managed cluster. Where:

  • adminNamespace, adminName are empty in this case, as platform admin is doing this operation (not a tenant admin);
  • c client.Client is the management cluster client (utils.GetAccessInstance().GetClient() returns the client to access management cluster).

Maybe we could change collectLogLevelConfiguration to accept the DebuggingConfiguration (and do same for updateLogLevelConfiguration). Or something similar.

So we can fetch DebuggingConfiguration from either management cluster or managed cluster with the logic you suggested.

Today, collectLogLevelConfiguration method assumes it needs to talk to the management cluster only. So with following code it gets DebuggingConfiguration from management cluster.

	instance := utils.GetAccessInstance()

	dc, err := instance.GetDebuggingConfiguration(ctx)
	if err != nil {
		if apierrors.IsNotFound(err) {
			return make([]*componentConfiguration, 0), nil
		}
		return nil, err
	}

PS: Really good job finding all those info on your own. I could have added more info on the issue (apologies).

gianlucam76 avatar Sep 02 '23 17:09 gianlucam76

Thank you and no worries! @gianlucam76

So in-addition to the cluster namespace and name, we would need to also add the clusterType👍

It's also good to see that there's already a method that deals with the API server communication. 👍

What you suggested sounds great. Making tweaks to the collectLogLevelConfiguration & updateLogLevelConfiguration, to accept the DebuggingConfiguration and then deal with whether it's a management or managed cluster with some logic.

Would you have a chronological procedure in mind on making these changes? - some sort of Acceptance Criteria (AC)?

Also for testing, do you test the changes right after merging into dev ?

AdamSadek avatar Sep 02 '23 19:09 AdamSadek

Hi @AdamSadek regarding testing, unit tests can be added as part of the PR. Set command already has some tests. You can add others based on your change.

Then sveltosctl has "make lint" and "make test". After you make changes you can run those locally to make sure all is good (those are also part of each PR's CI).

If you want to test on a test setup, you can clone this repo. Then run "make quickstart" which will creates:

  1. management cluster with clusterAPI and projectsveltos
  2. a managed cluster ("kubectl get cluster" -A to see it)

at this point you can test your sveltosctl changes by setting "sveltos-agent" component log severity in the managed cluster and verify it is set as expected.

repo in test/fv/workload_kubeconfig you can find kubeconfig to access the managed cluster so you can run "KUBECONFIG=<addon-controller_location>/ test/fv/workload_kubeconfig kubectl get debuggingconfiguration -o yaml" to verify it is set.

Thank you and please do not hesitate if you have any other question. Thank you so much.

gianlucam76 avatar Sep 03 '23 06:09 gianlucam76