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

Add support for deployments in existing subnet #780

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

kon-angelo
Copy link
Contributor

How to categorize this PR?

/area control-plane
/kind enhancement
/platform openstack

What this PR does / why we need it:

/invite @Kumm-Kai

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Add support for creating shoots in existing subnets 

@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else labels May 28, 2024
@gardener-robot
Copy link

@kon-angelo Command "/InviteCommand" failed with "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the gardener/gardener-extension-provider-openstack repository.".

Additional Information
Redacted in public. Check backend logs.

@gardener-robot gardener-robot added area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension needs/review Needs review platform/openstack OpenStack platform/infrastructure size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) labels May 28, 2024
@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 May 28, 2024
@gardener-robot-ci-3 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 May 28, 2024
@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 May 28, 2024
@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 May 28, 2024
@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 Jun 10, 2024
@kon-angelo kon-angelo added the area/ipcei IPCEI (Important Project of Common European Interest) label Jun 27, 2024
@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Jul 24, 2024
@gardener-robot
Copy link

@kon-angelo You need rebase this pull request with latest master branch. Please check.

@gardener-robot-ci-2 gardener-robot-ci-2 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 Sep 11, 2024
@gardener-robot-ci-1 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 Sep 16, 2024
@kon-angelo
Copy link
Contributor Author

/test

@testmachinery
Copy link

testmachinery bot commented Sep 16, 2024

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

+---------------------+-----------------------------+-----------+----------+
|        NAME         |            STEP             |   PHASE   | DURATION |
+---------------------+-----------------------------+-----------+----------+
| bastion-test        | bastion-test                | Succeeded | 5m2s     |
| infrastructure-test | infrastructure-test-flow    | Succeeded | 7m46s    |
| infrastructure-test | infrastructure-test-migrate | Succeeded | 8m1s     |
| infrastructure-test | infrastructure-test-recover | Failed    | 9m38s    |
| infrastructure-test | infrastructure-test         | Omitted   | 0s       |
+---------------------+-----------------------------+-----------+----------+

@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 Sep 16, 2024
@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 7, 2024
@kon-angelo
Copy link
Contributor Author

/test

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Oct 21, 2024
@testmachinery
Copy link

testmachinery bot commented Oct 21, 2024

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

+---------------------+-----------------------------+-----------+----------+
|        NAME         |            STEP             |   PHASE   | DURATION |
+---------------------+-----------------------------+-----------+----------+
| infrastructure-test | infrastructure-test-flow    | Succeeded | 8m3s     |
| infrastructure-test | infrastructure-test-migrate | Succeeded | 7m36s    |
| infrastructure-test | infrastructure-test-recover | Succeeded | 8m0s     |
| bastion-test        | bastion-test                | Succeeded | 5m9s     |
| infrastructure-test | infrastructure-test         | Succeeded | 7m37s    |
+---------------------+-----------------------------+-----------+----------+

@gardener-robot gardener-robot added needs/review Needs review needs/second-opinion Needs second review by someone else and removed needs/review Needs review labels Oct 21, 2024
@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 21, 2024
@gardener-robot gardener-robot removed the reviewed/lgtm Has approval for merging label Oct 21, 2024
@gardener-robot-ci-1 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 Oct 21, 2024
@kon-angelo kon-angelo requested a review from hebelsan October 21, 2024 12:18
Copy link
Contributor

@hebelsan hebelsan left a comment

Choose a reason for hiding this comment

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

We should probably add documentation for bringing your own subnetID.

return fctx.ensureNewSubnet(ctx)
}

func (fctx *FlowContext) ensureConfiguredSubnet(_ context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the context param if we don't use it.
Actually, I'd pass the subnetID as string param instead of the context.
This would make it easier to spot that it's checked for nil.

@@ -995,7 +990,6 @@ func (vp *valuesProvider) addCSIManilaValues(
values["openstack"] = map[string]interface{}{
"availabilityZones": vp.getAllWorkerPoolsZones(cluster),
"shareNetworkID": shareNetworkID,
"shareClient": infrastructure.WorkersCIDR(infraConfig),
Copy link
Contributor

Choose a reason for hiding this comment

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

How can the "shareClient" be set if we remove it from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't set it basically. The shareClient constricts who can access the share, for which at the moment we have no strict need to do, it even restrict customer use cases. The template doesn't necessarily need to change, we just choose not to configure this.

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 area/ipcei IPCEI (Important Project of Common European Interest) kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/second-opinion Needs second review by someone else platform/openstack OpenStack platform/infrastructure size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants