Setting valid Kyma Resource Name in a new step #937

jaroslaw-pieszka · 2024-07-17T11:26:47Z

Description

Changes proposed in this pull request:

Step calculating Kyma Resource name using step.KymaName method as a single point of truth, the value is used to update the operation
removal of no longer needed CreateKymaForOwnCluster step

Related issue(s)

github-actions · 2024-07-17T11:27:03Z

Add one of following labels

- kind/feature -> Use it when you want to submit a new feature

- kind/enhancement -> Use it when you modify or improve an existing feature

- kind/bug -> Use it when you fix a bug

ralikio · 2024-07-17T12:49:35Z

internal/process/provisioning/create_kyma_name_step.go

+
+	// could be simplified but this provides single source of truth for Kyma name
+	operation.KymaResourceName = ""
+	operation.KymaResourceName = steps.KymaName(operation)


Is that duplicated assignment intentional?

that is what comment is about.

In effect this makes this simple assignment:
operation.KymaResourceName=strings.ToLower(operation.RuntimeID)
But using always steps.KymaName we ensure consistence.

I do not understand why line 40 is required. I guess it is just to force KymaResourceName to be empty so that steps.KymaName for sure sets it to RuntimeId. Is that correct? It seems confusing because we need to get into details of invoked method when analysing what the empty assignment is all about.

I have the same feeling. It seems awkward. I need to reconsider this with @piotrmiskiewicz if this is what we wanted and if we have to pay such a price for this single point of truth.
I would prefer to change steps.KymaName() semantics and avoid emptying the operation.KymaResourceName.
But this function is already used exactly in this way.

I changed it slightly in the next commit.

I tried to eat a cake and have a cake. Please take a look.

ralikio · 2024-07-17T13:06:12Z

cmd/broker/provisioning.go

