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 multiple facilities, metros and reservation IDs #7

Merged
merged 6 commits into from
Aug 25, 2021

Conversation

deitch
Copy link
Collaborator

@deitch deitch commented Aug 24, 2021

Signed-off-by: Avi Deitcher [email protected]

What this PR does / why we need it:

Adds support for:

  • metros
  • multiple facilities
  • using reserved instances, both with constrained only to reserved instances and growth to on-demand

Special notes for your reviewer:

This has not been properly tested yet. I need @rfranzke to review and make sure it aligns with gardener-extension-provider-packet 154, and then I can test it.

@rfranzke please take make sure I captured your target resource API correctly.

Release note:

Support for metros, multiple facilities, reservation IDs

@deitch deitch requested a review from rfranzke August 24, 2021 15:55
@gardener-robot
Copy link
Contributor

@deitch Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Aug 24, 2021
@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 Aug 24, 2021
@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 Aug 24, 2021
kubernetes/machine-class.yaml Outdated Show resolved Hide resolved
kubernetes/machine-class.yaml Outdated Show resolved Hide resolved
kubernetes/machine-class.yaml Outdated Show resolved Hide resolved
pkg/provider/apis/provider_spec.go Outdated Show resolved Hide resolved
pkg/provider/apis/provider_spec.go Outdated Show resolved Hide resolved
pkg/provider/core.go Show resolved Hide resolved
pkg/provider/core.go Show resolved Hide resolved
Signed-off-by: Avi Deitcher <[email protected]>
@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 Aug 25, 2021
@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 Aug 25, 2021
@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 Aug 25, 2021
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Sorry, I was wrong, we will create one MachineClass per facility, so you need exactly this value in the providerSpec (not a list).

kubernetes/machine-class.yaml Outdated Show resolved Hide resolved
kubernetes/machine-class.yaml Outdated Show resolved Hide resolved
pkg/provider/apis/provider_spec.go Show resolved Hide resolved
pkg/provider/core.go Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Aug 25, 2021
@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 Aug 25, 2021
@deitch
Copy link
Collaborator Author

deitch commented Aug 25, 2021

Looks like commenting is back (sort of).

Sorry, I was wrong, we will create one MachineClass per facility, so you need exactly this value in the providerSpec (not a list).

I didn't understand it that way. Doesn't each worker pool in the Shoot spec (which has one metro/region and 0 or more facilities/zones in that metro) lead to one MachineDeployment, which has one MachineClass? I think what we have is correct?

@rfranzke
Copy link
Member

OK, I missed that the EQXM can take multiple facilities when a device is created (contrary/different compared to how it's done for hyperscalers). In that case, forget about the previous review, it should be all good as is.

@deitch
Copy link
Collaborator Author

deitch commented Aug 25, 2021

I cannot get in to see the CI build error. Can you see what it is, so I can fix it? After that, I will test against live EQXM, and then we can merge in.

Signed-off-by: Avi Deitcher <[email protected]>
@deitch
Copy link
Collaborator Author

deitch commented Aug 25, 2021

OK, migration removed. Let's see if this passes.

@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 Aug 25, 2021
@deitch deitch marked this pull request as ready for review August 25, 2021 08:30
@deitch deitch requested review from a team as code owners August 25, 2021 08:30
@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 Aug 25, 2021
@deitch
Copy link
Collaborator Author

deitch commented Aug 25, 2021

CI is clean!

@rfranzke
Copy link
Member

/hold until final tests were conducted

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Aug 25, 2021
@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 Aug 25, 2021
@deitch
Copy link
Collaborator Author

deitch commented Aug 25, 2021

I had to change the manifests and add a debug log line. Already tested:

  • metro without facilities
  • metro with facilities

Will be testing with reservations later today, then this can go in.

@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 Aug 25, 2021
@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 Aug 25, 2021
Signed-off-by: Avi Deitcher <[email protected]>
@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 Aug 25, 2021
@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 Aug 25, 2021
@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 Aug 25, 2021
@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 Aug 25, 2021
@deitch deitch merged commit ae2d528 into gardener:main Aug 25, 2021
@deitch deitch deleted the metros-and-facilities branch August 25, 2021 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/review Needs review reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants