flow improvements #715

kon-angelo · 2024-03-19T12:57:58Z

How to categorize this PR?

/area control-plane
/kind enhancement
/platform gcp

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

[infrastructure] General stability flow reconciliation improvements.

[infrastructure] Fix a bug where the firewall rules were not properly filtered for infrastructure deletion by the flow reconciler

kon-angelo · 2024-03-19T12:58:03Z

/hold

kon-angelo · 2024-03-19T12:58:09Z

/test

testmachinery · 2024-03-19T12:58:13Z

🔥 Oops, something went wrong @kon-angelo
unable to create Testrun

Command test
Runs a testrun that is either specified by command line flags or in the default values.
The specified path is rendered as testrun and the current repository is injected as a default location.


Example: /test [sub-command] [--flags]

Usage:
      --dry-run              Print the rendered testrun
      --set stringArray      sets additional helm values
      --template             run go templating on the configured file before execution
      --testrunPath string   path to the testrun file that should be executed

Instructions for interacting with me using PR comments are available here

kon-angelo · 2024-03-19T13:11:41Z

/test

testmachinery · 2024-03-19T13:11:45Z

Testrun: e2e-gpfp2
Workflow: e2e-gpfp2-wf
Phase: Failed

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| infrastructure-test | infra-flow-test     | Failed    | 12m4s    |
| infrastructure-test | infra-migrate-test  | Failed    | 13m15s   |
| bastion-test        | bastion-test        | Succeeded | 11m12s   |
| infrastructure-test | infrastructure-test | Failed    | 11m35s   |
+---------------------+---------------------+-----------+----------+

gardener-robot · 2024-03-19T13:15:06Z

@kon-angelo Label kind/enhancements does not exist.

kon-angelo · 2024-03-19T13:37:21Z

/test

testmachinery · 2024-03-19T13:37:25Z

Testrun: e2e-tzvmv
Workflow: e2e-tzvmv-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| bastion-test        | bastion-test        | Succeeded | 10m28s   |
| infrastructure-test | infra-migrate-test  | Succeeded | 11m45s   |
| infrastructure-test | infrastructure-test | Succeeded | 10m34s   |
| infrastructure-test | infra-flow-test     | Succeeded | 11m4s    |
+---------------------+---------------------+-----------+----------+

kon-angelo · 2024-03-20T12:17:50Z

/test

testmachinery · 2024-03-20T12:17:54Z

Testrun: e2e-zk2j6
Workflow: e2e-zk2j6-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| bastion-test        | bastion-test        | Succeeded | 10m7s    |
| infrastructure-test | infra-flow-test     | Succeeded | 11m7s    |
| infrastructure-test | infra-migrate-test  | Succeeded | 11m47s   |
| infrastructure-test | infrastructure-test | Succeeded | 10m38s   |
+---------------------+---------------------+-----------+----------+

kon-angelo · 2024-03-20T15:52:34Z

/unhold

pkg/apis/gcp/v1alpha1/types_infrastructure.go

pkg/controller/infrastructure/infraflow/shared/basic_context.go

AndreasBurger · 2024-03-20T14:40:50Z

pkg/controller/infrastructure/infraflow/shared/extras.go

-			delta := int(time.Since(w.start).Seconds())
-			w.log.Info(fmt.Sprintf("%s [%ds]", w.message.Load(), delta), w.keysAndValues...)
+			delta := time.Since(w.start)
+			w.log.Info(fmt.Sprintf("%s [%s]", w.message.Load(), delta.Round(time.Second).String()), w.keysAndValues...)


Suggested change

w.log.Info(fmt.Sprintf("%s [%s]", w.message.Load(), delta.Round(time.Second).String()), w.keysAndValues...)

w.log.Info(fmt.Sprintf("%s [%.2f s]", w.message.Load(), delta.Seconds()), w.keysAndValues...)

Just an Idea, feel free to discard.

pkg/controller/infrastructure/infraflow/shared/whiteboard.go

pkg/controller/infrastructure/infraflow/graph.go

pkg/controller/infrastructure/infraflow/reconciler.go

pkg/controller/infrastructure/actuator.go

pkg/controller/infrastructure/strategyselector.go

pkg/controller/infrastructure/actuator.go

hebelsan · 2024-03-21T07:14:09Z

