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

KIM - Fix of the problem with removing exiting shoots using client.Update function to change shoot workers #650

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
+54 −1
Open

KIM - Fix of the problem with removing exiting shoots using client.Update function to change shoot workers #650

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
18385f5
Use KIM Field Manager to create shoot
koala7659 Feb 6, 2025
931d9e1
test client.Patch for shoot create
koala7659 Feb 6, 2025
4466d61
adding dns and toleration values for patch converter
koala7659 Feb 6, 2025
5173c6a
adjust unit tests
koala7659 Feb 6, 2025
3fc113f
Use update call in patch state to replace workers collection in the s…
koala7659 Feb 7, 2025
112b970
Fix Update call and add intercepter function to simulate Shoot Update
koala7659 Feb 9, 2025
1354f85
Experimental only Shoot Workers update during patch
koala7659 Feb 9, 2025
f31b662
Use full Shoot object to update Shoot workers
koala7659 Feb 9, 2025
b4129c7
Use combination of copy shoot + update + patch to update shoot workers
koala7659 Feb 9, 2025
5883a98
Update unit tests
koala7659 Feb 9, 2025
6375cf3
Merge branch 'main' into kim-fieldManager-shoot-create
Disper Feb 10, 2025
49e15ed
Add some comments to explain additional client.Update call to replace…
koala7659 Feb 10, 2025
34fc57a
Change function workersAreDifferent into workersAreEqual
koala7659 Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,25 @@ func sFnPatchExistingShoot(ctx context.Context, m *fsm, s *systemState) (stateFn

m.log.Info("Shoot converted successfully", "Name", updatedShoot.Name, "Namespace", updatedShoot.Namespace)

copyShoot := s.shoot.DeepCopy()
copyShoot.Spec.Provider.Workers = updatedShoot.Spec.Provider.Workers

err = m.ShootClient.Update(ctx, copyShoot,
Copy link
Member

@Disper Disper Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

could you just shortly describe in a comment why this second update is needed?

Also, do you think it would be worth to extract this to another method such as `handleWorkerPoolsUpdate() to clearly distinguish which code is responsible for this exceptional logic?

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it would make sense to check if shootWorkers differ from the ones specified in runtimeCR and doing additional update only if they do.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is the whole workers comparison and matching logic inside converter, this idea would imply this code would have to be duplicated. I can try to come up with something

&client.UpdateOptions{
FieldManager: fieldManagerName,
})

if err != nil {
if k8serrors.IsConflict(err) {
m.log.Info("Gardener shoot for runtime is outdated, retrying", "Name", s.shoot.Name, "Namespace", s.shoot.Namespace)
return updateStatusAndRequeueAfter(m.RCCfg.GardenerRequeueDuration)
}

m.log.Error(err, "Failed to update shoot worker list, exiting with no retry")
m.Metrics.IncRuntimeFSMStopCounter()
return updateStatePendingWithErrorAndStop(&s.instance, imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonProcessingErr, fmt.Sprintf("Gardener API shoot patch error: %v", err))
}

err = m.ShootClient.Patch(ctx, &updatedShoot, client.Apply, &client.PatchOptions{
FieldManager: fieldManagerName,
Force: ptr.To(true),
Expand Down
12 changes: 12 additions & 0 deletions internal/controller/runtime/fsm/testing/fake_k8s_client_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,15 @@ func GetFakePatchInterceptorFn() func(ctx context.Context, client client.WithWat
return nil
}
}

func GetFakeUpdateInterceptorFn() func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
return func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
shoot, ok := obj.(*gardener_api.Shoot)
if !ok {
return client.Update(ctx, obj, opts...)
}
// Update the generation to simulate shoot object being updated using interceptor function.
shoot.Generation++
return nil
}
}
3 changes: 2 additions & 1 deletion internal/controller/runtime/fsm/utilz_for_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ var (
WithObjects(objs...).
WithStatusSubresource(objs...).
WithInterceptorFuncs(interceptor.Funcs{
Patch: fsm_testing.GetFakePatchInterceptorFn(),
Patch: fsm_testing.GetFakePatchInterceptorFn(),
Update: fsm_testing.GetFakeUpdateInterceptorFn(),
}).Build()

return func(fsm *fsm) error {
Expand Down
5 changes: 4 additions & 1 deletion pkg/gardener/shoot/converter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,10 @@ func NewConverterPatch(opts PatchOpts) Converter {
opts.MachineImage.DefaultVersion,
opts.Workers,
opts.InfrastructureConfig,
opts.ControlPlaneConfig))
opts.ControlPlaneConfig),
extender2.NewDNSExtender(opts.DNS.SecretName, opts.DNS.DomainPrefix, opts.DNS.ProviderType),
extender2.ExtendWithTolerations,
)

extendersForPatch = append(extendersForPatch,
extensions.NewExtensionsExtenderForPatch(opts.AuditLogData, opts.Extensions),
Expand Down
1 change: 0 additions & 1 deletion pkg/gardener/shoot/converter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ func TestConverter(t *testing.T) {
assert.Equal(t, "1.30", shoot.Spec.Kubernetes.Version)
assert.Equal(t, "gardenlinux", shoot.Spec.Provider.Workers[0].Machine.Image.Name)
assert.Equal(t, "1592.2.0", *shoot.Spec.Provider.Workers[0].Machine.Image.Version)
assert.Nil(t, shoot.Spec.DNS)

extensionLen := len(shoot.Spec.Extensions)
require.Equalf(t, extensionLen, 5, "unexpected number of extensions: %d, expected: 5", extensionLen)
Expand Down
Loading