feat: remove OdigosConfig CRD #1405
Conversation
rauno56
force-pushed
the
feat/remove-odigosconfig-crd
branch
7 times, most recently
from
a186284
to
f2eaeff
Compare
rauno56
force-pushed
the
feat/remove-odigosconfig-crd
branch
8 times, most recently
from
a23cd00
to
d33d26d
Compare
rauno56
force-pushed
the
feat/remove-odigosconfig-crd
branch
3 times, most recently
from
f3ce9f3
to
252d0ea
Compare
| func GetCurrentConfig(ctx context.Context, client *kube.Client, ns string) (*common.OdigosConfiguration, error) { | ||
| configMap, err := client.CoreV1().ConfigMaps(ns).Get(ctx, consts.OdigosConfigurationName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var odigosConfig common.OdigosConfiguration | ||
| if err := yaml.Unmarshal([]byte(configMap.Data[consts.OdigosConfigurationFileName]), &odigosConfig); err != nil { | ||
| return nil, err | ||
| } | ||
| return &odigosConfig, nil | ||
| } |
There was a problem hiding this comment.
This function is also defined in autoscaler, frontent, instrumentor, odiglet and maybe other places.
WDYT about moving it to k8sutils module use this function in all places?
There was a problem hiding this comment.
Refactored the use cases in controllers. Left the others intact.
|
looks great. In addition to Amir's great comments:
|
|
Although Amir's comments about the permissions are technically correct about being too broad. I'll leave the those the same for now - the current implementation mimics exactly what OdigosConfig had. I will narrow down the permissions in a future PR.
No.
I have a feeling it wasn't there previously because there's just no way that anything works if the services cannot find the config. Added it!
I will remove it in this PR - the expectation is that the install will not fail and that workaround is not required. |
rauno56
force-pushed
the
feat/remove-odigosconfig-crd
branch
from
3bb702c
to
829cf69
Compare
rauno56
force-pushed
the
feat/remove-odigosconfig-crd
branch
from
c3c7e5d
to
ce3c385
Compare
There was a problem hiding this comment.
that's a lot of places to follow and clean up, Thank you @rauno56 for the thorough work
so happy to have this fixed 🎉
Left just one comment about deleting the old resource to keep the cluster clean and consistent with helm. other than this, I think we can merge
| fmt.Println("Odigos upgrade failed - unable to read the current Odigos configuration.") | ||
| os.Exit(1) | ||
| } | ||
| config = odigosConfig.ToCommonConfig() | ||
| } |
There was a problem hiding this comment.
should we also delete the deprecated object from k8s so that we keep only the one we need in the cluster?
| "sigs.k8s.io/yaml" | ||
| ) | ||
|
|
||
| func GetCurrentConfig(ctx context.Context, k8sClient client.Client) (common.OdigosConfiguration, error) { |
There was a problem hiding this comment.
nit: "Config" can mean few things, perhaps we can rename it GetCurrentOdigosConfig so that it communicate what is being returned in the function name
This will remove usage of
odigosconfigurations.odigos.ioCRD and replace it with a plain ConfigMap to remove race conditions when installing named CRD and CR at the same time during the install process. New ConfigMap will look as following:With the exception of removal of
SupportedSDKs(as per @blumamir request to remove it from CM) the contents ofconfig.yamlvalue and theSpecfield are identical and serialize from the same string and parse to the same object.Installs now just install ConfigMap instead of the CR of OdigosConfiguration, Helm upgrades are unchanged, as they used to just overwrite the CR anyway. CLI parses the old config unless there's already a ConfigMap and creates it as a ConfigMap.
The old CRD is left there unused for now to be able to keep on using the API for it(for migration) - can be removed when we feel we are ready to remove the migration path.