pkg/controller/infrastructure/flow_reconciler.go

+		return fmt.Errorf("failed to create flow context: %v", err)
+	}
+
+	status, state, err := fctx.Reconcile(ctx)


Can we maybe come up with something more meaningful then status and state.
Some suggestions: privState, currentState, progState, progress...

kon-angelo · 2024-04-02T17:48:17Z

/test

testmachinery · 2024-04-02T17:48:21Z

Testrun: e2e-xp895
Workflow: e2e-xp895-wf
Phase: Failed

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| bastion-test        | bastion-test        | Succeeded | 8m24s    |
| infrastructure-test | infrastructure-test | Failed    | 2m29s    |
| infrastructure-test | infra-flow-test     | Omitted   | 0s       |
| infrastructure-test | infra-migrate-test  | Omitted   | 0s       |
+---------------------+---------------------+-----------+----------+

AndreasBurger

lgtm

AndreasBurger · 2024-04-04T09:17:00Z

pkg/controller/infrastructure/infraflow/ensure.go

+	vpcName := fctx.vpcNameFromConfig()
+
+	routes, err := fctx.computeClient.ListRoutes(ctx, client.RouteListOpts{
+		// Filter: fmt.Sprintf("network~'%s'", vpcName),


Leftover from development? If so rm. If this is here for future reference consider adding a few words as to why

AndreasBurger · 2024-04-04T09:17:53Z

pkg/controller/infrastructure/infraflow/ensure.go

-	for _, name := range names {
-		log.Info(fmt.Sprintf("destroying route[name=%s]", name))
-		err := c.computeClient.DeleteRoute(ctx, name)
+		log.Info(fmt.Sprintf("destroying route[name=%s]", route.Name))


missing whitespace in route[name=…

AndreasBurger · 2024-04-04T09:23:06Z

pkg/controller/infrastructure/infraflow/reconciler.go

+		// Data: map[string]string{
+		//
+		// 	CreatedResourcesExistKey: ptr.Deref(fctx.whiteboard.Get(CreatedResourcesExistKey), ""),
+		// },


Rm, unless you plan to use it in future

AndreasBurger · 2024-04-04T09:26:18Z

pkg/controller/infrastructure/infraflow/shared/whiteboard.go

@@ -271,6 +271,7 @@ func IsValidValue(value string) bool {
 	return value != "" && value != deleted
 }

+// ObjectKeys returns a slice containing the keys of the Whiteboard in ascending order.


That should be in l.287 I think

kon-angelo · 2024-04-05T13:24:24Z

/test

testmachinery · 2024-04-05T13:24:28Z

Testrun: e2e-9cmvf
Workflow: e2e-9cmvf-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| bastion-test        | bastion-test        | Succeeded | 7m56s    |
| infrastructure-test | infrastructure-test | Succeeded | 8m18s    |
| infrastructure-test | infra-flow-test     | Succeeded | 8m32s    |
| infrastructure-test | infra-migrate-test  | Succeeded | 8m39s    |
+---------------------+---------------------+-----------+----------+

AndreasBurger

lgtm

kon-angelo requested review from a team as code owners March 19, 2024 12:57

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 19, 2024

gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2024

kon-angelo force-pushed the update-this branch from 2ee697e to 17ed653 Compare March 19, 2024 13:11

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2024

gardener-robot added needs/review Needs review kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else area/control-plane Control plane related platform/gcp Google cloud platform/infrastructure labels Mar 19, 2024

gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies labels Mar 19, 2024

kon-angelo force-pushed the update-this branch from 17ed653 to 00a87e4 Compare March 19, 2024 13:36

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 19, 2024

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 19, 2024

gardener-robot added the kind/enhancement Enhancement, improvement, extension label Mar 19, 2024

kon-angelo force-pushed the update-this branch from 00a87e4 to 85645b1 Compare March 20, 2024 09:31

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 20, 2024

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 20, 2024

kon-angelo force-pushed the update-this branch from 1cba50f to 579fe78 Compare March 20, 2024 12:08

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 20, 2024

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 20, 2024

gardener-robot removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Mar 20, 2024

kon-angelo requested a review from AndreasBurger March 20, 2024 15:52

AndreasBurger reviewed Mar 20, 2024

View reviewed changes

improvements

bfbcb8c

kon-angelo force-pushed the update-this branch from 579fe78 to bfbcb8c Compare March 20, 2024 16:35

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 20, 2024

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 20, 2024

hebelsan reviewed Mar 21, 2024

View reviewed changes

comments gardener#1

59ed95d

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 2, 2024

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 2, 2024

AndreasBurger previously approved these changes Apr 4, 2024

View reviewed changes

comments#2

a777c41

kon-angelo dismissed AndreasBurger’s stale review via a777c41 April 5, 2024 13:19

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 5, 2024

AndreasBurger approved these changes Apr 9, 2024

View reviewed changes

kon-angelo merged commit 5fa95f4 into gardener:master Apr 9, 2024
10 checks passed

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Apr 9, 2024

kon-angelo deleted the update-this branch May 24, 2024 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flow improvements #715

flow improvements #715

kon-angelo commented Mar 19, 2024 •

edited

Loading

kon-angelo commented Mar 19, 2024

kon-angelo commented Mar 19, 2024

testmachinery bot commented Mar 19, 2024

kon-angelo commented Mar 19, 2024

testmachinery bot commented Mar 19, 2024 •

edited

Loading

gardener-robot commented Mar 19, 2024

kon-angelo commented Mar 19, 2024

testmachinery bot commented Mar 19, 2024 •

edited

Loading

kon-angelo commented Mar 20, 2024

testmachinery bot commented Mar 20, 2024 •

edited

Loading

kon-angelo commented Mar 20, 2024

AndreasBurger Mar 20, 2024

hebelsan Mar 21, 2024

kon-angelo commented Apr 2, 2024

testmachinery bot commented Apr 2, 2024 •

edited

Loading

AndreasBurger left a comment

AndreasBurger Apr 4, 2024

AndreasBurger Apr 4, 2024

AndreasBurger Apr 4, 2024

AndreasBurger Apr 4, 2024

kon-angelo commented Apr 5, 2024

testmachinery bot commented Apr 5, 2024 •

edited

Loading

AndreasBurger left a comment

	w.log.Info(fmt.Sprintf("%s [%s]", w.message.Load(), delta.Round(time.Second).String()), w.keysAndValues...)
	w.log.Info(fmt.Sprintf("%s [%.2f s]", w.message.Load(), delta.Seconds()), w.keysAndValues...)

flow improvements #715

flow improvements #715

Conversation

kon-angelo commented Mar 19, 2024 • edited Loading

kon-angelo commented Mar 19, 2024

kon-angelo commented Mar 19, 2024

testmachinery bot commented Mar 19, 2024

kon-angelo commented Mar 19, 2024

testmachinery bot commented Mar 19, 2024 • edited Loading

gardener-robot commented Mar 19, 2024

kon-angelo commented Mar 19, 2024

testmachinery bot commented Mar 19, 2024 • edited Loading

kon-angelo commented Mar 20, 2024

testmachinery bot commented Mar 20, 2024 • edited Loading

kon-angelo commented Mar 20, 2024

AndreasBurger Mar 20, 2024

Choose a reason for hiding this comment

hebelsan Mar 21, 2024

Choose a reason for hiding this comment

kon-angelo commented Apr 2, 2024

testmachinery bot commented Apr 2, 2024 • edited Loading

AndreasBurger left a comment

Choose a reason for hiding this comment

AndreasBurger Apr 4, 2024

Choose a reason for hiding this comment

AndreasBurger Apr 4, 2024

Choose a reason for hiding this comment

AndreasBurger Apr 4, 2024

Choose a reason for hiding this comment

AndreasBurger Apr 4, 2024

Choose a reason for hiding this comment

kon-angelo commented Apr 5, 2024

testmachinery bot commented Apr 5, 2024 • edited Loading

AndreasBurger left a comment

Choose a reason for hiding this comment

kon-angelo commented Mar 19, 2024 •

edited

Loading

testmachinery bot commented Mar 19, 2024 •

edited

Loading

testmachinery bot commented Mar 19, 2024 •

edited

Loading

testmachinery bot commented Mar 20, 2024 •

edited

Loading

testmachinery bot commented Apr 2, 2024 •

edited

Loading

testmachinery bot commented Apr 5, 2024 •

edited

Loading