Skip to content
This repository has been archived by the owner on Apr 7, 2020. It is now read-only.

Improve amount of controller reconciliations #138

Merged
merged 4 commits into from
Jun 18, 2019
Merged

Improve amount of controller reconciliations #138

merged 4 commits into from
Jun 18, 2019

Conversation

timuthy
Copy link
Contributor

@timuthy timuthy commented Jun 12, 2019 •
edited
Loading

What this PR does / why we need it:
This PR generally reworks predicates and event mappers of controllers to improve the amount of reconciliations to its required minimum.

This includes the following:

  • Add predicates to controllers which watch cluster resources. These predicates check a Generation change inside the cluster resource, especially of the shoot, rather than a Generation change of the cluster resource itself. The Generation of a cluster can change frequently and thus triggered an unnecessary amount of reconciliations.

  • If a resource is watched and mapped to another resource (e.g. Cluster -> Infrastructure), an update event is sent twice to the respective map function. This causes also a second reconciliation which should be avoided.

Special notes for your reviewer:
Additional changes in this PR:

  • Examples for OSC controllers have been updated, aligned.
  • Mappings for Certificate-Service have been refactored.
  • Enable to use sha versions for Gardener-Extension Docker images.

Release note:

The amount of required reconciliations for `Worker`, `ControlPlane` and `CertificateService` controllers has been improved.

@timuthy timuthy added kind/enhancement Enhancement, improvement, extension priority/normal Standard backlog priority, that can be worked on now or later reviewed/do-not-merge Has no approval for merging, may not be merged as it may break things or be of poor quality size/m A few days of work or medium change topology/seed Affects Seed clusters labels Jun 12, 2019
@timuthy timuthy requested a review from stoyanr as a code owner June 12, 2019 11:41
@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) 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 Jun 12, 2019
@timuthy timuthy changed the title Refine cluster watch predicates [DON'T MERGE] Refine cluster watch predicates Jun 12, 2019
zanetworker
zanetworker reviewed Jun 12, 2019
View reviewed changes
Copy link
Contributor

@zanetworker zanetworker left a comment

Choose a reason for hiding this comment

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

Cool :)

pkg/controller/predicate.go Outdated Show resolved Hide resolved
pkg/controller/worker/controller.go Outdated Show resolved Hide resolved
pkg/controller/extension/mapper.go Outdated Show resolved Hide resolved
pkg/controller/extension/mapper.go Outdated Show resolved Hide resolved
pkg/controller/predicate.go Outdated Show resolved Hide resolved
pkg/controller/predicate.go Outdated Show resolved Hide resolved
@timuthy timuthy force-pushed the enhancement/cert-service-cluster-update branch from 23acbc5 to 64a32e6 Compare June 13, 2019 11:16
@timuthy timuthy 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 needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 13, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 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 Jun 13, 2019
@timuthy timuthy force-pushed the enhancement/cert-service-cluster-update branch from f2f5e60 to 55e31c7 Compare June 13, 2019 13:21
@timuthy timuthy changed the title [DON'T MERGE] Refine cluster watch predicates [DON'T MERGE] Improve amount of controller reconciliations Jun 13, 2019
@timuthy timuthy 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 needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 13, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 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 Jun 13, 2019
@timuthy timuthy added size/l A few weeks of work or large change and removed size/m A few days of work or medium change labels Jun 13, 2019
@timuthy timuthy force-pushed the enhancement/cert-service-cluster-update branch from 55e31c7 to 6b90f53 Compare June 13, 2019 15:10
@timuthy timuthy 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 needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 13, 2019
@timuthy timuthy force-pushed the enhancement/cert-service-cluster-update branch from 6b90f53 to 811a908 Compare June 14, 2019 07:58
@gardener-robot-ci-1 gardener-robot-ci-1 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 Jun 14, 2019
@timuthy timuthy 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 needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 14, 2019
@timuthy timuthy changed the title [DON'T MERGE] Improve amount of controller reconciliations Improve amount of controller reconciliations Jun 14, 2019
@timuthy timuthy removed the reviewed/do-not-merge Has no approval for merging, may not be merged as it may break things or be of poor quality label Jun 14, 2019
rfranzke
rfranzke previously approved these changes Jun 18, 2019
View reviewed changes
Copy link
Contributor

@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.

/lgtm

@timuthy timuthy force-pushed the enhancement/cert-service-cluster-update branch from 811a908 to fc4773e Compare June 18, 2019 10:37
@timuthy timuthy added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 18, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 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 Jun 18, 2019
timuthy and others added 4 commits June 18, 2019 13:04
@timuthy
Improve cluster resource predicates
de9c64f
@timuthy
Improve example manifests
b524c33
@timuthy
Allow sha images for controllers
7dcd0a3
@timuthy @adracus
Handle update events for event handlers correctly
bcdbf1f 
If a resource is watched and mapped to another resource, an update
event is sent twice to the respective map function. This causes also
a second reconciliation which should be avoided.

Co-authored-by: Axel Christ <axel.christ@sap.com>
@timuthy timuthy force-pushed the enhancement/cert-service-cluster-update branch from fc4773e to bcdbf1f Compare June 18, 2019 11:07
@rfranzke rfranzke 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 Jun 18, 2019
rfranzke
rfranzke approved these changes Jun 18, 2019
View reviewed changes
Copy link
Contributor

@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.

/lgtm

@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 Jun 18, 2019
@rfranzke rfranzke merged commit 842f29d into gardener-attic:master Jun 18, 2019
@timuthy timuthy deleted the enhancement/cert-service-cluster-update branch July 30, 2019 08:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zanetworker zanetworker zanetworker left review comments

@rfranzke rfranzke rfranzke approved these changes

@stoyanr stoyanr Awaiting requested review from stoyanr

Assignees
No one assigned
Labels
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) priority/normal Standard backlog priority, that can be worked on now or later size/l A few weeks of work or large change topology/seed Affects Seed clusters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timuthy @zanetworker @rfranzke @gardener-robot-ci-1