refactor: reduce usage of k8s.io/kubernetes packages #205
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
| @@ -38,7 +38,6 @@ type OnKubectlRunFunc func(command string) (CleanupFunc, error) | |||
|
|
|||
| type Kubectl interface { | |||
| ApplyResource(ctx context.Context, config *rest.Config, obj *unstructured.Unstructured, namespace string, dryRunStrategy cmdutil.DryRunStrategy, force, validate bool) (string, error) | |||
| ConvertToVersion(obj *unstructured.Unstructured, group, version string) (*unstructured.Unstructured, error) | |||
There was a problem hiding this comment.
vX->vY conversion only works on known objects, does not work on CRDs. Also, no need to do it here since the engine is watching all resources anyway - is that right? Or what are the cases when conversion is needed?
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
+ Coverage 49.21% 49.95% +0.73%
==========================================
Files 40 39 -1
Lines 3251 3007 -244
==========================================
- Hits 1600 1502 -98
+ Misses 1491 1351 -140
+ Partials 160 154 -6
Continue to review full report at Codecov.
|
|
@alexmt =) |
Signed-off-by: Mikhail Mazurskiy <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Sorry for ignoring PR for so long.
I agree with the EndPoint related change, however, I don't think we can remove the conversion code just yet. Client-side conversion allows us to reconcile a lot of resources very quickly and don't store multiple versions of the same resource in memory.
@ash2k I understand the desire to get rid of k8s dependencies.
I think the only way do to it is to switch to server-side diffing. The list of K8S CRDs is growing and client-side diffing will be less and less efficient in the future. So we have to switch sooner or later. This would allow us to get rid of k8s/kubernetes dependency.
It is not the top priority for Argo CD yet. How urgent it is work Gitlab k8s agent? We can consider and workaround: switch engine to server-side diffing as a default but make it possible to override diffing and use client-side diffing in Argo CD only.
| @@ -597,19 +596,6 @@ func (k *KubectlCmd) authReconcile(ctx context.Context, config *rest.Config, kub | |||
| return strings.Join(out, ". "), nil | |||
| } | |||
|
|
|||
| // ConvertToVersion converts an unstructured object into the specified group/version | |||
| func (k *KubectlCmd) ConvertToVersion(obj *unstructured.Unstructured, group string, version string) (*unstructured.Unstructured, error) { | |||
There was a problem hiding this comment.
GitOps engine "watch" preferred versions of the resources to minimize memory usage. So we have to rely on conversion to avoid making an API cal and do client-side only diffing. It is critical for performance when the engine is used to manage multiple clusters.
|
Another note: Client-side diffing is very critical for Argo CD because it has the requirement to manage hundreds of clusters and visualizes real-time diff in the UI. We were planning to "cache" diffing result using probably |
Updates #56.
The only usage left is in the diff:
gitops-engine/pkg/diff/diff.go
Line 117 in 0b4199b
It uses the legacy scheme (via https://github.com/argoproj/gitops-engine/blob/master/pkg/utils/kube/scheme/scheme.go) to perform defaulting, which depends on all the internal k/k schemas. It's fragile (see #56 (comment)) and it does not work for custom resources too. I wonder if there is a better way? Maybe it's possible to use the apply api with dry-run flag (don't know if that's a thing)?
Defaulting using the scheme was introduced in #154. /cc @jgwest
Regardless of the above, I think this PR is mergeable with one last step remaining to fix #56.