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

infrastructure flow reconciler #739

Merged
merged 6 commits into from
Dec 1, 2023

Conversation

kon-angelo
Copy link
Contributor

@kon-angelo kon-angelo commented Oct 23, 2023

How to categorize this PR?

/area control-plane
/kind enhancement
/platform azure

What this PR does / why we need it:

This is a continuation from #596. Special thanks to @elchead for all the effort in the initial implementation 💯

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Add new flow-based infrastructure reconciler.

@kon-angelo kon-angelo requested review from a team as code owners October 23, 2023 12:39
@kon-angelo
Copy link
Contributor Author

/hold

@gardener-robot-ci-2 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 Oct 23, 2023
@gardener-robot gardener-robot added 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 kind/enhancement Enhancement, improvement, extension platform/azure Microsoft Azure platform/infrastructure needs/review Needs review 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 Oct 23, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 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 Oct 23, 2023
@gardener-robot-ci-1 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 Oct 23, 2023
@kon-angelo kon-angelo force-pushed the terraform-remove-new branch from c43b2fb to 3ca4f7e Compare October 23, 2023 13:57
@gardener gardener deleted a comment from testmachinery bot Oct 23, 2023
@gardener-robot-ci-1 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 Oct 23, 2023
@gardener gardener deleted a comment from testmachinery bot Oct 23, 2023
@gardener-robot-ci-1 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 Oct 23, 2023
@testmachinery
Copy link

testmachinery bot commented Oct 24, 2023

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

+---------------------+---------------------------+-----------+----------+
|        NAME         |           STEP            |   PHASE   | DURATION |
+---------------------+---------------------------+-----------+----------+
| infrastructure-test | infrastructure-test       | Failed    | 41m27s   |
| infrastructure-test | infra-flow-test           | Failed    | 32m3s    |
| infrastructure-test | infra-flow-migration-test | Failed    | 43m29s   |
| bastion-test        | bastion-test              | Succeeded | 11m11s   |
+---------------------+---------------------------+-----------+----------+

@gardener gardener deleted a comment from testmachinery bot Oct 24, 2023
@gardener gardener deleted a comment from testmachinery bot Oct 24, 2023
@gardener-robot-ci-2 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 Oct 24, 2023
@testmachinery
Copy link

testmachinery bot commented Oct 24, 2023

Testrun: e2e-6jrp7
Workflow: e2e-6jrp7-wf
Phase: Failed

+---------------------+---------------------------+-----------+----------+
|        NAME         |           STEP            |   PHASE   | DURATION |
+---------------------+---------------------------+-----------+----------+
| bastion-test        | bastion-test              | Succeeded | 10m46s   |
| infrastructure-test | infrastructure-test       | Failed    | 44m2s    |
| infrastructure-test | infra-flow-test           | Failed    | 41m38s   |
| infrastructure-test | infra-flow-migration-test | Failed    | 46m32s   |
+---------------------+---------------------------+-----------+----------+

@gardener-robot-ci-2 gardener-robot-ci-2 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 Oct 24, 2023
@kon-angelo kon-angelo force-pushed the terraform-remove-new branch from bad7a3f to d40f2cb Compare October 24, 2023 10:17
@testmachinery
Copy link

testmachinery bot commented Oct 24, 2023

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

+---------------------+---------------------------+-----------+----------+
|        NAME         |           STEP            |   PHASE   | DURATION |
+---------------------+---------------------------+-----------+----------+
| bastion-test        | bastion-test              | Succeeded | 12m53s   |
| infrastructure-test | infrastructure-test       | Succeeded | 34m35s   |
| infrastructure-test | infra-flow-test           | Succeeded | 27m20s   |
| infrastructure-test | infra-flow-migration-test | Failed    | 35m34s   |
+---------------------+---------------------------+-----------+----------+

@gardener gardener deleted a comment from testmachinery bot Oct 24, 2023
@kon-angelo
Copy link
Contributor Author

/test

@testmachinery
Copy link

testmachinery bot commented Oct 24, 2023

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

+---------------------+---------------------------+-----------+----------+
|        NAME         |           STEP            |   PHASE   | DURATION |
+---------------------+---------------------------+-----------+----------+
| bastion-test        | bastion-test              | Succeeded | 12m22s   |
| infrastructure-test | infra-flow-migration-test | Succeeded | 33m54s   |
| infrastructure-test | infrastructure-test       | Succeeded | 31m58s   |
| infrastructure-test | infra-flow-test           | Succeeded | 27m4s    |
+---------------------+---------------------------+-----------+----------+

Copy link
Member

@MartinWeindel MartinWeindel left a comment

Choose a reason for hiding this comment

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

Should we try to unify whiteboard and inventory as the state of the whiteboard is also persisted for other providers like AWS and Openstack?
Looks to me as if we have resource ids and other data in whiteboard and inventory with sometimes overlapping purposes:

  • managed items (created by the infrastructure controller), persisted for providing a list of managed resources in case for manual cleanup on forced deletion
  • managed or looked up resource ids and names exposed for third-party controllers.
  • managed resource ids/names in state if there is no suitable place to store this information on IaaS side (like tags, etc)
  • managed resource ids/names in state for informative purposes
  • internal caching of resource object details to simplify code

pkg/apis/azure/v1alpha1/types_infrastructure.go Outdated Show resolved Hide resolved
pkg/apis/azure/types_infrastructure.go Outdated Show resolved Hide resolved
@elchead
Copy link
Contributor

elchead commented Oct 30, 2023

thanks for the headsup @kon-angelo. Happy to see it alive! 🚀 Could you co-author me if possible in the merge commit?

@gardener-robot-ci-3 gardener-robot-ci-3 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 Nov 27, 2023
@kon-angelo
Copy link
Contributor Author

/test

@testmachinery
Copy link

testmachinery bot commented Nov 27, 2023

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

+---------------------+---------------------------+-----------+----------+
|        NAME         |           STEP            |   PHASE   | DURATION |
+---------------------+---------------------------+-----------+----------+
| bastion-test        | bastion-test              | Succeeded | 11m23s   |
| infrastructure-test | infrastructure-test       | Succeeded | 33m40s   |
| infrastructure-test | infra-flow-test           | Succeeded | 30m47s   |
| infrastructure-test | infra-flow-migration-test | Succeeded | 35m0s    |
+---------------------+---------------------------+-----------+----------+

@gardener-robot-ci-3 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 Nov 29, 2023
@kon-angelo
Copy link
Contributor Author

@MartinWeindel I reimplemented the inventory on top of the whiteboard.

The reason the inventory had a separate implementation rather just storing the IDs, is that it has a bit of special logic with regards to additions/deletions that would not apply for other providers. So pushing more tight integration doesn't make sense from my view.

@gardener-robot-ci-3 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 Nov 29, 2023
Copy link
Member

@MartinWeindel MartinWeindel left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks to @elchead for all the initial work and @kon-angelo for taking care of for both the big picture and all the details.

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review needs/second-opinion Needs second review by someone else labels Nov 29, 2023
@gardener-robot-ci-2 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 Nov 29, 2023
@kon-angelo kon-angelo merged commit e8c8533 into gardener:master Dec 1, 2023
13 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Dec 1, 2023
@kon-angelo kon-angelo deleted the terraform-remove-new branch December 1, 2023 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/azure Microsoft Azure platform/infrastructure reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants