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.

Sign up for GitHub

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

Jump to bottom

Add checks for seed existence in the create state #623

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add checks for seed existence in the create state #623

wants to merge 10 commits into from
+165 −29

Conversation

akgalwas
Copy link
Contributor

@akgalwas akgalwas commented Jan 27, 2025 •
edited
Loading

Description

Changes proposed in this pull request:

  • If the seed for the region doesn't exist the shoot is not created, and the failed state is set

Related issue(s)
#561

@akgalwas akgalwas requested a review from a team as a code owner January 27, 2025 14:38
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 27, 2025
@kyma-bot kyma-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 28, 2025
tobiscr
tobiscr requested changes Jan 29, 2025
View reviewed changes
internal/controller/runtime/fsm/runtime_fsm_create_shoot.go Outdated
}

if !seedAvailable {
msg := fmt.Sprintf("Seed for region %s doesn't exist", s.instance.Spec.Shoot.Region)
Copy link
Contributor

@tobiscr tobiscr Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Alignment with KEB is:

  • We provide in the error a list of available regions which include a seed
  • We use a dedicated Error-Type for Seed-Not-Found reason

Copy link
Contributor

@tobiscr tobiscr Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Long term solution is to reply an JSON string in the error-reply to be able to provide structured data to KEB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

m00g3n
m00g3n reviewed Jan 31, 2025
View reviewed changes
internal/controller/runtime/fsm/seed.go Outdated
var seedList gardener_types.SeedList
var regionsWithSeeds []string

err := client.List(context.TODO(), &seedList)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can pass context from sFnCreateShoot function state 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.

Done.

@tobiscr tobiscr requested review from tobiscr February 3, 2025 10:30
@kyma-bot kyma-bot added the lgtm Looks good to me! label Feb 3, 2025
@Disper Disper self-requested a review February 3, 2025 11:27
@Disper
Copy link
Member

Disper commented Feb 3, 2025 •
edited
Loading

During verification I've noticed following aspects that I'm not sure about:

  1. Even though implemented check fails, the clusters creation is still requested in Gardener UI (no status and readiness icons)
image
  1. How should it be configured? Will this be passed from KEB or should we perhaps have some per landscape configuration for enforceSeedLocation?

@Disper Disper added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2025
Disper
Disper approved these changes Feb 3, 2025
View reviewed changes
Copy link
Member

@Disper Disper left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Regarding previous comment, I've learned from @akgalwas that

  1. It is intended for it to work like that (regarding resources consumption, there is a separate issue for labeling clusters for billings)
  2. Will be configured in KEB (not yet done)

@tobiscr could you check if that's how you envisioned it should work? If so, please remove the do-not-merge/hold label to merge it (I've left LGTM, but I won't be available tomorrow to approve it).

@Disper Disper enabled auto-merge February 3, 2025 15:39
@tobiscr
Copy link
Contributor

tobiscr commented Feb 3, 2025 •
edited
Loading

During verification I've noticed following aspects that I'm not sure about:

Even though implemented check fails, the clusters creation is still requested in Gardener UI (no status and readiness icons)

This would indicate a misbehaviour - the cluster should not be created if we cannot fulfil the requirement that Seed and Shoot are in the same region. It would not help to create the cluster as customers don't want a shoot in a different region than the seed.

How should it be configured? Will this be passed from KEB or should we perhaps have some per landscape configuration for enforceSeedLocation?

When a customer creates a Kyma runtime via BTP cockpit, they can set a checkbox to inform us, that the created shoot has to be in the same region as the seed. So, yes - KEB is setting the field in RuntimeCR, but the decision if this field is TRUE or FALSE comes from customers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m00g3n m00g3n m00g3n left review comments

@Disper Disper Disper approved these changes

@tobiscr tobiscr Awaiting requested review from tobiscr

Assignees

@Disper Disper

@tobiscr tobiscr

Labels
cla: yes Indicates the PR's author has signed the CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@akgalwas @Disper @tobiscr @m00g3n @kyma-bot