Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix/fix-differing-determinations-of-template-at-k8s-resource #393

Merged

Conversation

alidaw
Copy link
Contributor

@alidaw alidaw commented Sep 16, 2024

The implementations out for review are used to fix differing determination of tempate at the k8s resource. The determination of the service template the k8s resource shall base on partly is implemented not that well thought out and recurring code fragments are spreaded, which increases risk of failure of logic. This makes reading and finally quick understanding of the logic that is intended to rule the determination of the service template harder and recurring calls of one and the same kind of iterations better should be avoided as well from perspective of performance.
The issue the merge request has been created for rather is in-between what might be placed as a feature and a bug than being a critical bug, but finally - especially at the point where the update of a k8s resource is implemented the way it is implemented bears risk and logic differs from other places where the routines happen as well. Following I'll list what works correct and what needs fix/improvement.

What works correct:

  • The template ID (UUID) as it is represented by the property called service_template_uuid is not intended to get read from what comes as input from the requester/user, but is defined to be computed, which means that it gets derived from either the Kubernetes release (property release) or the GSK version (property gsk_version), both optional input properties.
  • If neither the release nor the GSK version is given, the processing fails with adequate error message.
  • At the implementation of creation of a k8s resource an error gets raised if release and GSK version are given at once for determination of template.
  • At the implementation of creation and update of a k8s resource an error gets raised if given release or given GSK version aren’t valid (aren’t related to a gridscale Kubernetes service template)

What works not that well:

  • There is not a well thought out centric implementation that can be called from anywhere the determination of template via release or GSK version is required. Instead we have recurring code fragments at any place in the k8s resource module where template gets determined via given release or GSK version, among others we end in recurringly running one and the same iterations, which might have not that heavy, but at least an effect on performance.

What works wrong:

  • At the implementation of update of k8s resource the determination of the template via given GSK version always overwrites the determination of the template via release if both are given. This logic won't lead to an error in processing as for both kind of determination the logic ensures that only a valid release or finally a valid GSK version will rule the determination. But it will lead to an error from semantical perspective in case if the given release has nothing to do with the given GSK version and the GSK version the user had addressed accidentally with a typo, but is valid. Then unwelcome GSK version would overwrite welcome release. So finally, this is why the issue rather has been defined to be a bug than a feature request.

Goal:
Come with a centric implementation of the base logic to avoid need to run recurring code fragments, spreaded and partly differing code fragments and fix what went wrong at the implementation used to apply the update of a k8s resource.

@alidaw alidaw self-assigned this Sep 16, 2024
@alidaw alidaw force-pushed the bugfix/fix-differing-determinations-of-template-at-k8s-resource branch from f6c83bf to e365b1a Compare September 16, 2024 23:10
@nvthongswansea
Copy link
Member

@alidaw I think we can improve the performance a bit by calling paasTemplates, err := client.GetPaaSTemplateList(context.Background()) once, and pass paasTemplates to deriveK8sTemplateFromGSKVersion and deriveK8sTemplateFromGSKRelease.

@alidaw
Copy link
Contributor Author

alidaw commented Sep 17, 2024

@alidaw I think we can improve the performance a bit by calling paasTemplates, err := client.GetPaaSTemplateList(context.Background()) once, and pass paasTemplates to deriveK8sTemplateFromGSKVersion and deriveK8sTemplateFromGSKRelease.

No. That doesn't make sense to me. I've adjusted the logic in a way that in case of creation or update anyway simply the function called deriveK8sTemplateFromResourceData would get called once and there also the function where the list of available PaaS templates will get fetched once. I don't see where anything gets called twice anymore. So we do better in remaining with the fetching of the list of available PaaS templates within respective function. The information about available PaaS templates shouldn't be manipulated from outside via paramter at function's call, but should be fetched within respective function as otherwise the task of that function would be alienated as the function is intended to work as source and not to get a source from outside to do iteration on it for derivation of template by given factor like UUID, release of GSDK version.

Copy link
Member

@nvthongswansea nvthongswansea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@alidaw alidaw merged commit 98d617b into master Sep 17, 2024
1 check passed
@alidaw alidaw deleted the bugfix/fix-differing-determinations-of-template-at-k8s-resource branch September 19, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants