Skip to content
This repository has been archived by the owner on Oct 21, 2024. It is now read-only.

fix: update the target service deployment spec after the plugin execution #57

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+26 −28
Open

fix: update the target service deployment spec after the plugin execution #57

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24557ba
fix in apply patch
leoporoli Sep 27, 2024
d2f5901
unit tests update
leoporoli Sep 27, 2024
4bc25fb
dummyspec edited
leoporoli Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions kontrol-service/engine/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,9 @@ func GenerateProdDevCluster(baseClusterTopologyMaybeWithTemplateOverrides *resol
return nil, stacktrace.NewError("Service with UUID %s has no DeploymentSpec", devServiceName)
}

deploymentSpec := flow.DeepCopyDeploymentSpec(devService.DeploymentSpec)

// TODO: find a better way to update deploymentSpec, this assumes there is only container in the pod
deploymentSpec.Template.Spec.Containers[0].Image = item.Image

patches = append(patches, flow_spec.ServicePatch{
Service: devServiceName,
DeploymentSpec: deploymentSpec,
Service: devServiceName,
Image: item.Image,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use the image instead of the spec?

})
}

Expand Down
13 changes: 6 additions & 7 deletions kontrol-service/engine/flow/dev_flow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ import (
"kardinal.kontrol-service/plugins"
"kardinal.kontrol-service/types/cluster_topology/resolved"
"kardinal.kontrol-service/types/flow_spec"

v1 "k8s.io/api/apps/v1"
)

// CreateDevFlow creates a dev flow from the given topologies
Expand Down Expand Up @@ -56,7 +54,7 @@ func CreateDevFlow(
if err != nil {
return nil, err
}
_, err = applyPatch(pluginRunner, topologyRef, clusterGraph, flowID, targetService, servicePatch.DeploymentSpec)
_, err = applyPatch(pluginRunner, topologyRef, clusterGraph, flowID, targetService, servicePatch.Image)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -147,7 +145,7 @@ func applyPatch(
clusterGraph graph.Graph[resolved.ServiceHash, *resolved.Service],
flowID string,
targetService *resolved.Service,
deploymentSpec *v1.DeploymentSpec,
newImage string,
) (*resolved.ClusterTopology, error) {
// Find downstream stateful services
statefulPaths := findAllDownstreamStatefulPaths(targetService, clusterGraph, topology)
Expand All @@ -167,7 +165,7 @@ func applyPatch(

externalPaths := findAllDownstreamExternalPaths(targetService, clusterGraph, topology)
externalServices := make([]*resolved.Service, 0)
alreadyHandledExternalServices := make([]string, 0)

for _, path := range externalPaths {
externalServiceHash, err := lo.Last(path)
if externalServiceHash == "" || err != nil {
Expand Down Expand Up @@ -206,7 +204,8 @@ func applyPatch(
}

modifiedTargetService := DeepCopyService(targetService)
modifiedTargetService.DeploymentSpec = deploymentSpec
// TODO: find a better way to update deploymentSpec, this assumes there is only container in the pod
modifiedTargetService.DeploymentSpec.Template.Spec.Containers[0].Image = newImage
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the target service deployment spec could have been updated when executing the external plugins before these lines, for instance, there could be some env vars overwriting that we need to keep in the deployment spec.
The new image is the only value we need to maintain from the flow_patch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should think on a MergeSpec(a Spec, b Spec) Spec function instead. Then we decide how to merge it inside the function (like a always take precedence)

modifiedTargetService.Version = flowID
err := topology.UpdateWithService(modifiedTargetService)
if err != nil {
Expand Down Expand Up @@ -266,7 +265,7 @@ func applyPatch(
}

// if the service is an external service of the target service, it was already handled above
if lo.Contains(externalServices, service) && !lo.Contains(alreadyHandledExternalServices, service.ServiceID) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"alreadyHandledExternalServices" is never filled.
I think the service handled above doesn't fall under this condition because "lo.Contains" is comparing pointers and the service pointer has changed after that modification above

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this still true after the new gateway support? let me check it

if lo.Contains(externalServices, service) {
// assume there's only one parent service for now but eventually we'll likely need to account for multiple parents to external service
parentServices := topology.FindImmediateParents(service)
if len(parentServices) == 0 {
Expand Down
6 changes: 2 additions & 4 deletions kontrol-service/types/flow_spec/resolved.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
package flow_spec

import v1 "k8s.io/api/apps/v1"

type FlowPatch struct {
FlowId string
ServicePatches []ServicePatch
}

type ServicePatch struct {
Service string
DeploymentSpec *v1.DeploymentSpec
Service string
Image string
}
Loading