+		{
+			condition: provisioning.SkipForOwnClusterPlan,
+			stage:     createRuntimeStageName,
+			step:      provisioning.NewCreateKymaNameStep(db.Operations()),


Is this steps going to do anything else? For now it feels like it is just doing one assignment? I do not think that it is necessary to create a new step for such assignment. Maybe we could put that in initialisation step or somewhere else?

This is how we decided to proceed after discussion.

See #905 (comment)

In my opinion this approach is justified, if you have doubts we can arrange a meeting tomorrow.
Some pros:

simple logic,

postcondition guaranteed,

easy to manage in the process of migration from Provisioner to KIM - we will remove some steps gradually

easy to extend

easy to test

Currently we have a lot of quite complex steps, and it is hard to grasp what is set where, could we remove step, could we add a new one, and where to put it, to have all required data i to provide all required data for subsequent steps.

the question is where we should put this assignment. What are hard requirements? Such assignment (logic, which calculates the Kyma resoruce name) must:

be executed after runtime ID gathering/generation (after interaction with Provisioner)

be before Runtime CR creation which must be before ApplyKyma step (currently the logic is in the apply kyma step)

@ralikio - I know this logic is very simple, but we can't put it into initialisation step or something like this (it is too early).
We can't put it into a provisioner interaction step (we gather runtime ID there), because this step will be removed soon or even sometimes the provisioner interaction is not executed (own cluster plan)

ralikio · 2024-07-17T13:06:31Z

internal/process/steps/lifecycle_manager.go

@@ -54,6 +54,10 @@ func KymaName(operation internal.Operation) string {
 	return strings.ToLower(operation.RuntimeID)
 }

+func KymaRuntimeResourceName(operation internal.Operation) string {


Where is this method used?

We decided to create single point of truth for Runtime Resource as well. I will add invocation in next commit.

Initially I wanted to add invocation in separate PR, but it is more consistent if I add it here.

piotrmiskiewicz · 2024-07-18T05:47:57Z

internal/process/provisioning/create_kyma_name_step.go

+		return s.operationManager.OperationFailed(operation, fmt.Sprint("RuntimeID not set, cannot create Kyma name"), nil, log)
+	}
+
+	operation.KymaResourceName = steps.CreateKymaNameFromOperation(operation)


this line should be inside the inline function in the UpdateOperation

in the next commit

piotrmiskiewicz · 2024-07-18T05:48:14Z

internal/process/provisioning/create_kyma_name_step.go

+	operation.KymaResourceName = steps.CreateKymaNameFromOperation(operation)
+
+	return s.operationManager.UpdateOperation(operation, func(op *internal.Operation) {
+		op.RuntimeID = operation.RuntimeID


we do not need to update RuntimeID

In the next commit

piotrmiskiewicz · 2024-07-18T05:49:00Z

internal/process/provisioning/create_kyma_name_step.go

+}
+
+func (s *CreateKymaNameStep) Run(operation internal.Operation, log logrus.FieldLogger) (internal.Operation, time.Duration, error) {
+	if time.Since(operation.UpdatedAt) > CreateRuntimeTimeout {


In my opinion, we do not need to check any timeout

Removed in the next commit.

piotrmiskiewicz · 2024-07-18T05:49:52Z

internal/process/provisioning/create_runtime_resource_step.go

@@ -58,7 +58,8 @@ func (s *CreateRuntimeResourceStep) Run(operation internal.Operation, log logrus
 		return operation, 0, nil
 	}

-	kymaResourceName, kymaResourceNamespace := getKymaNames(operation)
+	kymaResourceName := steps.KymaRuntimeResourceName(operation)


once, we set the KymaName in the previous step, we can just rely on an operation field, like namespace (one line below)

Changed in the next commit.

piotrmiskiewicz · 2024-07-18T10:19:48Z

cmd/broker/provisioning.go

@@ -84,6 +84,13 @@ func NewProvisioningProcessingQueue(ctx context.Context, provisionManager *proce
 			stage:     createRuntimeStageName,
 			step:      provisioning.NewGenerateRuntimeIDStep(db.Operations(), db.RuntimeStates(), db.Instances()),
 		},
+		// postcondition: operation.RuntimeID is set
+		{
+			condition: provisioning.SkipForOwnClusterPlan,


we do not need this condition, it should wokr also for own cluster

In the next commit.

But then tests for OwnCluster fail.
Need to investigate.

Removed no longer needed step.

jaroslaw-pieszka added 4 commits July 17, 2024 08:46

comment as initial step

92934c6

initial version of step with invocation

8bd3393

comments

37963af

tests

c93a42b

jaroslaw-pieszka requested a review from a team as a code owner July 17, 2024 11:26

jaroslaw-pieszka self-assigned this Jul 17, 2024

kyma-bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 17, 2024

jaroslaw-pieszka added kind/enhancement Categorizes issue or PR as related to modifying or improving an existing feature and removed cla: yes Indicates the PR's author has signed the CLA. labels Jul 17, 2024

kyma-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cla: yes Indicates the PR's author has signed the CLA. labels Jul 17, 2024

ralikio reviewed Jul 17, 2024

View reviewed changes

jaroslaw-pieszka added 4 commits July 17, 2024 15:17

go imports

99423be

Use utility function

71e85b0

simplified

508f5b0

naming corrected

5442579

piotrmiskiewicz reviewed Jul 18, 2024

View reviewed changes

jaroslaw-pieszka added 4 commits July 18, 2024 07:54

timeout check removed

77960ee

review remark

4f23965

review remarks

13649a3

go imports

73b33b3

szwedm assigned piotrmiskiewicz Jul 18, 2024

comment added

fa5c691

piotrmiskiewicz reviewed Jul 18, 2024

View reviewed changes

jaroslaw-pieszka added 8 commits July 18, 2024 12:29

condition removed as pointed out in review

541fede

removal of now redundant step

55163d9

fixtures reverted

86042a2

make fix

f097617

comment updated

ded3ac2

no sleep

2f2a8bf

assertion order insensitive

5aa5de7

Step condition removed

4c71e45

piotrmiskiewicz previously approved these changes Jul 18, 2024

View reviewed changes

kyma-bot added the lgtm Looks good to me! label Jul 18, 2024

make fix

09a5318

jaroslaw-pieszka dismissed piotrmiskiewicz’s stale review via 09a5318 July 18, 2024 12:05

kyma-bot removed the lgtm Looks good to me! label Jul 18, 2024

piotrmiskiewicz approved these changes Jul 18, 2024

View reviewed changes

kyma-bot added the lgtm Looks good to me! label Jul 18, 2024

kyma-bot merged commit 9461f9b into kyma-project:main Jul 18, 2024
24 of 28 checks passed

jaroslaw-pieszka deleted the valid-kyma-name-and-namespace branch July 18, 2024 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting valid Kyma Resource Name in a new step #937

Setting valid Kyma Resource Name in a new step #937

jaroslaw-pieszka commented Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 17, 2024

ralikio Jul 17, 2024 •

edited

Loading

jaroslaw-pieszka Jul 17, 2024

jaroslaw-pieszka Jul 17, 2024 •

edited

Loading

ralikio Jul 17, 2024

jaroslaw-pieszka Jul 17, 2024

jaroslaw-pieszka Jul 17, 2024

jaroslaw-pieszka Jul 17, 2024

ralikio Jul 17, 2024

jaroslaw-pieszka Jul 17, 2024 •

edited

Loading

jaroslaw-pieszka Jul 17, 2024

jaroslaw-pieszka Jul 17, 2024

piotrmiskiewicz Jul 17, 2024 •

edited

Loading

ralikio Jul 17, 2024 •

edited

Loading

jaroslaw-pieszka Jul 17, 2024

jaroslaw-pieszka Jul 17, 2024

piotrmiskiewicz Jul 18, 2024

jaroslaw-pieszka Jul 18, 2024

piotrmiskiewicz Jul 18, 2024

jaroslaw-pieszka Jul 18, 2024

piotrmiskiewicz Jul 18, 2024

jaroslaw-pieszka Jul 18, 2024

piotrmiskiewicz Jul 18, 2024 •

edited

Loading

jaroslaw-pieszka Jul 18, 2024

piotrmiskiewicz Jul 18, 2024

jaroslaw-pieszka Jul 18, 2024

jaroslaw-pieszka Jul 18, 2024 •

edited

Loading

jaroslaw-pieszka Jul 18, 2024

Setting valid Kyma Resource Name in a new step #937

Setting valid Kyma Resource Name in a new step #937

Conversation

jaroslaw-pieszka commented Jul 17, 2024 • edited Loading

github-actions bot commented Jul 17, 2024

ralikio Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaroslaw-pieszka Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaroslaw-pieszka Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrmiskiewicz Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

ralikio Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrmiskiewicz Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaroslaw-pieszka Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaroslaw-pieszka commented Jul 17, 2024 •

edited

Loading

ralikio Jul 17, 2024 •

edited

Loading

jaroslaw-pieszka Jul 17, 2024 •

edited

Loading

jaroslaw-pieszka Jul 17, 2024 •

edited

Loading

piotrmiskiewicz Jul 17, 2024 •

edited

Loading

ralikio Jul 17, 2024 •

edited

Loading

piotrmiskiewicz Jul 18, 2024 •

edited

Loading

jaroslaw-pieszka Jul 18, 2024 •

edited

Loading